967110 - Add an inverted help icon and arrow to show on the menu panel anchor when the Help subview is open

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

We have inverted icons for the icons that show subviews (such as developer tools and bookmarks), but we are missing one for the help view. It currently just shows a blue background without the icon or arrow.

Stephen said he was working on the icon for this and will add it to the menuPanel-help.png sprite. Setting needinfo accordingly.

Comment 1

[Stephen Horlander [:shorlander]]

Attachment: menuPanel-help.png — Windows

Comment 2

[Stephen Horlander [:shorlander]]

Attachment: menuPanel-help.png — OS X

Comment 3

[Stephen Horlander [:shorlander]]

Attachment: [e-mail] — OS X @2x

Comment 6

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

Attachment: WIP Patch

I had to do some hacky things to get the arrow to fit in there since the help button is so small. Also had to tweak the anchor for the help subview.

Still need to do one more pass over it to make sure all the CSS changes are clean, but this should be good for a feedback pass.

Comment 7

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

Attachment: Patch

Fixed HiDPI and RTL support. While testing RTL support I noticed that one of the gradients had gotten flipped in the wrong direction by bug 979499.

Comment 8

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

Comment on attachment 8400402 [details] [diff] [review]
Patch

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

It looks good on almost every platform - except for hidpi OS X. There, we have no icon when we open the subview - you need to use the @2x graphic.

I'm concerned about the approach you're using here to get the subview arrow to appear, via the PanelUI-quit button. I think that's fragile. What alternatives did you explore for this?

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +58,5 @@
>    #PanelUI-quit:not([disabled]):hover:active {
>      -moz-image-region: rect(0, 96px, 32px, 64px);
>    }
>  
> +  #PanelUI-help[panel-multiview-anchor=true] {

Nit: let's use "true" for consistency.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +554,5 @@
>  #PanelUI-quit:not([disabled]):hover:active {
>    -moz-image-region: rect(0, 48px, 16px, 32px);
>  }
>  
> +#PanelUI-help[panel-multiview-anchor=true] {

Same nit as before, "true" please.

@@ +852,5 @@
> +}
> +
> +#PanelUI-help[panel-multiview-anchor="true"] {
> +  background-image: linear-gradient(rgba(255,255,255,0.3), rgba(255,255,255,0));
> +  background-position: 0;

Doesn't this just default to 0? Or are you overriding something here?

@@ +858,5 @@
> +
> +#PanelUI-help[panel-multiview-anchor="true"] + toolbarseparator + #PanelUI-quit {
> +  background-color: Highlight;
> +  background-position: left center;
> +  background-image: url(chrome://browser/skin/customizableui/subView-arrow-back-inverted.png),

Oh my. I guess it wasn't possible to use a pseudoelement on #PanelUI-help for this? Sticking the subview arrow for #PanelUI-help on to #PanelUI-quit is pretty weird - and it will break if add-ons overlay buttons inbetween these two (which I've started to see in the wild).

Comment 9

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

Attachment: Patch v2

Comment 10

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

Attachment: Patch v2.1

Forgot to hit Save on the osx changes.

Comment 11

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

Comment on attachment 8400843 [details] [diff] [review]
Patch v2.1

So, I think this looks fine - however, for large fonts on Windows, I'm noticing a glitch:

http://i.imgur.com/FrLxH41.png

Almost certainly from the static heights you're putting in. How hard do you believe it would be to address that here?

Comment 12

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

Attachment: Patch v2.2

Comment 13

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

Comment on attachment 8401376 [details] [diff] [review]
Patch v2.2

As discussed in IRC, this is way better, but the gradient isn't stretching properly on Retina displays on OS X.

Comment 14

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

Attachment: Patch v2.3

Only hypothetically tested, but this should fix the issue for Retina OSX.

Comment 15

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

Comment on attachment 8401425 [details] [diff] [review]
Patch v2.3

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

Ok, I dig this. Thanks Jared!

Comment 16

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

Comment on attachment 8401425 [details] [diff] [review]
Patch v2.3

This looks great!

And I gave you r+, and then noticed one last thing - the subview arrow on OS X Retina is low-res. We need the @2x asset in that case.

I think the next patch is the one. I can feel it.

Comment 17

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

Attachment: Patch v2.4

Thanks for catching that! Hopefully this is the last round :)

Comment 18

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

Comment on attachment 8401897 [details] [diff] [review]
Patch v2.4

Go go go!

Thanks jaws. :)

Comment 19

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

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

Comment 20

[:Gijs (out for now; he/him)]

http://graphs.mozilla.org/graph.html#tests=[[309,132,31]]&sel=1396803860000,1396976660000&displayrange=7&datatype=running and https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/ygFhulxhgl8 suggest that this regressed CART by about 7.6% on Win8. :-\

I've not checked other OSes.

If I had to guess, it'd be about relative-positioning the footer.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/162bd85500e1

Comment 22

[Tim Nguyen :ntim]

I noticed a problem : Half of the help icon is cut. Should I fill a bug ?

Comment 23

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

(In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment #22)
> I noticed a problem : Half of the help icon is cut. Should I fill a bug ?

Half, as in the left half? That is intentional, as you can see with other icons when entering a subview such as the History subview.

Comment 24

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

Comment on attachment 8401897 [details] [diff] [review]
Patch v2.4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): missing inverted icon for help menu subview
User impact if declined: inconsistent subview appearance 
Testing completed (on m-c, etc.): on m-c for a few days now
Risk to taking this patch (and alternatives if risky): none expected, needs to land along with the patch in bug 993421
String or IDL/UUID changes made by this patch: none

Comment 25

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

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/78867264da1d


Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/5dcc734a5736

Comment 26

[Carol Chung[:lizardwalk5]]

Attachment: Screenshot Toolbar menu with Help expanded

[bugday-20140423]
status-firefox29.0 verified 
status-firefox30.0a2 verified
status-firefox31.0a1 verified

tested on X86 Mac OSX 10.9.2

Steps
1 From right side of Toolbar, click Menu icon.
2 From bottom of popup menu, click Help icon.
3 From bottom left of the nav menu, click the Help icon and back button.
The main menu should display.

Screenshot was attached for Firefox 29.0 but similar screenshots resulted for Aurora and Nightly builds.

Comment 27

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

Thanks for verifying this! I'll update the various flags on the bug for you :)

Comment 28

[Petruta Horea [:phorea], Desktop Test Engineering]

https://bug979499.bugzilla.mozilla.org/attachment.cgi?id=8412565 - arrow in rtl view
https://bug979499.bugzilla.mozilla.org/attachment.cgi?id=8412614 - arrow for "Open Help Menu" in rtl view

Jared, please see the above screenshots found when verifying bug 979499 and let me know if a follow-up bug is needed for the Help arrow in rtl view. Thank you!

Comment 29

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

(In reply to Petruta Rasa [QA] [:petruta] from comment #28)
> https://bug979499.bugzilla.mozilla.org/attachment.cgi?id=8412565 - arrow in
> rtl view
> https://bug979499.bugzilla.mozilla.org/attachment.cgi?id=8412614 - arrow for
> "Open Help Menu" in rtl view
> 
> Jared, please see the above screenshots found when verifying bug 979499 and
> let me know if a follow-up bug is needed for the Help arrow in rtl view.
> Thank you!

Hi Petruta, yeah, a followup bug for RTL is needed. Thanks for catching that.