Closed Bug 897496 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

29 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: Gijs, Assigned: jaws)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P2])

Attachments

(4 files, 9 obsolete files)

16.18 KB, image/png
mmaslaney
: ui-review+
Details
22.96 KB, image/png
Details
8.23 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
18.51 KB, image/png
Details
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.
Whiteboard: [Australis:P4] → [Australis:P3]
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Blocks: 960258
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 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 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.
Attachment #8366758 - Flags: review?(gijskruitbosch+bugs)
Attachment #8366758 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
With platform dependent colors for the gradients and the menuPanelColumn code removed.
Attachment #8366853 - Attachment is obsolete: true
Attachment #8367548 - Flags: review?(gijskruitbosch+bugs)
Attached image Screenshot of patch
This changes the shape of the panel buttons to show half of the third line of text. Look OK to you?
Attachment #8367551 - Flags: ui-review?(mmaslaney)
(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?
(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 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.
Attachment #8367548 - Flags: review?(gijskruitbosch+bugs) → review+
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.
I'm going to guess that was Jared futzing with the string manually.
Flags: needinfo?(jaws)
Yes, that's all. Not changing the string, just showing what a longer string would look like if it was faded out.
Flags: needinfo?(jaws)
Ahh, gotcha. Looking good, Jared.
Attachment #8367551 - Flags: ui-review?(mmaslaney) → ui-review+
Landed with some changes to make toolbarbutton[type=menu-button] work correctly.

https://hg.mozilla.org/integration/fx-team/rev/a6ac946ef4fe
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.
Backed out again for the same error: https://hg.mozilla.org/integration/fx-team/rev/f33d37e23b14

I guess tryserver will be your friend?
(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?
Depends on: 966213
Attached patch Patch 2.1 (obsolete) — Splinter Review
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?
Attachment #8368690 - Flags: feedback?(enndeakin)
Attached patch interdiff.patch (obsolete) — Splinter Review
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.
Attachment #8368690 - Attachment is obsolete: true
Attachment #8368690 - Flags: feedback?(enndeakin)
Attachment #8368908 - Flags: review?(gijskruitbosch+bugs)
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 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.
Attachment #8368908 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch Patch v3 (obsolete) — Splinter Review
Gijs, how's this? The incorrect colors on OSX were because I was using RGB from the colorpicker and not "native colors".
Attachment #8367548 - Attachment is obsolete: true
Attachment #8368908 - Attachment is obsolete: true
Attachment #8369663 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8369663 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch Patch v4 (obsolete) — Splinter Review
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.
Attachment #8369663 - Attachment is obsolete: true
Attachment #8369877 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8369877 - Flags: review?(gijskruitbosch+bugs)
Attached patch Patch v5 (obsolete) — Splinter Review
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
Attachment #8369877 - Attachment is obsolete: true
Attachment #8370247 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8370247 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch Simpler patch (obsolete) — Splinter Review
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.
Attachment #8370247 - Attachment is obsolete: true
Attachment #8370787 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8370787 - Flags: review?(gijskruitbosch+bugs)
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.
Whiteboard: [Australis:P3] → [Australis:P2]
Attached patch Simpler patch v2Splinter Review
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.
Attachment #8370787 - Attachment is obsolete: true
Attachment #8370974 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8370974 [details] [diff] [review]
Simpler patch v2

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

Ship it. :-)
Attachment #8370974 - Flags: review?(gijskruitbosch+bugs) → review+
I don't know if it was intended but the buttons in the menu are huge on Fx team.
(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?
Attached image 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.
https://hg.mozilla.org/mozilla-central/rev/ea246bfdd262
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Blocks: 963365
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
Attachment #8370974 - Flags: approval-mozilla-aurora?
Attachment #8370974 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 966606
Depends on: 964244
Depends on: 971246
Depends on: 969458
QA Contact: cornel.ionce
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?
Flags: needinfo?(jaws)
(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
Flags: needinfo?(jaws)
(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.
(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...
(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!
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
Status: RESOLVED → VERIFIED
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"?
(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.
You need to log in before you can comment on or make changes to this bug.