Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090225 Minefield/3.2a1pre Steps to reproduce: 1. Make the OS's theme XP default (Luna). 2. Open the findbar. 3. Make the browser window narrower than the findbar. Actual result: The content is rendered outside the window, the same symptom as Bug 367622 and Bug 479957. When "Reached end of page,..." is displayed, the width of content gets wider, then highlighted text moves. This makes findbar unusable under certain situation. Expected result: The content should be rendered inside the window. BuildID=20090204032912 SourceStamp=4ada519b5c97 works BuildID=20090205033258 SourceStamp=5de9f1e51c68 falis BuildID=20090206084003 SourceStamp=764d6458f6d4 works BuildID=20090224033722 SourceStamp=69c86f3acc5a works BuildID=20090224183225 SourceStamp=0e41c1979d25 fails Please note that Bug 479957 also occurs if OS's theme is Classic.

I suspect Bug 445087 form the regression range.

When I add :root {text-rendering: optimizeSpeed !important;} to userChrome.css, the problem has gone.

Could you make a screenshot showing the situation where content is rendered outside the window? I tried to reproduce this but I don't think I am understanding it properly.

It looks like the minimum width of some part of the window has changed. I really doubt it's 445087.

(In reply to comment #5) > It looks like the minimum width of some part of the window has changed. Yes, minimum width of Find Toolbar was small enough before yesterday but it can't be smaller than its content now.

The fact that altering the text-rendering hint affects it is interesting, though; suggests that it may have something to do with glyph extents and the resulting frame overflow areas, which would be affected by 445087. I wonder how the Find toolbar calculates its minimum width.....

Confirmed that this is triggered by the patch in 445087; I can reproduce the same effect on Vista. However, my feeling is still that 445087 isn't really at fault, the root problem lies elsewhere (probably in the XUL layout code) and could in principle be triggered in other ways as well. It's likely that a localization where the text in the findbar happens to begin or end with characters that naturally extend beyond their typographic bounds (like an italic lowercase "f" in a typical Latin font) would run into the same problem, independent of the ClearType fix, as it must be the presence of an overflow-area rect that is confusing the layout. Working around it by changing the text-rendering hint (comment #2) seems like a kludge that might not work in all circumstances, depending on the exact text and fonts involved in the UI.

(In reply to comment #8) > Working around it by changing the text-rendering hint (comment #2) seems like a > kludge that might not work in all circumstances, depending on the exact text > and fonts involved in the UI. findbar {overflow: hidden;} also works fine for me, though I'm not sure it works in all circumstances.

That would be a more reliable workaround, as it would fix the issue regardless of the cause of the overflow -- whether it comes from the ClearType-related patch from 445087, or the inherent shape of the glyphs involved.

adding overflow:hidden to the findbar chrome sounds reasonable to me. However, I would like to understand what's causing this. It may be in XUL layout. Jonathan, are you up for that? :-)

The behavior seems very odd, so I certainly think we should try to understand it better. I've been spending some time today trying to track it down, but no real progress so far. I can keep digging, but I'm sure there are people who'd have a much better idea where to look! :)

One way to proceed would be to attach a debugger, break on, say, nsBoxFrame::GetMinSize, open the find bar (triggering the breakpoint), run nsIFrameDebug::DumpFrameTree(this), find the findbar's box in the frame tree, and then break on nsBoxFrame::GetMinSize with a condition on 'this'.

Been there, done that. GetMinSize appears to be fine; the problem is somewhere higher, with how that minimum is (or isn't) used.

That's progress :-) How about the GetMinSize for the outermost XUL frame (nsRootBoxFrame)? If that's less than the window width, the problem lies with nsRootBoxFrame::Reflow or thereabouts. If it's not, there's a problem with GetMinSize calculations outside the findbar.

(In reply to comment #9) > findbar {overflow: hidden;} also works fine for me, though I'm not sure it > works in all circumstances. Well, findbar { overflow: hidden } code was removed to fix bug 412646. So if someone is going to readd that, he/she should look out that it doesn't regress that bug.

OK, I think I'm beginning to understand. During reflow, if a frame finds that it has a child that has an overflow area, it resets the child's rect to include that overflow area (as noted in the comment here): http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#6612 This change of the child's rect (which I don't feel good about, it seems like a hack - I thought the idea was that we tracked the "true" rect as needed for layout purposes, and the "overflow" rect needed for painting, as potentially distinct things) is noticed by nsSprocketLayout: http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsSprocketLayout.cpp#525 And I think the child size recalculations triggered by this are leading to the min-width of the hbox getting overwritten by a new minimum (haven't traced exactly how that happens yet). That probably shouldn't be happening, if the enclosing hbox was styled with an explicit minimum width that's smaller than the natural width of its contents.

This is a version of the testcase that shows the same behavior on OS X, demonstrating that it is not specific to Windows and that the patch in bug 445087 is not the real underlying cause -- it just makes it easier to encounter the issue on Windows, because text frame overflow areas become more widespread.

Yeah, resizing the child to include its overflow area is an evil XUL hack :-(. In which part of the findbar is text overflowing and triggering problems?

(In reply to comment #20) > In which part of the findbar is text overflowing and triggering problems? <html:span> which contains "Match case", which is generated by XBL of checkbox.

(In reply to comment #21) > (In reply to comment #20) > > In which part of the findbar is text overflowing and triggering problems? > > <html:span> which contains "Match case", which is generated by XBL of checkbox. Right; also the possible status messages such as "Phrase not found". This can cause the content area of the window to be resized as the status message appears or changes, and everything jumps around. (With a sufficiently wide window, of course, none of this matters. But as soon as the window is narrow enough that any of the messages extend beyond the edge, it's a major problem.) With ClearType enabled, most text strings in a small font will end up having a one- or two-pixel overflow area, unless they end with a character that has sufficient blank sidebearings within its width to completely contain the antialiasing. I can reproduce the same problems even on other platforms by styling the findbar to use italic text in the labels, so that overflow is more likely to happen even without the ClearType pixel-bleed effect.

(In reply to comment #18) > And I think the child size recalculations triggered by this are leading to the > min-width of the hbox getting overwritten by a new minimum (haven't traced > exactly how that happens yet). That probably shouldn't be happening, if the > enclosing hbox was styled with an explicit minimum width that's smaller than > the natural width of its contents. Yeah, maybe that can be fixed without fixing the way XUL resizes children to fit their overflow (which is hard and risky to fix).

(In reply to comment #20) > Yeah, resizing the child to include its overflow area is an evil XUL hack :-(. Do you know specific cases that show why we do this? As an experiment, I tried removing the block of code at nsFrame.cpp#6612, and reftests still pass fine (haven't run mochitest yet), but I'm sure it must have been done for a reason.

See bug 458231. I don't know of any specific things that would break, but it seems scary. XUL layout test coverage is poor. Maybe we should take a deep breath and get rid of the code that expands children to their overflow areas for 1.9.2 and see what happens. I wonder what the Neils think of that idea... That would still leave us with a problem for 1.9.1. Maybe try what I said in comment #23 as well.

(In reply to comment #25) > See bug 458231. I don't know of any specific things that would break, but it > seems scary. XUL layout test coverage is poor. > > Maybe we should take a deep breath and get rid of the code that expands > children to their overflow areas for 1.9.2 and see what happens. I wonder what > the Neils think of that idea... FYI I have of course been running with my patch in bug 458231 ever since I wrote it, without any noticable side effects (apart from the bugs it fixes!) However there is one other hack where I believe Firefox uses overflow: hidden; because a CSS min-width in toolbar.css isn't being honoured, so if anyone can fix that case for 1.9.1 that would still be enough to make me happy.

That's encouraging. I've just been running some tests with your patch from 458231, and it looks good to me too. Regarding CSS min-width not being honoured, do you recall where this might have been (besides the findbar)?

I seem to recall it being something to do with the URLbar.

Is it sufficient to use text-rendering: optimizeSpeed just on the findbar or parts of it for 1.9.1? I think we just should just go with Neil's patch. The patch issue only affects blocks inside boxes right?

OK, let's take the patch in bug 458231 for 1.9.2. text-rendering:optimizeSpeed won't help if bug 475968 lands for 1.9.1 (which I'm hoping it will).

Adding "overflow: hidden;" to the findbar would work around the issue for 1.9.1, if we can't fix the real problem of XUL layout ignoring the CSS min-width (I haven't gotten to the bottom of that yet; will investigate some more...). I've tried the overflow: hidden fix on Vista and did not see any recurrence of bug 412646 (see comment #17), but that needs to be checked on other platforms as well before we can decide if it's safe to use this.

I'm at an impasse on this; I don't see a good way to fix it in the layout code without major work that would be too risky to consider for 1.9.1. The root of the problem is the "evil hack" in nsFrame, where child frames are resized to include their overflow rect. This causes higher-level elements to re-do their layout in order to fit the resized children (e.g. nsSprocketLayout), and in the process, the min-width that was specified in the XUL stylesheet is forgotten and the frame re-lays itself to its new preferred width instead of accepting the original, narrower width that was imposed from the window size. This can be seen "in action" by tracing nsBoxFrame::Reflow(), at http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsBoxFrame.cpp#755 Here, the frame has just had its bounds set (correctly) according to the available space. In the non-ClearType, non-frame-overflow case, its children (including the text frames) get laid out in this space, and may be clipped if it is too narrow for them to fit (as the stylesheet gives the box a 1px min-width, so it is allowed to be narrow despite its contents). However, if one of the text frames has an overflow rect, which is highly likely with ClearType, the nsFrame "hack" causes it to change its bounds; this is noticed by nsSprocketLayout and causes another layout pass, during which the enclosing box gets enlarged in order to fit its "resized" contents. So checking its mRect immediately after the Layout() call, we find that it has grown. This enlarged box is what then causes the content area to be laid out to a minimum width that is wider than the window, and content is lost and inaccessible on the right-hand side. Although I don't really have good grasp of the whole layout process yet, I think the issue arises because of the difference between bottom-up HTML layout and top-down XUL, and the fact that we share some code between the two models. With HTML, if a child such as a text frame adjusts its size during layout, it is correct for the enclosing elements to notice this and adjust themselves accordingly; we do layout "from the inside out". But with XUL, we don't necessarily want to do that; we have a known size imposed from the outside, and need to work inwards fitting frames into this predetermined size. The patch in bug 458231 will help because it means that overflow rects (due to antialiasing, or to glyphs like 'f' at the end of a text frame) will no longer result in the child "resizing" during layout. Strictly speaking that may not be a complete solution, if there are other scenarios where a frame could change its size during its layout process, but at the moment I haven't figured out a scenario where that happens. Alternatively, disabling the call to ChildResized() within nsSprocketLayout::Layout() (line 539), so that the sprocketLayout doesn't propagate the size-change up the hierarchy, will fix the findbar problem. Or simply removing the following code that updates the originalClientRect in response. But this kind of hack causes a mochitest failure on tooltips, where a tooltip fails to grow in order to accommodate longer content. (Actually, this is a bit puzzling, as the tooltip grows horizontally but not vertically. This looks like it could be related to the odd label sizing behavior noted in bug 458231 comment 2.) Anyhow, for 1.9.1, if 458231 is too risky to accept, I think our best option is to add the "overflow: hidden" property to the findbar CSS. I have tried this on both Vista and XP, and have not seen any sign of 412646 reappearing. (I believe the real cause of that was some other font-metrics bug, and has since been fixed, so it is no longer sensitive to the overflow property.) We'll also need to add "overflow: hidden" to the toolbar, as (at least on XP) the same problem can be triggered by that, depending on the presence of text in the url box and/or the search box.

By adding "overflow: hidden" to the findbar and toolbar CSS, this patch works around the problem where the widths of these elements get munged and part of the browser content becomes inaccessible. The underlying problem in XUL layout, as illustrated by attachment 364552 [details], remains unresolved for now; bug 458231 should address this. Until that lands, it's possible that we will encounter other UI elements that exhibit similar problem behavior. I have not seen any visual problems on either XP or Vista as a result of the CSS patch; I don't know how to write tests that would actually verify this, as if any glitches are going to come up they'll probably be related to particular font choices, theming, etc. If we can get 458231 safely landed soon, it might be worth considering it for 1.9.1 as well (if we could get it into b4), as I think it's a much better fix than these CSS tweaks.

(In reply to comment #32) > Strictly speaking that may not be > a complete solution, if there are other scenarios where a frame could change > its size during its layout process, but at the moment I haven't figured out a > scenario where that happens. Bug 428939 is similar problem. #urlbar {min-width: 7em;} is specified in the default theme. So #urlbar is going to be 7em width when window width is narrow. But if the total of urlbar's children is wider than 7em (e.g. feed icon is shown), overflow happens and toolbar's min-width:1px is forgotten.

Comment on attachment 366094 [details] [diff] [review] CSS patch to work around toolbar resizing issue >+/* bug 480255, shouldn't be needed after 458231 lands */ >+toolbar { >+ overflow: hidden; >+} This can work around also Bug 428939. So it may be need even if Bug 458231 lands.

I think I'd prefer to wait for 458231 to land and stick and avoid patching the CSS here. Regarding comment #35, if that is needed for bug 428939 it could be attached there.

(In reply to comment #36) > I think I'd prefer to wait for 458231 to land It's going to be a long wait if nobody can work out why the patch makes the top of the box align to the baseline of the containing block instead of the bottom (not that that's correct behaviour either, but at least it passes reftests.)

Jonathan and I basically decided to not take bug 445087 on 1.9.1 branch, so this bug does not need to block 1.9.1.

Fixed by bug 458231, and verified fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090618 Minefield/3.6a1pre

Comment on attachment 366094 [details] [diff] [review] CSS patch to work around toolbar resizing issue not needed