Closed Bug 331052 Opened 15 years ago Closed 14 years ago

RTL <select> inside table is misaligned with incremental reflow

Categories

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

defect
Not set
normal

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.
Attached image correct layout
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.
Attached image regression #1
This is how this looked between 2006-01-13 and 2006-02-21. Notice that the text left-aligned instead of right aligned.
Attached image regression #2
Between  2006-02-22 and 2006-03-15.
The text is pushed to the right, and the rightmost part of it is cut off.
Attached image regression #3
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.
Attached image 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. 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.
> 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.
Attached file testcase
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.
Keywords: testcase
>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.
s/left/right/
(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).
Attached file reflow log
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.
Reproduced on Linux: All/All
OS: MacOS X → All
Hardware: Macintosh → All
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.
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.
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.
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.
(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?
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?
(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.

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?
(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.

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....
Could be related to bug 331049, even though they regressed at different times.
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.
... 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.
...which is really just because that select has a width.
Flags: blocking1.9a1?
Blocks: 137995
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.
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
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]
*** Bug 362424 has been marked as a duplicate of this bug. ***
Fixed on trunk by the reflow-refactor landing.
Status: NEW → RESOLVED
Closed: 14 years ago
Depends on: reflow-refactor
Resolution: --- → FIXED
Adding in-testsuite? nomination per bz's request in m.d.t.l. Sorry for the bugspam.
Flags: in-testsuite?
Flags: wanted1.9+
Whiteboard: [reflow-refactor] [wanted-1.9] → [reflow-refactor]
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
Flags: in-testsuite?
Flags: blocking1.9-
You need to log in before you can comment on or make changes to this bug.