897496 - Fade out and cut off third-to-nth line of toolbarbutton labels in menupanel

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

As a followup from bug 872544 we should fade out the third and following lines of the labels in the menupanel to ensure a single long button label doesn't break the complete flow.

I think this is lower-prio, but feel free to up it if anyone thinks I've underestimated how important this is.

Comment 1

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

Attachment: Patch using gradient to fade out the text

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 8366758 [details] [diff] [review]
Patch using gradient to fade out the text

>+   menuPanelColumn is needed because calc() cannot do nested calculations such as calc(((menuPanelWidth / 3) - 46px) / 2).

I think according to the spec, calc() can be nested as in calc(calc(...) + calc(...)). Also, what you wrote would be equivalent to calc(menuPanelWidth / 6 - 23px). Why do you need to nest stuff?

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8366758 [details] [diff] [review]
Patch using gradient to fade out the text

>+toolbaritem[cui-areatype="menu-panel"][sdkstylewidget="true"]:not(.panel-wide-item):-moz-focusring,
>+#PanelUI-contents toolbarbutton:-moz-focusring,
>+toolbarpaletteitem[place="panel"] > toolbarbutton:-moz-focusring,
>+toolbarpaletteitem[place="palette"] > toolbarbutton:-moz-focusring,
>+toolbarpaletteitem[place="panel"] > toolbaritem > toolbarbutton:-moz-focusring,
>+toolbarpaletteitem[place="palette"] > toolbaritem > toolbarbutton:-moz-focusring {
>+  outline-offset: -1px; /* Push the focusring just outside of the gradient. */
>+}

Given bug 946395, how can these things be focused?

Comment 4

[:Gijs (he/him)]

Comment on attachment 8366758 [details] [diff] [review]
Patch using gradient to fade out the text

I think this is the right idea, but there's a couple of issues I've noticed:

- this needs rebasing (including addressing comment 3, by just removing that rule) which I've done locally, coming up in a second.
- the fade colors are wrong on OS X, which looks butt-ugly. :-)
- the button hover 'blocks' all become non-square (and, AFAICT, slightly more oblongated than they are now (we need to teach our spellchecker that's a word)). You need ui-review for that. I don't think there's another way to do this, but we should still check.

Comment 5

[:Gijs (he/him)]

Attachment: "Fade out and cut off third-to-nth line of toolbarbutton labels in menupanel" [

rebased

Comment 6

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

Attachment: Patch v2

With platform dependent colors for the gradients and the menuPanelColumn code removed.

Comment 7

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

Attachment: Screenshot of patch

This changes the shape of the panel buttons to show half of the third line of text. Look OK to you?

Comment 8

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #7)
> Created attachment 8367551 [details]
> Screenshot of patch
> 
> This changes the shape of the panel buttons to show half of the third line
> of text. Look OK to you?

Unrelated (I hope) - what's up with the zoom button alignment in that screenshot?

Comment 9

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

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Jared Wein [:jaws] from comment #7)
> > Created attachment 8367551 [details]
> > Screenshot of patch
> > 
> > This changes the shape of the panel buttons to show half of the third line
> > of text. Look OK to you?
> 
> Unrelated (I hope) - what's up with the zoom button alignment in that
> screenshot?

That screenshot is from an earlier version of the patch, please disregard the zoom button styling here. Just focus on the box shape for the hovered/active larger panel buttons :)

Comment 10

[:Gijs (he/him)]

Comment on attachment 8367548 [details] [diff] [review]
Patch v2

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

Nice work! :-)

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +155,5 @@
>  .customization-palette .toolbarbutton-1,
>  .panel-customization-placeholder-child {
>    -moz-appearance: none;
>    -moz-box-orient: vertical;
> +  width: calc(@menuPanelWidth@ / 3);

IIRC there are some places where we overrode these min-width/max-width/max-height bits. Have you checked that they don't need updating?

@@ +261,5 @@
>  .customization-palette #bookmarks-toolbar-placeholder > .toolbarbutton-icon,
>  .panel-customization-placeholder-child > .toolbarbutton-icon {
>    min-width: 32px;
>    min-height: 32px;
> +  margin: 4px calc(@menuPanelWidth@ / 6 - 23px);

Have you checked this with a type="menu-button" button (see bug 940307...)? The web developer toolbar or firebug can be useful testcases.

Comment 11

[Michael Maslaney [:mmaslaney] ([e-mail])]

Jared, is there a reason why we chose to move forward with "New Firefox Australis Window", as opposed to "New Window"? I'd prefer the latter.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

I'm going to guess that was Jared futzing with the string manually.

Comment 13

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

Yes, that's all. Not changing the string, just showing what a longer string would look like if it was faded out.

Comment 14

[Michael Maslaney [:mmaslaney] ([e-mail])]

Ahh, gotcha. Looking good, Jared.

Comment 15

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

Landed with some changes to make toolbarbutton[type=menu-button] work correctly.

https://hg.mozilla.org/integration/fx-team/rev/a6ac946ef4fe

Comment 16

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

Backed out in https://hg.mozilla.org/integration/fx-team/rev/96a80f54716e for browser-chrome bustage on at least Windows:
https://tbpl.mozilla.org/php/getParsedLog.php?id=33833439&tree=Fx-Team

Comment 17

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

Relanded with the negative margin added back to the dropdown and removed from the menubutton-button. 

https://hg.mozilla.org/integration/fx-team/rev/3d0775f7bd0f

Ran the tests locally to confirm that this fixed the test breakage.

Comment 18

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

Backed out again for the same error: https://hg.mozilla.org/integration/fx-team/rev/f33d37e23b14

I guess tryserver will be your friend?

Comment 19

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #15)
> Landed with some changes to make toolbarbutton[type=menu-button] work
> correctly.

(In reply to Jared Wein [:jaws] from comment #17)
> Relanded with the negative margin added back to the dropdown and removed
> from the menubutton-button. 

Can you please get that reviewed?

Comment 20

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

Attachment: Patch 2.1

Hey Neil, when I apply this patch and run the test at /browser/components/customizableui/tests/browser_940307_panel_click_closure_handling.js, I can see the menu popup and the context menu open and close very quickly, causing the test to timeout while it waits for the menuitem to be clicked on. I think the click is missing the menuitem and just reaching the panel behind where the menuitem should be shown.

Do you think you could help me out here?

Comment 21

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

Attachment: interdiff.patch

Gijs and I debugged this together and found the issue of the test failing is coming from the presence of the absolutely-generated content. This is an interdiff of the changes I'd like to make.

This includes changes to the test to get it to reopen the menupopup if the first attempt caused it to open/close very quickly. This will only be added if the try run shows that it fixes the orange (https://tbpl.mozilla.org/?tree=Try&rev=89bb43924e80).

If the change to the test doesn't fix the orange, I'll disable the test using a browser.ini change.

Comment 22

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

Comment on attachment 8368908 [details] [diff] [review]
interdiff.patch

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

::: browser/components/customizableui/test/browser_940307_panel_click_closure_handling.js
@@ +106,5 @@
> +  let elt = document.elementFromPoint(x, y, false, false);
> +  info("element from point at " + x + "," + y + " is " + elt + "#" + elt.id + "." + elt.className);
> +
> +  EventUtils.synthesizeMouse(selectAll, rect.width / 2, rect.height / 2, {}, window);
> +  //EventUtils.synthesizeMouseAtCenter(selectAll, {});

You can disregard this block of changes, I forgot to revert it.

Comment 23

[:Gijs (he/him)]

Comment on attachment 8368908 [details] [diff] [review]
interdiff.patch

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

Unfortunately, looking at this again made me realize that (a) the gradient doesn't match the panel on OS X, and (b) icons get warped out of shape in the customization palette. And then there's the below. :-(

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +234,5 @@
>    padding: 0;
>  }
>  
> +.panelUI-grid .toolbarbutton-1 > .toolbarbutton-menubutton-button {
> +  margin-top: 3px; /* Hack needed to get type=menu-button to properly align vertically. */

This hack doesn't work properly on OS X, the text inside type="menu-button" buttons is too low, and the button's y coordinate is 1 too high compared with that of its row siblings.

We should really figure out a proper fix here, probably by at least getting the height of the inner button right, and getting rid of the 1px border on the inner button.

Comment 24

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

Attachment: Patch v3

Gijs, how's this? The incorrect colors on OSX were because I was using RGB from the colorpicker and not "native colors".

Comment 25

[:Gijs (he/him)]

Comment on attachment 8369663 [details] [diff] [review]
Patch v3

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

I'm still seeing squashed icons in customize mode (they need a different width, I think, and your new rule might be overriding that?)

The OS X colors are still not quite right (I can see the gradients, although it's better than it was)

And finally, I noticed that if you hover from one item to another, the gradient for the ::after bit changes immediately, but the background of the item itself doesn't, which looks really bad. :-(

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +156,5 @@
>  .panel-customization-placeholder-child {
>    -moz-appearance: none;
>    -moz-box-orient: vertical;
> +  width: calc(@menuPanelWidth@ / 3);
> +  height: calc(40px + 4em);

I do idly wonder if we can cut this down without looking terrible. 3.6em? (from the general rule that line-height is ~1.2em). OTOH, don't want to hold up this bug too much.

@@ +281,5 @@
>  .customization-palette #bookmarks-toolbar-placeholder > .toolbarbutton-icon,
>  .panel-customization-placeholder-child > .toolbarbutton-icon {
>    min-width: 32px;
>    min-height: 32px;
> +  margin: 4px calc(@menuPanelWidth@ / 6 - 23px);

Nit: please document the magic 23px. 46px divided by 2 - but why 46? 32 for the width of the item, 2 times 4px for the margins we had before, still leaves 6px hanging.

@@ +480,5 @@
> +.subviewbutton,
> +.widget-overflow-list .toolbarbutton-1,
> +#edit-controls@inAnyPanel@ > toolbarbutton,
> +#zoom-controls@inAnyPanel@ > toolbarbutton {
> +  border-width: 1px;

Meh. TBH, I'd prefer to just override this for the inner menu button to reduce the number of rules, but I don't feel strongly.

Comment 26

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

Attachment: Patch v4

The attached patch fixes the issues brought up in the previous review with the exception of two items:

1. There are still tests failing, and run locally the browser_880164_customization_context_menus.js also fails, and it looks like it's the same issue with the context menu opening and closing immediately. The other test showing this is browser_940307_panel_click_closure_handling.js.

2. The ::after gradient is shown immediately when transitioning between psuedo-classes (:hover, :hover:active, and no psuedo-class). The background-color has a 150ms transition, but since we are using background-image to render the ::after gradient, we can't use CSS transitions on it since background-image is not an animatable property. Other than removing the 150ms transition (which I think provides a nice transition for moving between :hover and :hover:active) I'm not sure what we can do here.

I tried switching to 3.6em but there is not enough of the 3rd line of text showing to make it obvious that there is more text. With only 3.6em it looks more like there are some dirty pixels at the bottom of the button, instead of it being the top-half of the letters.

On an extra note, I modified the OSX colors again in this patch to address your feedback. I'm not sure why my system is reporting different colors, but the color value that I used in this latest patch provides the same color through the color picker on the gradient as it does on the neighboring pixels so it should be good-to-go.

Comment 27

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

Try run of the latest patch,
https://tbpl.mozilla.org/?tree=Try&rev=f04978f07314

Comment 28

[:Gijs (he/him)]

Comment on attachment 8369877 [details] [diff] [review]
Patch v4

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

I really want to r+ this, but icons are still squashed in customize mode. Did you forget to qref? :-(

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +198,5 @@
> +  height: 1.5em;
> +  margin-top: calc(-40px - 4em);
> +  margin-bottom: -0.5em;
> +%ifdef XP_MACOSX
> +  background-image: linear-gradient(rgba(243,243,243,0), rgb(243,243,243) 75%);

This is still wrong. It should be rgba(245,245,245,0) (and likewise for the end value)

@@ +295,5 @@
>    min-width: 32px;
>    min-height: 32px;
> +  /* Explanation for calc(A / B - C):
> +     A (@menuPanelWidth@)
> +       Each button is @menuPanelWidth@ wide.

Eh?

Comment 29

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

Attachment: Patch v5

This patch now uses an SVG mask to fade out the text, and as such it will match the same transition that the background-color of the button uses.

I've added an attribute to the panel contents to disable the fade while running tests in the hopes that it will stop the test failures we were seeing that were coming from the presence of the generated content. Try server push: 
https://tbpl.mozilla.org/?tree=Try&rev=656fc839b580

Comment 30

[:Gijs (he/him)]

Comment on attachment 8370247 [details] [diff] [review]
Patch v5

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

OK. r=me but please update those IDs. Also, and this is just me, but I think I'd prefer to have this in a nightly for a few days before requesting uplift.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +197,5 @@
>                label="&customizeMenu.addMoreItems.label;"/>
>    </menupopup>
> +  <svg:svg height="0">
> +    <svg:defs>
> +      <svg:linearGradient gradientUnits="objectBoundingBox"  id="gradient" x2="0" y2="1">

Let's not call this just 'gradient'

@@ +201,5 @@
> +      <svg:linearGradient gradientUnits="objectBoundingBox"  id="gradient" x2="0" y2="1">
> +        <svg:stop stop-offset="0" />
> +        <svg:stop stop-color="white" offset="0.8"/>
> +      </svg:linearGradient>
> +      <svg:mask id="mask" maskUnits="objectBoundingBox" maskContentUnits="objectBoundingBox" x="0" y="0" width="100%" height="100%">

... or this just 'mask'. Unless I misunderstand something, those ids need to be unique as regards the rest of the document, too...

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +196,5 @@
> +  position: absolute;
> +  height: 1.5em;
> +  margin-top: calc(-40px - 4em);
> +  margin-bottom: -0.5em;
> +  mask: url(chrome://browser/content/browser.xul#mask);

This will need to be updated.

@@ +202,5 @@
> +  background-color: rgb(245,245,245);
> +%elif XP_WIN
> +  background-color: white;
> +%else
> +  background-color: rgb(242,241,240);

WTF, linux. Also, I hope you checked these aren't dependent on GTK themes or something like that?

@@ +304,5 @@
> +       The button has 12px of horizontal padding (6 on each side).
> +       The button has 2px of horizontal border (1 on each side).
> +       Total width of button should therefore be 46px.
> +     D (2)
> +       Divide by 2 since each button has two horizontal margins.

... so Z is 23px.

Comment 31

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

Attachment: Simpler patch

This is a simpler patch that no longer has to hardcode the colors of the panel background. I've moved the mask to the multiline-text element and therefore was able to get rid of the use of the generated content. This now passes all tests without having to modify them, and it also means that the drag image will now retain the faded out text.

Comment 32

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

Try push, https://tbpl.mozilla.org/?tree=Try&rev=ee8a238f7f95

Comment 33

[:Gijs (he/him)]

Comment on attachment 8370787 [details] [diff] [review]
Simpler patch

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

This looks OK code-wise, but it has bugs. :-(

Screenshot in a bit.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +202,5 @@
> +        <svg:stop stop-color="white" offset=".5"/>
> +        <svg:stop stop-color="black" offset="1"/>
> +      </svg:linearGradient>
> +      <svg:mask id="verticalFadeMask" maskUnits="objectBoundingBox" maskContentUnits="objectBoundingBox" x="0" y="0" width="100%" height="100%">
> +        <svg:rect width="1" height="1" fill="url(#verticalFadeOut)"/>

Nit: I'd be in favour of a still more specific ID. "menuPanelButtonTextFadeOut[Mask]" or something.

Comment 34

[:Gijs (he/him)]

Attachment: Screen Shot 2014-02-05 at 16.17.34.png

There are two problems here. One, the two-line character encoding widget is faded at the bottom. Two, the many-line private browsing widget (with stupid text that I inserted to test) hasn't got enough fading going on.

I expect that you need to ensure that the multiline-text box has a specific height in order for this to work properly.

Comment 35

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

Attachment: Simpler patch v2

This will work for most cases, but labels that are over 3 lines long will start to lose the fade. If a label is > 4 lines long, there may be no fade at all. It is a tradeoff that Gijs and I agreed is acceptable.

Comment 36

[:Gijs (he/him)]

Comment on attachment 8370974 [details] [diff] [review]
Simpler patch v2

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

Ship it. :-)

Comment 37

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

https://hg.mozilla.org/integration/fx-team/rev/ea246bfdd262

Comment 38

[u428464]

I don't know if it was intended but the buttons in the menu are huge on Fx team.

Comment 39

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

(In reply to Guillaume C. [:ge3k0s] from comment #38)
> I don't know if it was intended but the buttons in the menu are huge on Fx
> team.

They were expected to grow in size vertically to allow room for the third line of text. Can you attach a screenshot?

Comment 40

[u428464]

Attachment: Buttons in menu panel

(In reply to Jared Wein [:jaws] from comment #39)
> (In reply to Guillaume C. [:ge3k0s] from comment #38)
> > I don't know if it was intended but the buttons in the menu are huge on Fx
> > team.
> 
> They were expected to grow in size vertically to allow room for the third
> line of text. Can you attach a screenshot?

Here it is. It must be intended then, but they take a lot of space IMO.

Comment 41

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

Yes, that is intended.

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea246bfdd262

Comment 43

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

Comment on attachment 8370974 [details] [diff] [review]
Simpler patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature
User impact if declined: buttons with long text (esp l10n) will overflow their container and buttons can be different sizes in the panel
Testing completed (on m-c, etc.): landed on m-c for a few days
Risk to taking this patch (and alternatives if risky): none expected
String or IDL/UUID changes made by this patch: none

Comment 44

[:Gijs (he/him)]

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

Comment 45

[Andrei Vaida [:avaida]]

Jared, I looked for an add-on with a viable (long) name in order to verify this issue on the latest Nightly and Aurora, but I was unable to find one in a reasonable amount of time. Do you know of any add-on that would help me verify this, or maybe a workaround?

Comment 46

[:Gijs (he/him)]

(In reply to Andrei Vaida, QA [:AndreiVaida] from comment #45)
> Jared, I looked for an add-on with a viable (long) name in order to verify
> this issue on the latest Nightly and Aurora, but I was unable to find one in
> a reasonable amount of time. Do you know of any add-on that would help me
> verify this, or maybe a workaround?

Enter customize mode, drag the bookmarks toolbar items to the menu panel

Comment 47

[Andrei Vaida [:avaida]]

(In reply to :Gijs Kruitbosch from comment #46)
> Enter customize mode, drag the bookmarks toolbar items to the menu panel

"Bookmark toolbar items" label takes only 2 lines, if I understood correctly, we need a string that would occupy 3 lines in order to verify this. Please correct me if I'm wrong.

Comment 48

[:Gijs (he/him)]

(In reply to Andrei Vaida, QA [:AndreiVaida] from comment #47)
> (In reply to :Gijs Kruitbosch from comment #46)
> > Enter customize mode, drag the bookmarks toolbar items to the menu panel
> 
> "Bookmark toolbar items" label takes only 2 lines, if I understood
> correctly, we need a string that would occupy 3 lines in order to verify
> this. Please correct me if I'm wrong.

Takes 3 lines on my mac and the last time I looked at a Windows machine with default settings...

Comment 49

[Andrei Vaida [:avaida]]

(In reply to :Gijs Kruitbosch from comment #48)
> Takes 3 lines on my mac and the last time I looked at a Windows machine with
> default settings...

Right, it seems that I need a smaller screen to verify this, I just checked on a 17" display and it indeed takes 3 lines, while on my 22" display it takes 2 lines only. I'll update this issue right away, thanks!

Comment 50

[Andrei Vaida [:avaida]]

I was able to confirm the fix for this issue on Windows 7 64-bit [1] Mac OS X 10.9.1 [2] and Ubuntu 13.10 64-bit [3], using:
- the latest Nightly (Build ID: 20140306030201),
- the latest Aurora (Build ID: 20140306004001),
with the "Bookmark toolbar items" label (Comment 46).

[1] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:30.0) Gecko/20100101 Firefox/30.0
[2] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0
[3] Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0

Comment 51

[Francesco Lodolo [:flod]]

Just an info before filing a new bug: I've just realized that the third line fading is gone on my Mac on Nightly. Bug or "feature"?

Comment 52

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #51)
> Just an info before filing a new bug: I've just realized that the third line
> fading is gone on my Mac on Nightly. Bug or "feature"?

feature, see bug 979477.