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)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
People
(Reporter: martijn.martijn, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: regression, rtl, testcase)
Attachments
(3 files)
|
382 bytes,
text/html
|
Details | |
|
355 bytes,
text/html
|
Details | |
|
4.43 KB,
patch
|
uriber
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
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.
| Reporter | ||
Comment 1•18 years ago
|
||
| Reporter | ||
Comment 2•18 years ago
|
||
| Reporter | ||
Comment 3•18 years ago
|
||
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
Comment 4•18 years ago
|
||
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.
| Assignee | ||
Updated•18 years ago
|
Keywords: regression
| Assignee | ||
Comment 5•18 years ago
|
||
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 | ||
Updated•18 years ago
|
OS: Windows XP → All
Hardware: PC → All
Comment 6•18 years ago
|
||
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+
| Assignee | ||
Updated•18 years ago
|
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+
| Assignee | ||
Comment 8•18 years ago
|
||
> 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
Comment 9•18 years ago
|
||
I'm surprised the setTimeout of 10ms in the reftest doesn't cause problems.
Comment 10•17 years ago
|
||
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
Comment 11•16 years ago
|
||
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
Comment 12•16 years ago
|
||
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.)
Comment 13•16 years ago
|
||
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. :)
| Assignee | ||
Comment 14•16 years ago
|
||
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.
Description
•