Closed Bug 375716 Opened 18 years ago Closed 18 years ago

Incremental reflow bug with direction: rtl and bidi-override

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: regression, rtl, testcase)

Attachments

(3 files)

See upcoming testcases. The two testcases should render the same, but they don't in current trunk builds. This regressed between 2007-01-06 and 2007-01-07, I think a regression from bug 365909.
Attached file testcase
Attached file reference rendering
Uhm, I don't really know what the correct rendering is. It seems actually, that Mozilla used to do it always wrong. So that means this would not be a regression.
Keywords: regression
The rendering of the testcase seems more correct than that of the "reference": the last (non-bolded) "text" should certainly not be reversed. Not sure about the spaces, though.
Keywords: regression
Attached patch Patch rev. 1Splinter Review
Actually, there is a subtle change in the logic... Bug 365909 made it so that the directionalFrame was not created at the start of the loop when !frame->GetPrevContinuation() is true, this is correct but it also affects the test for creating the 'kPDF' directional frame at the end. This patch restores the logic for creating the 'kPDF' frame as before bug 365909 and cleans up the code a bit while still not leaking I hope.
Assignee: nobody → mats.palmgren
Status: NEW → ASSIGNED
Attachment #259935 - Flags: review?(uriber)
OS: Windows XP → All
Hardware: PC → All
Comment on attachment 259935 [details] [diff] [review] Patch rev. 1 r=me. Hopefully this time I didn't miss anything.
Attachment #259935 - Flags: review?(uriber) → review+
Attachment #259935 - Flags: superreview?(dbaron)
Comment on attachment 259935 [details] [diff] [review] Patch rev. 1 sr=dbaron, and sorry for the long delay here. Please add Martijn's testcases to the reftests in layout/reftests/bugs/ .
Attachment #259935 - Flags: superreview?(dbaron) → superreview+
> Please add Martijn's testcases to the reftests in layout/reftests/bugs/ They were not reliable so I had to rewrite them. Checked in to trunk at 2007-05-06 03:29 PDT. -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
I'm surprised the setTimeout of 10ms in the reftest doesn't cause problems.
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
This bug's reftest needs to use "reftest-wait". It just failed twice, with the testcase still having large text: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1257905810.1257909197.9746.gz
Oops, sorry -- it turns out I was looking at the source for the wrong file (the -ref version) when I saw the lack of "reftest-wait". 375716-1.html does indeed use reftest-wait, as it should. (I'm not sure what caused the tinderbox failure, though... In the failure's snapshot, the test version is definitely larger than the ref version. Perhaps it's a real regression from a recent checkin.)
Ah, I was right in comment 11 after all! Was confused because reftest-wait was there in my up-to-date m-c build. Turns out Mats *just* fixed this in bug 527806 / http://hg.mozilla.org/mozilla-central/rev/d4f8b4b9ccac , so I had the already-fixed version in comment 12. :)
Yep, sorry for the confusion, I should have made a note in this bug too.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: