Opened 10 years ago

Closed 10 years ago

#3034 closed Bug (fixed)

Element document offset position not correct

Reported by: Garry Yao Owned by: Frederico Caldeira Knabben
Priority: Must have (possibly next milestone) Milestone: CKEditor 3.0
Component: General Version: SVN (FCKeditor) - Retired
Keywords: Review+ Cc:

Description

The coordinates returned from CKEDITOR.dom.element::getDocumentPosition is a few pixes shifted.

Attachments (8)

test-element-getDocumentPosition.patch (1.9 KB) - added by Garry Yao 10 years ago.
Unit test case
Picture 27.png (135.7 KB) - added by Martin Kou 10 years ago.
Screenshot in Firefox.
Picture 25.png (124.2 KB) - added by Martin Kou 10 years ago.
Screenshot in IE.
Picture 28.png (161.5 KB) - added by Martin Kou 10 years ago.
Another screenshot in Firefox, which shows the 350,450 answer is correct in FF.
3034.patch (3.3 KB) - added by Martin Kou 10 years ago.
3034_2.patch (3.8 KB) - added by Martin Kou 10 years ago.
3034_3.patch (4.6 KB) - added by Martin Kou 10 years ago.
3034_4.patch (1.7 KB) - added by Frederico Caldeira Knabben 10 years ago.

Download all attachments as: .zip

Change History (25)

Changed 10 years ago by Garry Yao

Unit test case

comment:1 Changed 10 years ago by Frederico Caldeira Knabben

Priority: NormalHigh

Fails in IE only. This is the kind of test to be run in quirks and standard modes.

comment:2 Changed 10 years ago by Martin Kou

Owner: set to Martin Kou
Status: newassigned

comment:3 Changed 10 years ago by Martin Kou

Actually, the answers in both IE and Firefox are wrong, and the reference answer (350, 450) is wrong too - YUI's setXY() isn't pixel accurate.

To get the real answer, you'll have to measure the pixel position from a screenshot in GIMP or Photoshop. You can get screenshots with the target by changing the scroll CSS class to overflow: visible; and adding background color to to the target div, and then change the test logger's div to display: none after the test.

So here're the results I've got from GIMP: IE6/WinXP: (348, 448) Firefox 3/Mac OS X: (358, 450)

I've found a logic error getDocumentPosition() which has caused to +10, +10 error in IE6. But I'm still investigating why the result in Firefox is wrong.

Changed 10 years ago by Martin Kou

Attachment: Picture 27.png added

Screenshot in Firefox.

Changed 10 years ago by Martin Kou

Attachment: Picture 25.png added

Screenshot in IE.

Changed 10 years ago by Martin Kou

Attachment: Picture 28.png added

Another screenshot in Firefox, which shows the 350,450 answer is correct in FF.

comment:4 Changed 10 years ago by Martin Kou

Ok, I've just tried the test case in Firefox again with the overflow and background-color CSS already embedded in the test case HTML - which means less fiddling in Firebug. And the screenshot measurement result did come out with (350, 450). So yes, the function is returning correct results in Firefox.

So the only case left is IE6/WinXP, but the answer should be (348, 448) there.

comment:5 Changed 10 years ago by Garry Yao

While it's great to see the accurate measurement with pixel from GIMP, I guess we just need to get the 'right' result which browser think is right, which means that by specify the left/top css style to a absolute positioned element within body, with the value return from 'CKEDITOR.dom.element::getDocumentPosition', if the element is overlapping with the measuring target accurately, then the function should be considered right.

comment:6 Changed 10 years ago by Martin Kou

If it's simply setting the top/left/position=absolute of a div under the body then there'd be no need to do the measurements in GIMP. But the (350, 450) here is set into a few nested divs with static, relative and absolute positioning, via YUI's setXY(). So (350, 450) is definitely not "what the browser thinks is right" - it's what YUI thinks is right.

The browser itself does not tell you directly the position of a nested element whose elements has been static, relative and absolute positioned. So unless there's some prove that YUI's positioning algorithm is mathematically correct, it should be better to do the measurements in GIMP - by calculating the delta between the (0, 0) point in the document (which can be obtained by adding an absolutely positioned colored div into the body) and the top left corner pixel of the target div.

comment:7 Changed 10 years ago by Martin Kou

Some other measurement results:

IE7/WinXP: (348, 448) IE8/WinVista x64 (in IE7 compat mode): (349, 448) (yes it's 349 =_= - the 0,0 in IE8/Vista eats one pixel into the window border)

I guess the test case should be modified to accept (348 or 349, 448) for IE, and remain the same for the other browsers.

Changed 10 years ago by Martin Kou

Attachment: 3034.patch added

comment:8 Changed 10 years ago by Martin Kou

Keywords: Review? added

comment:9 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The real sense of this ticket is providing a fix for #2995, which has been marked as a DUP for this one. The provided patch, even if correct, still don't fix #2995, so I believe the TC is not complete for our needs.

comment:10 Changed 10 years ago by Martin Kou

Ok, I think I've finally found the culprit.

It seems I'll need to account for the clientTop and clientLeft properties for some elements in IE as well - the offsetWidth and offsetTop don't always include the borders in IE. I've confirmed the browser bug with at least the <fieldset> element, but a bit more investigation is needed for accurate results.

Changed 10 years ago by Martin Kou

Attachment: 3034_2.patch added

comment:11 Changed 10 years ago by Martin Kou

Keywords: Review? added; Review- removed

comment:12 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

Ok... but... is it an IE only issue?

I'm looking at the toolbar combos panels:

Before patch:

  • IE : mispositioned
  • FF : mispositioned
  • Safari : mispositioned
  • Opera : correctly positioned

After patch:

  • IE : correctly positioned
  • FF : mispositioned
  • Safari : correctly positioned
  • Opera : mispositioned

So, the patch fixed it for IE and Safari, breaking Opera. No changes for Firefox instead.

Am I missing something?

comment:13 Changed 10 years ago by Martin Kou

Keywords: Review? added; Review- removed

I've done quite some experiments - the offsetTop and offsetLeft attributes returned for the cke_text elements are simply off by 1 or 2 pixels in Firefox, Opera, and Safari. Even Firebug's highlight position and layout display give the wrong results. So it seems we can't correct that by fixing getDocumentPosition().

On the other hand, I found putting position: relative on the cke_text CSS class would automatically make getDocumentPosition()'s output correct in Firefox, Opera and Safari. I have no idea why that would work... but there it is.

Changed 10 years ago by Martin Kou

Attachment: 3034_3.patch added

comment:14 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
Owner: changed from Martin Kou to Frederico Caldeira Knabben
Status: assignednew

After a few hours of research, I've found out that clientTop is not an IE bug, and it's actually needed in all browsers (except Opera) to properly find the element position.

Also, by changing position to "relative", you are not really fixing that function, but simply setting the parentOffset property of that element to <body>, which makes the getDocumentPosition calculation much simpler.

I've found a final solution, which looks correct, or at least don't require CSS changes.

Changed 10 years ago by Frederico Caldeira Knabben

Attachment: 3034_4.patch added

comment:15 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed
Status: newassigned

comment:16 Changed 10 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:17 Changed 10 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: assignedclosed

Fixed with [3337].

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy