Empty line in Panic/Forget menu under some conditions

VERIFIED FIXED in Firefox 33

Status

()

Firefox
Toolbars and Customization
--
minor
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: aryx, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 36
x86_64
Windows 8.1
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox33 verified, firefox34 verified, firefox35 verified, firefox36 verified)

Details

Attachments

(8 attachments, 4 obsolete attachments)

24.19 KB, image/png
Details
412.54 KB, application/octet-stream
Details
2.46 KB, patch
jaws
: review+
Gijs
: checkin+
Details | Diff | Splinter Review
415.02 KB, application/octet-stream
Details
30.20 KB, image/png
Details
6.68 KB, patch
jaws
: review+
mconley
: feedback+
Gijs
: checkin+
Details | Diff | Splinter Review
8.65 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
64.78 KB, image/png
Details
Created attachment 8497134 [details]
screenshot of issue

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">
(Assignee)

Comment 1

3 years ago
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.
(Assignee)

Comment 3

3 years ago
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?
Flags: needinfo?(archaeopteryx)
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.
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 5

3 years ago
(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.
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 6

3 years ago
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. :-(
(Assignee)

Comment 7

3 years ago
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).
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 9

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

Updated

3 years ago
Flags: qe-verify?
Flags: firefox-backlog+

Updated

3 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 36.1
Points: --- → 3
(Assignee)

Comment 10

3 years ago
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).
(Assignee)

Comment 11

3 years ago
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?
Flags: needinfo?(jaws)
Flags: needinfo?(dolske)
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.
Flags: needinfo?(dolske)
(Assignee)

Comment 13

3 years ago
Created attachment 8507422 [details] [diff] [review]
set node width in addition to height to avoid empty lines due to different wrapping,

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.
Attachment #8507422 - Flags: review?(jaws)
(Assignee)

Updated

3 years ago
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(jaws)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1085129
Attachment #8507422 - Flags: review?(jaws) → review+
(Assignee)

Comment 15

3 years ago
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?
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(archaeopteryx)
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.
Flags: needinfo?(francesco.lodolo)
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.
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 18

3 years ago
(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?
Flags: needinfo?(archaeopteryx)
Created attachment 8508135 [details]
language pack with vertical scrollbar
Flags: needinfo?(archaeopteryx)
Created attachment 8508137 [details]
screenshot vertical scrollbar
https://hg.mozilla.org/mozilla-central/rev/3416fed83bb4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
(Assignee)

Comment 22

3 years ago
Argh. While it won't hurt too much to leave the current thing on trunk, I need to look at this again.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 23

3 years ago
(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
(Assignee)

Comment 25

3 years ago
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.
Attachment #8508975 - Flags: feedback?(jaws)
(Assignee)

Comment 26

3 years ago
(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.
(Assignee)

Comment 27

3 years ago
(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.
(Assignee)

Comment 28

3 years ago
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.
Attachment #8509410 - Flags: review?(jaws)
(Assignee)

Updated

3 years ago
Attachment #8508975 - Attachment is obsolete: true
Attachment #8508975 - Flags: feedback?(jaws)
(Assignee)

Comment 29

3 years ago
Created attachment 8509411 [details] [diff] [review]
use CSS instead of hacks - m-c version,

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?
Attachment #8509411 - Flags: review?(jaws)
Attachment #8509411 - Flags: feedback?(mconley)
(Assignee)

Updated

3 years ago
Attachment #8507422 - Flags: checkin+
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.
Attachment #8509411 - Flags: feedback?(mconley) → feedback+
(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.
Attachment #8509410 - Flags: review?(jaws) → review+
Attachment #8509411 - Flags: review?(jaws) → review+
(Assignee)

Comment 33

3 years ago
Created attachment 8509852 [details] [diff] [review]
use CSS instead of hacks,

This adds an attribute for beta to be able to set overflow: visible in the right place.
Attachment #8509852 - Flags: review?(jaws)
(Assignee)

Updated

3 years ago
Attachment #8509410 - Attachment is obsolete: true
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?
Attachment #8509852 - Flags: review?(jaws) → review+
(Assignee)

Comment 35

3 years ago
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!
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(archaeopteryx)
(Assignee)

Updated

3 years ago
Attachment #8509411 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8509852 - Attachment is obsolete: true
(Assignee)

Comment 36

3 years ago
Created attachment 8509859 [details] [diff] [review]
Patch with attribute and comments for beta/33.1
(Assignee)

Comment 37

3 years ago
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
Attachment #8509859 - Flags: review+
Attachment #8509859 - Flags: approval-mozilla-beta?
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.
Attachment #8507980 - Attachment is obsolete: true
Flags: needinfo?(francesco.lodolo)
Tested only the 'actions' part and that wrapped as expected and no scrollbars shown.
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 40

3 years ago
(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.
(Assignee)

Updated

3 years ago
Blocks: 1088003
https://hg.mozilla.org/mozilla-central/rev/6071fbf83d63
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
QA Contact: bogdan.maris
status-firefox33: --- → unaffected
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → fixed
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)?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(archaeopteryx)
Yes, this looks fixed on Nightly.
Flags: needinfo?(archaeopteryx)
Comment on attachment 8509859 [details] [diff] [review]
Patch with attribute and comments for beta/33.1

Thanks for verifying! Beta+
Attachment #8509859 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/46916559304f
status-firefox34: affected → fixed
(Assignee)

Comment 46

3 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 47

3 years ago
Per discussion, uplifting this to Alder today.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 48

3 years ago
remote:   https://hg.mozilla.org/projects/alder/rev/c55d0a927772
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [fixed-alder]
https://hg.mozilla.org/releases/mozilla-release/rev/c55d0a927772
Whiteboard: [fixed-alder]
(In reply to :Gijs Kruitbosch from comment #46)
> I'm hoping to uplift the nightly patch to aurora given sufficient baking.

Baked enough now? :)
status-firefox33: unaffected → fixed
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 51

3 years ago
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
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8509411 - Flags: approval-mozilla-aurora?
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.
Status: RESOLVED → VERIFIED
status-firefox33: fixed → verified
status-firefox34: fixed → verified
status-firefox36: fixed → verified
Attachment #8509411 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing for Aurora uplift.
Flags: needinfo?(gijskruitbosch+bugs)
Keywords: branch-patch-needed
(Assignee)

Comment 54

3 years ago
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/de1a68af9d28
status-firefox35: affected → fixed
Flags: needinfo?(gijskruitbosch+bugs)
Keywords: branch-patch-needed
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.
status-firefox35: fixed → verified
You need to log in before you can comment on or make changes to this bug.