Firefox Nightly 20140929 on Windows 8.1 See attached screenshot. The panic's button list of actions can contain an empty line. From the localized browser.dtd: <!ENTITY panicButton.view.deleteCookies "Kürzlich angelegte oder geänderte <html:strong>Cookies</html:strong> löschen foo bar baz foo bar baz foo bar baz foo bar baz foo bar baz"> <!ENTITY panicButton.view.deleteHistory "Die kürzlich angelegte <html:strong>Chronik</html:strong> löschen">

I think with bug 1074498 fixed this will be hard to reproduce (by which I mean, I could no longer reproduce it, but I didn't try suuuuper-hard). It'd be useful to know if that's the case. I couldn't reproduce with the exact string from the screenshot, on OS X, but with a different string pre-1074498. I also think that this might not happen much in practice (it's a 1-character-difference thing - ie you have to be pretty unlucky to hit this, or trying really hard) and also that it is something with which I could ultimately live. At least text isn't unreadable. It's some kind of weird XUL layout bug (in the case I saw, the set height is actually ~20px, which is much less than the visible height of the item, so I'm really not sure what's going on, either).

Still reproducible with Nightly 20141001, so not affected by the fix for bug 1074498.

Can you provide all the strings you're using to reproduce? I struggled to get the same thing to happen with the strings from comment #0, then tried using some more from the screenshot, and still didn't manage. Also, on what OS are you testing, and does it happen in the main menupanel and/or if it's a standalone panel?

(In reply to :Gijs Kruitbosch from comment #3) > Can you provide all the strings you're using to reproduce? Language pack containing the strings (near the bottom of browser.dtd) attached. Drag and drop it into the browser, install it, set general.useragent.locale to 'de' (no idea if a restart is necessary). > Also, on what OS are you testing Windows 8.1 64 bit, zoom 125% on OS level > does it happen in the main menupanel and/or if it's a standalone panel? Only happens as standalone panel.

(In reply to Archaeopteryx [:aryx] from comment #4) > Created attachment 8498199 [details] > firefox-35.0a1.de.langpack.xpi > > (In reply to :Gijs Kruitbosch from comment #3) > > Can you provide all the strings you're using to reproduce? > Language pack containing the strings (near the bottom of browser.dtd) > attached. Drag and drop it into the browser, install it, set > general.useragent.locale to 'de' (no idea if a restart is necessary). > > > Also, on what OS are you testing > Windows 8.1 64 bit, zoom 125% on OS level > > > does it happen in the main menupanel and/or if it's a standalone panel? > Only happens as standalone panel. Thanks for the detailed instructions! I can reproduce under these specific circumstances (good thing my desktop machine also runs Win8.1). Do you also hit this for the actual German translation? I hope not, of course, but I'm curious. I wonder if it can happen only if the text for this item ends up being exactly two lines (not that likely to happen for actual translations, I think), or if it'd happen if it's exactly 1 line as well (more likely to happen with real strings). :-) In the meantime, investigating what's going on here.

Right, so, it seems that in this case, the height is set to 54.5px, and the reflows for the rest of the panel then make it so the node gets wider, and so the text fits on 2 lines, and so the third line is empty. If I remove the height declaration, the text wraps onto 3 lines, the panel immediately goes wonky and text overlaps and everything is sad. Mostly me, because man, XUL layout really needs to improve here. :-(

Also setting width in the code that sets height fixes this, but I'm far too scared that this will lead to content clipping to actually ship that as a solution here. :-(

(In reply to :Gijs Kruitbosch from comment #5) > Do you also hit this for the actual German translation? No (at least not on this machine).

QA found this happens to the French locale in some situations.

I wonder if this has to do with the windows dpi settings and we use the wrong size somewhere... I just cannot get this to reproduce on OS X (retina).

After a little more than a day or so of playing with this... here's a summary: Premises: 0) don't want to / can't change any strings at this point 1) this happens when we wrap text at the point where we set the height... and then the panel gets wider and so we stop wrapping. This makes it dependent on DPI+font size+OS+string length. 2) this wouldn't happen if we don't set the height of all these elements So I tried to fix this by disabling the height setting. That then means the panel ends up being too small if any of the strings wrap, leading to the button and/or other things getting cut off at the bottom of the panel. Then I tried using a bundle of layout hacks (mostly, setting display: flex and removing the overflow:hidden settings) in addition to that to fix that situation. I wasn't able to get this to work. At this point, I think we should either live with this or set the width as well. The more I think about it, the more I'm tempted to go with the width setting, do a try push with that and ask localizers + QA to check for issues with their locales (as language packs, installed on the try push). Assuming there's no massive other issues that show up, I'd like to then ship that (ie setting the width as well). Jared and Justin, does that sound like a plan?

Taked with Gijs -- yep. Worst case I'm ok with living with this bug on 33.1. It's a minor visual glitch. Sounds like the proposed fix is safe and understood enough, so let's try doing it and get some quick testing somewhere on Nightly/Aurora/Beta to make sure it (1) fixes the relevant locales and (2) doesn't screw up anything else. :) If all's good, we can try getting it landed in alder next week for pre-33.1 testing and uplift.

Per discussion with dolske, I think we should try this on nightly+aurora+beta, get some intensive QA on it, and if it checks out, use it on 33.1, too.

https://hg.mozilla.org/integration/fx-team/rev/3416fed83bb4 Francesco, Archaeopteryx, can you test the builds generated here: https://treeherder.mozilla.org/ui/#/jobs?repo=fx-team&revision=3416fed83bb4 with the strings that were problematic for you (as a locale pack or otherwise) and confirm this fixes things?

This is how it looks on that build, replacing browser.* string files with Italian ones. Looks good to me.

The environment changed since the bug report (button and label above it defining panel width), so the string from comment 0 doesn't end anymore at at end of a line. But removing the last two 'words' causes the last word to get its own line and the content of the panel to get a scrollbar (the latter causing the former, I guess). This looks a tad ugly.

(In reply to Archaeopteryx [:aryx] from comment #17) > The environment changed since the bug report (button and label above it > defining panel width), so the string from comment 0 doesn't end anymore at > at end of a line. But removing the last two 'words' causes the last word to > get its own line and the content of the panel to get a scrollbar (the latter > causing the former, I guess). This looks a tad ugly. Horizontal or vertical scrollbar? Can you post a screenshot and/or updated version of the langpack so I can test more easily?

Argh. While it won't hurt too much to leave the current thing on trunk, I need to look at this again.

(In reply to Francesco Lodolo [:flod] from comment #16) > Created attachment 8507980 [details] > Linux on new build > > This is how it looks on that build, replacing browser.* string files with > Italian ones. Looks good to me. FWIW, the margin below the red warning text here is too big, AFAICT. I wonder if there's something else going wrong there.

(In reply to :Gijs Kruitbosch from comment #23) > FWIW, the margin below the red warning text here is too big, AFAICT. I > wonder if there's something else going wrong there. You're right, this was taken before the patch https://bug1085129.bugzilla.mozilla.org/attachment.cgi?id=8507528

As per IRC, this needs better selectors so it only affects the panelmultiview when only showing the panic subview (at least for beta/aurora, I think we should actually do this for all single-view-panels on nightly), and I still see a scrollbar on 125% DPI when putting the button in the menu and removing lots of items so the panel has to grow to accommodate the subview. I've not figured out how to fix that, or even checked how general a problem that is - it might be more liveable than the issues we have now, and the code is certainly a lot better.

(In reply to :Gijs Kruitbosch from comment #25) > Created attachment 8508975 [details] [diff] [review] > use CSS instead of hacks, > > As per IRC, this needs better selectors so it only affects the > panelmultiview when only showing the panic subview (at least for > beta/aurora, I think we should actually do this for all single-view-panels > on nightly), and I still see a scrollbar on 125% DPI when putting the button > in the menu and removing lots of items so the panel has to grow to > accommodate the subview. I've not figured out how to fix that, or even > checked how general a problem that is - it might be more liveable than the > issues we have now, and the code is certainly a lot better. I can literally only reproduce this on Windows' 125% DPI - not on 150, or 110, or on retina OS X. Seems like a rounding error in the panel or layout code involved with setting the height. I'll look at this some more to see if I can figure it out. Either way, I'd much prefer shipping a variant of this patch than what we have on alder/beta/aurora/nightly right now.

(In reply to :Gijs Kruitbosch from comment #26) > (In reply to :Gijs Kruitbosch from comment #25) > > Created attachment 8508975 [details] [diff] [review] > > use CSS instead of hacks, > > > > As per IRC, this needs better selectors so it only affects the > > panelmultiview when only showing the panic subview (at least for > > beta/aurora, I think we should actually do this for all single-view-panels > > on nightly), and I still see a scrollbar on 125% DPI when putting the button > > in the menu and removing lots of items so the panel has to grow to > > accommodate the subview. I've not figured out how to fix that, or even > > checked how general a problem that is - it might be more liveable than the > > issues we have now, and the code is certainly a lot better. > > I can literally only reproduce this on Windows' 125% DPI - not on 150, or > 110, or on retina OS X. > > Seems like a rounding error in the panel or layout code involved with > setting the height. Yup. The actual height of the content is 463.2 on my machine, and then we round down to 463, which means there's 0.2px of scrolling going on... using math.ceil rather than math.round there fixes this, but I'm not sure I'd like to ship that directly on 33.1 considering the issues with panel sizing we've had in the past.

I'd like to ship this for 33.1. This will have a scrollbar in the menu panel on some locales and some panel configurations on Windows when using 125% DPI (and possibly on Linux with similar scaling factors). The CSS changes only affect the panic view.

And this on Nightly. This also adjusts the CSS for other subviews, as well as having the rounding fix. Perhaps after significant baking we want to uplift it into early 35 beta when that happens. I don't think I want to risk uplifting it to current 34 beta or 33. Mike, can you look at this rounding thing considering your panelmultiview experience?

Comment on attachment 8509411 [details] [diff] [review] use CSS instead of hacks - m-c version, Review of attachment 8509411 [details] [diff] [review]: ----------------------------------------------------------------- I can't see anything wrong with these changes by inspection. I applied the patch, and tried to find some problems opening subviews both in the menu panel and in the toolbar, and everything looked OK (at least on OS X). Math.ceil looks like a fine idea to me.

(In reply to :Gijs Kruitbosch from comment #28) > Created attachment 8509410 [details] [diff] [review] > use CSS instead of hacks, > > I'd like to ship this for 33.1. This will have a scrollbar in the menu panel > on some locales and some panel configurations on Windows when using 125% DPI > (and possibly on Linux with similar scaling factors). The CSS changes only > affect the panic view. I'm able to get rid of the scrollbar in the menu panel by setting .panel-subviews { overflow: visible; } I couldn't find anything that broke due to this change.

Comment on attachment 8509410 [details] [diff] [review] use CSS instead of hacks, Review of attachment 8509410 [details] [diff] [review]: ----------------------------------------------------------------- r=me, would be great if you found a way to add overflow:visible to .panel-subviews only when this subview is showing.

This adds an attribute for beta to be able to set overflow: visible in the right place.

Comment on attachment 8509852 [details] [diff] [review] use CSS instead of hacks, Review of attachment 8509852 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +1107,5 @@ > > +#customizationui-widget-multiview[mainViewId="PanelUI-panicView"] > .panel-viewcontainer, > +#customizationui-widget-multiview[mainViewId="PanelUI-panicView"] > .panel-viewcontainer > .panel-viewstack, > +#customizationui-widget-multiview[mainViewId="PanelUI-panicView"] > .panel-viewcontainer > .panel-viewstack > .panel-mainview, > +#PanelUI-multiView[currentsubview="PanelUI-panicView"] > .panel-viewcontainer > .panel-viewstack > .panel-subviews, Can you add a comment for this selector saying that currentsubview is temporary and a workaround?

Landing the nightly version: remote: https://hg.mozilla.org/integration/fx-team/rev/6071fbf83d63 Archaeopteryx and flod, sorry to have to ask you this a second time, but can you stress test the results at: https://treeherder.mozilla.org/ui/#/jobs?repo=fx-team&revision=6071fbf83d63 when they come in? Jared and I have been poking these for a good bit, the solution is a lot less hacky than what we had before, and so I'm fairly sure they're OK, but better safe than sorry. Thanks!

Comment on attachment 8509859 [details] [diff] [review] Patch with attribute and comments for beta/33.1 Approval Request Comment [Feature/regressing bug #]: forget button [User impact if declined]: silly layout on some languages/dpi settings [Describe test coverage new/current, TBPL]: no :( [Risks and why]: low - swapping really hacky JS with much less hacky CSS that's much more likely to actually work right; plus lots of manual testing (see earlier comments, bugs, or ask Jared and me...). [String/UUID change made/needed]: nope

On the left it's the actual Italian text, which now doesn't wrap. On the right it's me torturing the UI, which shows some issues on the top part, while the bottom half works great. By looking at the current translations, we don't have strings that long, so it might be worth fixing but not as urgent as the rest.

Tested only the 'actions' part and that wrapped as expected and no scrollbars shown.

(In reply to Archaeopteryx [:aryx] from comment #39) > Tested only the 'actions' part and that wrapped as expected and no > scrollbars shown. (In reply to Francesco Lodolo [:flod] from comment #38) > Created attachment 8510085 [details] > Test on Linux > > On the left it's the actual Italian text, which now doesn't wrap. > > On the right it's me torturing the UI, which shows some issues on the top > part, while the bottom half works great. > > By looking at the current translations, we don't have strings that long, so > it might be worth fixing but not as urgent as the rest. Thanks, also for the quick turnaround! It sounds to me like we can make the issue with the top part of the dialog a followup, and uplift this to 33.1/34 as-is; there then shouldn't be any issues with the languages we normally ship with.

aryx/Gijs - This fix should be in today's Nightly. Can you do a sanity check to ensure that this is fixed before uplift to Beta? Gijs - Should this also be uplifted to Aurora (approval only filed for Beta)?

Yes, this looks fixed on Nightly.

Comment on attachment 8509859 [details] [diff] [review] Patch with attribute and comments for beta/33.1 Thanks for verifying! Beta+

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #42) > aryx/Gijs - This fix should be in today's Nightly. Can you do a sanity check > to ensure that this is fixed before uplift to Beta? > > Gijs - Should this also be uplifted to Aurora (approval only filed for Beta)? I'm hoping to uplift the nightly patch to aurora given sufficient baking.

Per discussion, uplifting this to Alder today.

remote: https://hg.mozilla.org/projects/alder/rev/c55d0a927772

(In reply to :Gijs Kruitbosch from comment #46) > I'm hoping to uplift the nightly patch to aurora given sufficient baking. Baked enough now? :)

Comment on attachment 8509411 [details] [diff] [review] use CSS instead of hacks - m-c version, Approval Request Comment [Feature/regressing bug #]: panic button [User impact if declined]: empty lines in the text used in the panic button [Describe test coverage new/current, TBPL]: no. :-( [Risks and why]: reasonably low, makes a small change in how the main menupanel makes computations, which could trigger scrollbar issues. However, has baked for a while now and nothing's been reported that I'm aware of. [String/UUID change made/needed]: no

We verified using different locales that had this issue before on various platforms Windows Vista, Windows 7 64bit, Windows 8.1, Mac OS X 10.8.5, Ubuntu 14.04 64bit and 12.04 32bit using Firefox 34 beta 4, latest Alder build and latest Nightly and did not see this issue anymore. More information regarding the testing and locales used can be found in this etherpad: https://etherpad.mozilla.org/ForgetButton-bug1074520.

Needs rebasing for Aurora uplift.

remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/de1a68af9d28

We also verified across platforms using latest Aurora/Developer Edition build and did not see this issue anymore. More information can be seen in the same etherpad from comment 52.