1074498 - Panic/Forget button: warning text above button should line-wrap

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: screenshot of issue

Firefox Nightly 20140929 on Windows 8.1

The Forget button in the customizable widgets button opens a sidebar where the labels of the items don't wrap. See screenshot of the issue with German work-in-progress translation (issue is here that 'Recent' requires a participle in German).

Comment 1

[:Gijs (he/him)]

When I add arbitrary text to the en-US labels in DOMI, they wrap for me on OS X as well as Windows 8.

Comment 2

[:Gijs (he/him)]

Clarified on IRC, this is because the warning text at the bottom doesn't wrap. Fixing.

Comment 3

[:Gijs (he/him)]

Attachment: some of the panic button panel's labels don't wrap,

Also had to tweak the height fixup code so it worked in the menu panel (pre-patch version probably leaks the event listener...), then found out the hbox above the description doesn't size correctly (adding flex=1 there didn't help). :-(

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8497144 [details] [diff] [review]
some of the panic button panel's labels don't wrap,

Review of attachment 8497144 [details] [diff] [review]:
-----------------------------------------------------------------

r- for the hardcoding of 32, if my assumptions are correct.

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1031,4 @@
>        let win = aContainer.ownerDocument.defaultView;
>        for (let item of variableHeightItems) {
>          if (aSetHeights) {
> +          let h = win.getComputedStyle(item, null).getPropertyValue("height");

s/h/height/ for better readability. Single letter variables other than counter-specific ones such as `i`/`j` should be expanded.

@@ +1036,5 @@
> +          // In the main menu panel, need to set the height of the container of this
> +          // description because otherwise the text will overflow:
> +          if (item.id == "PanelUI-panic-mainDesc" &&
> +              view.getAttribute("current") == "true" &&
> +              parseInt(h) > 32) {

What significance does 32 have here? If 32 is used because it is based on a specific font-size, then we'll need a more robust solution here as it will break when people use larger or smaller font-sizes.

Comment 5

[:Gijs (he/him)]

Attachment: some of the panic button panel's labels don't wrap,

The 32 is in reference to the icon's height. I've added a comment to clarify.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8497376 [details] [diff] [review]
some of the panic button panel's labels don't wrap,

Review of attachment 8497376 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the clarification.

Comment 7

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/4abd3a660b4a

Comment 8

[:Gijs (he/him)]

Attachment: Patch for Aurora (folds in )

Comment 9

[:Gijs (he/him)]

Comment on attachment 8497742 [details] [diff] [review]
Patch for Aurora (folds in )

Sigh @ bzexport

Comment 10

[:Gijs (he/him)]

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4abd3a660b4a