Closed
Bug 331052
Opened 19 years ago
Closed 18 years ago
RTL <select> inside table is misaligned with incremental reflow
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
People
(Reporter: uriber, Assigned: mkaply)
References
(Blocks 1 open bug, )
Details
(Keywords: regression, rtl, testcase, Whiteboard: [reflow-refactor])
Attachments
(9 files)
On certain RTL pages (especially long ones) I notice that the text in select boxes is misaligned, and partially cut off. Selecting an option from the drop-down, or resizing the font on the page fixes the problem.
This is not 100% reproducabe, but can almost always be reproduced on the page in the URL field, after several reloads.
I could not create a local testcase which would reproduce the problem.
This apparently regressed in 3 stages:
* Until 2006-01-12, everything was fine
* Starting from 2006-01-13, the text became aligned to the left instead of to the right, but was still fully visible. A checkin which might be relevat is bug 51767
* Starting from 2006-02-22 (when bug 299065 landed), the text became shifted to the right, and partially cut-off.
* Starting from 2006-03-16 (landing of bug 192767, bug 96394, and bug 318116), this also seems to affect the nearby button, which is pushed to the next line.
I'll attach screenshots immediately.
Reporter | ||
Comment 1•19 years ago
|
||
This is the correct layout, as seen before 2006-01-13, or after resizing font.
I forgot to mention that these select boxes are near the bottom of the page in the URL field.
Reporter | ||
Comment 2•19 years ago
|
||
This is how this looked between 2006-01-13 and 2006-02-21. Notice that the text left-aligned instead of right aligned.
Reporter | ||
Comment 3•19 years ago
|
||
Between 2006-02-22 and 2006-03-15.
The text is pushed to the right, and the rightmost part of it is cut off.
Reporter | ||
Comment 4•19 years ago
|
||
Since 2006-03-16.
The select boxes themselves are the same as before, but notice that the button is pushed down to a separate line.
I think this picture gallery leads to nowhere. The claim that the thing rendered correctly before 2006-02-12 is at least questionable. On a nonreproducible bug attaching screenshots and then to chase the regression is bound to yield wrong regression dates. Try to add layout breakpoints with javascript to get the thing more reproducible.
Or to be more to the point, a layout bug without a testcase can only be in a unconfirmed state (see http://groups.google.de/group/netscape.public.mozilla.layout/browse_thread/thread/4b44ff47ef062b20/d30ce1ee477eceb0?lnk=st&q=dbaron+layout+bugs+triage&rnum=1&hl=de#d30ce1ee477eceb0)
Reporter | ||
Comment 7•19 years ago
|
||
> Created an attachment (id=215626) [edit]
> screenshot 2006-01-11
>
> I think this picture gallery leads to nowhere. The claim that the thing
> rendered correctly before 2006-02-12 is at least questionable.
Although your screenshot (which is correct) seems to confirm (or at least not contradict) this.
> On a
> nonreproducible bug attaching screenshots and then to chase the regression is
> bound to yield wrong regression dates. Try to add layout breakpoints with
> javascript to get the thing more reproducible.
OK, I'll look around for how to do this.
(In reply to comment #6)
> Or to be more to the point, a layout bug without a testcase can only be in a
> unconfirmed state (see
Yes, my bad. I really should have remembered this. Unfortunately there doesn't seem to be a way to unconfirm the bug now, so I'll try to see if I can create a reproducable testcase.
Reporter | ||
Comment 8•19 years ago
|
||
Minimal testcase, using Hyatt's offsetHeight trick.
With this testcase, I verified that the original regression indeed occured between 2006-01-12 and 2006-01-13.
>Although your screenshot (which is correct) seems to confirm
There is a padding missing on the left side of the text, load the testcase and toggle between the options to see what I mean.
Comment 10•19 years ago
|
||
s/left/right/
Reporter | ||
Comment 11•19 years ago
|
||
(In reply to comment #9)
> >Although your screenshot (which is correct) seems to confirm
> There is a padding missing on the [right] side of the text
That's bug 323040 (fixed on 2006-01-20).
Reporter | ||
Comment 12•19 years ago
|
||
This is a reflow log for the textcase.
The relevant frame here is 0x23a5c10. It appears three times on the log.
As far as I can tell from debugging, it is positioned incorrectly (300 twips to the right) the first time it's reflowed, and then positioned correctly the second time (which is inside the reflow of the combobox).
The third reflow is what's positioning it incorrectly again. And a fourth reflow, which would have corrected this, is missing: when 0x23a8d98 is reflowed again it doesn't reflow its children.
That's about as much as I can make of it right now. I'm not sure where the "real" problem is: the original incorrect positioning, or the fact that the "correction" is missing from the second time around.
Reporter | ||
Comment 14•19 years ago
|
||
This is the reflow log of the same testcase from the 1.8 branch (which works correctly). Notice that the third reflow I referred to above doesn't happen here (the block inside the table cell does not reflow its children).
This still doesn't mean much to me in terms of what's the real bug here, but hopefully it would be useful to one of you guys.
Comment 15•19 years ago
|
||
reflow logs thats a clear language:
wrong case:
area 0x23a7f48 r=0 a=2145,330 c=0,270 cnt=491
xxxxx
area 0x23a7f48 d=60,330 me=1560 o=(-2595,0) 2655 x 390
branch:
area 0x240bed0 r=0 a=2145,330 c=0,270 cnt=2971
xxxx
area 0x240bed0 d=2445,330 me=1320
the result looks pretty different, is obvious that it is completely bogus in the first case.
Comment 16•19 years ago
|
||
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/forms/nsComboboxControlFrame.cpp&rev=1.364&mark=830-845
looks like a hack to me, having written enough of them makes it fairly easy to recognize them when one sees them.
I would say on branch the 0 computed width is happily ignored and on trunk it is honoured. Thats the typical tale with all theses hacks.
Comment 17•19 years ago
|
||
Uri, could you check wether the hack is really still necessary? I guess not, but I don't have good understanding of RTL stuff. I would prefer not to touch this file at all, it raises blood pressure whenever I see it.
Reporter | ||
Comment 18•19 years ago
|
||
(In reply to comment #17)
> Uri, could you check wether the hack is really still necessary? I guess not,
> but I don't have good understanding of RTL stuff. I would prefer not to touch
> this file at all, it raises blood pressure whenever I see it.
>
I'll try to check, although I don't know this code at all.
Simon - do you know anything about this?
Comment 19•19 years ago
|
||
I would guess that the hack isn't necessary any more: q.v. bug 204531, where roc removed a bidi-specific hack in nsListControlFrame::Reflow()
Has anybody tried removing the hack to see what happens?
Reporter | ||
Comment 20•19 years ago
|
||
(In reply to comment #19)
> Has anybody tried removing the hack to see what happens?
I just did that. Short answer: it doesn't help. I'll opst a reflow log without the hack in a few minutes.
Reporter | ||
Comment 21•19 years ago
|
||
Visually, this looks exactly the same with the hack removed.
Looking at the reflow log, there seems to be some change (the negative offset is much smaller), but the basic scenario is pretty much the same.
Perhaps the IBMBIDI block about 50 lines below also has to be tweked with / removed?
Comment 22•19 years ago
|
||
(In reply to comment #21)
> Perhaps the IBMBIDI block about 50 lines below also has to be tweked with /
> removed?
Removing that block just puts the drop button on the wrong side. I'm deep in something else right now, and will look at this more seriously tomorrow.
By the way, attachment 121711 [details] is a good testcase for selects, though I say so myself.
Comment 23•19 years ago
|
||
The testcase verifies the rtl rendering under a incr. reflow, where it needs to report properly not only the desired size but also the MEW and the maximumWidth because it is in a table. And it gets an unconstrained initial reflow....
Comment 24•19 years ago
|
||
Could be related to bug 331049, even though they regressed at different times.
Comment 25•19 years ago
|
||
area 0x23cb748 d=2145,330 me=1560 o=(-510,0) 2655 x 390
looks like either a button and some padding or a scrollbar gets substracted too much. At least thats the first major difference comparing the reflow log to the ltr case.
Comment 26•19 years ago
|
||
Ignore comment 24; they're not related.
Comment 27•19 years ago
|
||
... well, except perhaps for the observation that in the testcase there, we're not leaving room for the vertical scrollbar when computing the intrinsic size of the dropdown, and in this case we are, and it seems like the different looks like the width of a vertical scrollbar.
Comment 28•19 years ago
|
||
...which is really just because that select has a width.
Reporter | ||
Updated•19 years ago
|
Flags: blocking1.9a1?
Reporter | ||
Comment 29•18 years ago
|
||
The behavior of the testcase changed on 2006-06-15 due to the fix for bug 328168: Now the text appears left-aligned, instead of cut off on the right side. Still, this is incorrect behavior.
Reporter | ||
Comment 30•18 years ago
|
||
This is very similar to, and has the same regression date as, bug 338222 - so I'm making it a dependency of that bug for now.
Depends on: 338222
Summary: RTL <select>s misaligned on certain long pages ({inc}?) → RTL <select> inside table is misaligned with incremental reflow
Reporter | ||
Comment 31•18 years ago
|
||
This appears to be fixed on the reflow branch.
Whiteboard: [reflow-refactor]
Flags: blocking1.9a1? → blocking1.9-
Whiteboard: [reflow-refactor] → [reflow-refactor] [wanted-1.9]
Reporter | ||
Comment 32•18 years ago
|
||
*** Bug 362424 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 33•18 years ago
|
||
Fixed on trunk by the reflow-refactor landing.
Comment 34•18 years ago
|
||
Adding in-testsuite? nomination per bz's request in m.d.t.l. Sorry for the bugspam.
Flags: in-testsuite?
Updated•17 years ago
|
Flags: wanted1.9+
Whiteboard: [reflow-refactor] [wanted-1.9] → [reflow-refactor]
Comment 35•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: zach → layout.fonts-and-text
Updated•12 years ago
|
Flags: in-testsuite?
Flags: blocking1.9-
You need to log in
before you can comment on or make changes to this bug.
Description
•