Closed Bug 525788 Opened 12 years ago Closed 12 years ago

rtl tests break for getClientRects and getBoundingClientRect in DOM Range

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: liucougar, Assigned: smontagu)

References

Details

Attachments

(4 files, 1 obsolete file)

This is a spin off bug 396392: two rtl tests are not executed for rtl, see bug 396392 comment 32

in addition, more tests can be added for 396392 (see bug 396392 comment 40)
Attached patch Patch 1 (obsolete) — Splinter Review
This adds back the "mixed-dir" tests from attachment 409474 [details] [diff] [review], changing them to use Latin text with rtl-override instead of Hebrew text (which was causing the tests to fail because of different heights in the English and Hebrew fonts), but doesn't yet include the other tests (spanAcrossLines and textAcrossLines) which are failing on Mac, nor any new tests for partial selection in mixed-directional text.
Assignee: nobody → smontagu
Attachment #417160 - Flags: review?(roc)
Patch 1 pushed in http://hg.mozilla.org/mozilla-central/rev/4ed95e61ee01. Leaving bug open for further work.
... but backed out in http://hg.mozilla.org/mozilla-central/rev/88ee36d9eb53 because of failures on Linux this time :S
Attached patch Patch 1aSplinter Review
This is a log of the failure on Linux:

25792 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_range_bounds.html | isLTR range.getClientRects for mixeddir: item at 1 - got "{left:50,right:50,top:151,bottom:151,width:0,height:0}", expected "{left:50,right:50,top:139,bottom:154,width:0,height:15}"
25846 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_range_bounds.html | isRTL range.getClientRects for mixeddir: item at 1 - got "{left:50,right:50,top:151,bottom:151,width:0,height:0}", expected "{left:50,right:50,top:139,bottom:154,width:0,height:15}"

The "expected width: 0" makes me think that the failure is caused by the unprintable directional override characters, so this version uses a <bdo> element instead. It passes locally, but I'm checking it on tryserver before asking for review.
Attachment #417160 - Attachment is obsolete: true
Attachment #418809 - Flags: review?(roc)
Comment on attachment 418809 [details] [diff] [review]
Patch 1a

The test passed on all platforms on tryserver (WINNT 5.2 try hg unit test failed with a timeout, but http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1261474059.1261483904.30217.gz&fulltext=1 shows that this specific test passed)
This is a checkpoint for the sake of clarity.
With the last attachment there are two kinds of failures:

With dir=rtl, the tests spanAcrossLines and textAcrossLines fail on all platforms because range.GetClientRects() returns different results in LTR and RTL. In principle this is expected, but I wonder what the right thing to do is when we get empty rects. This happens when you have left-to-right text at the end of a line in a right-to-left paragraph: whitespace at the end of the line is reordered to the left side of the line and so is non-contiguous with the end of the left-to-right text at the right side of the line; but the whitespace is collapsed so we end up with an empty rect.

The second failure, which is Mac-specific, looks like some kind of rounding error: we get things like:
got "{left:8,right:92,top:177.1999969482422,bottom:189.1999969482422,width:84,height:12}", expected "{left:8,right:92,top:177.20001220703125,bottom:189.20001220703125,width:84,height:12}"
cc-ing roc for feedback on the last comment.
Probably should compare the values after rounding them to the nearest 1/256 of a pixel or something like that.
This fixes the Mac issue as suggested in comment 10. The RTL issue turns out on further analysis to be at least partly caused by an actual reordering bug, reported as bug 536963.
Attachment #419305 - Flags: review?(roc)
Attachment #419305 - Attachment is patch: true
Attachment #419305 - Attachment mime type: application/octet-stream → text/plain
Attachment #419283 - Attachment is patch: true
Attachment #419283 - Attachment mime type: application/octet-stream → text/plain
Depends on: 536963
With the patch for bug 536963 this passes tryserver.
Attachment #419462 - Flags: review?(roc)
Checked in all outstanding patches plus an extra test for visually non-contiguous selection in mixed direction text.

http://hg.mozilla.org/mozilla-central/rev/a0044e7e0228
http://hg.mozilla.org/mozilla-central/rev/1b0db40fe73b
http://hg.mozilla.org/mozilla-central/rev/26672e99e008
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.