882807 - Invert the icons for the subview-originating button as well as add the arrow icon

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Attachment: Screenshot of mockup

When a subview is opened, the button that originated it gets a blue background. The icon for that button needs to be inverted (made white), and we need to add an arrow on the button pointing towards the subview.

See attached screenshot of mockup.

Comment 1

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

Do we have large inverted icons and the arrow graphic for this stuff somewhere?

Comment 2

[Stephen Horlander [:shorlander]]

I think I included inverted icons for all of the sub panel items in the menuPanel sprite. Will have to check on the arrow.

Comment 3

[u428464]

According to this mockup : http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html the main menu panel should also be greyed out when in subview.

Comment 4

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

Just talked to phlsa about this - while it's true that we don't have the little white arrow, we *do* have the inverted icons for the widgets that open subviews.

So, we just need to update the CSS to change the coordinates for the inverted icons. Piece of cake (I think).

Comment 5

[Dulanja Wijethunga [:dwij]]

Hi All ,
And Hi Mike its good to see you again. :) . Can I work on this ? 
why it shows half of the icon when subview is opened ?

Comment 6

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

(In reply to Dulanja Wijethunga [:dwij] from comment #5)
> Hi All ,
> And Hi Mike its good to see you again. :) . Can I work on this ? 
> why it shows half of the icon when subview is opened ?

Hey dwij! Sure, I can assign this to you.

We're showing half of the button as a way of indicating that this was the button that opened the subview - the mock-up can be viewed here: http://people.mozilla.org/~shorlander/panel-experiment-03/panel-experiment.html (click on "History").

Let me know if you have any questions on how to proceed.

Comment 7

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

We will need the white arrow too. Michael, can you provide the inverted white side-arrow?

Comment 8

[Dulanja Wijethunga [:dwij]]

Thanks Mike! Can you give some guidance on what to do?  
well I couldn't find exact place in the code base that affect the subview. 
Is there any clever method to find relevant code from the code base when fixing this kind of bugs?

Comment 9

[Tim Nguyen :ntim]

(In reply to Dulanja Wijethunga [:dwij] from comment #8)
> Thanks Mike! Can you give some guidance on what to do?  
> well I couldn't find exact place in the code base that affect the subview. 
> Is there any clever method to find relevant code from the code base when
> fixing this kind of bugs?
First, if you don't know how to code firefox yet, check http://codefirefox.com

To add the arrow, edit this file : http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#399

Use the CSS selector : toolbarbutton.panel-multiview-anchor

And to make the icons white :
#toolbarbutton-id.panel-multiview-anchor {
-moz-image-region: rect(32px, 160px, 64px, 128px);
}
In this file : http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/menupanel.inc.css

Comment 10

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

ntim has totally nailed the approach.

Comment 11

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

Attachment: subView-arrow.zip

Comment 12

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

Attachment: Patch

Hey Dulanja, I needed to fix this bug to start working on bug 941436. I'll try to find another bug for you to help out on.

Comment 13

[Dulanja Wijethunga [:dwij]]

(In reply to Jared Wein [:jaws] from comment #12)
> Created attachment 8369024 [details] [diff] [review]
> Patch
> 
> Hey Dulanja, I needed to fix this bug to start working on bug 941436. I'll
> try to find another bug for you to help out on.
It's kk :jaws  :)  ... These days I'm little bit busy. because new semester started.

Comment 14

[Tim Nguyen :ntim]

(In reply to Jared Wein [:jaws] from comment #12)
> Created attachment 8369024 [details] [diff] [review]
> Patch
> 
> Hey Dulanja, I needed to fix this bug to start working on bug 941436. I'll
> try to find another bug for you to help out on.

You should add HDPI support for image regions.

Comment 15

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

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

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

Generally this looks good, but you forgot the OS X hidpi icon bits.

::: browser/themes/linux/jar.mn
@@ +65,5 @@
>    skin/classic/browser/Secure.png
>    skin/classic/browser/Security-broken.png
>    skin/classic/browser/setDesktopBackground.css
>    skin/classic/browser/slowStartup-16.png
> +  skin/classic/browser/customizableui/subView-arrow-back-inverted.png  (../shared/customizableui/subView-arrow-back-inverted.png)

Please put this next to the other customizableui files and maintain alphabetical ordering

::: browser/themes/osx/jar.mn
@@ +103,5 @@
>    skin/classic/browser/Secure-Glyph.png
>    skin/classic/browser/[e-mail]
>    skin/classic/browser/slowStartup-16.png
> +  skin/classic/browser/customizableui/subView-arrow-back-inverted.png  (../shared/customizableui/subView-arrow-back-inverted.png)
> +  skin/classic/browser/customizableui/[e-mail]  (../shared/customizableui/[e-mail])

Please group these with the other customizableui files, and maintain the alphabetical ordering

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +534,5 @@
> +toolbarbutton.panel-multiview-anchor {
> +  background-image: url(chrome://browser/skin/customizableui/subView-arrow-back-inverted.png),
> +                    linear-gradient(rgba(255,255,255,0.3), rgba(255,255,255,0));
> +  background-position: right 5px center;
> +  background-repeat: no-repeat, repeat-x;

Curious, why repeat-x?

::: browser/themes/windows/jar.mn
@@ +77,5 @@
>          skin/classic/browser/searchbar-dropdown-arrow.png
>          skin/classic/browser/Secure24.png
>          skin/classic/browser/setDesktopBackground.css
>          skin/classic/browser/slowStartup-16.png
> +        skin/classic/browser/customizableui/subView-arrow-back-inverted.png  (../shared/customizableui/subView-arrow-back-inverted.png)

And this one too. :-)

@@ +385,5 @@
>          skin/classic/aero/browser/searchbar-dropdown-arrow.png       (searchbar-dropdown-arrow-aero.png)
>          skin/classic/aero/browser/Secure24.png                       (Secure24-aero.png)
>          skin/classic/aero/browser/setDesktopBackground.css
>          skin/classic/aero/browser/slowStartup-16.png
> +        skin/classic/aero/browser/customizableui/subView-arrow-back-inverted.png  (../shared/customizableui/subView-arrow-back-inverted.png)

And this one. :-)

Comment 16

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

(In reply to :Gijs Kruitbosch from comment #15)
> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ +534,5 @@
> > +toolbarbutton.panel-multiview-anchor {
> > +  background-image: url(chrome://browser/skin/customizableui/subView-arrow-back-inverted.png),
> > +                    linear-gradient(rgba(255,255,255,0.3), rgba(255,255,255,0));
> > +  background-position: right 5px center;
> > +  background-repeat: no-repeat, repeat-x;
> 
> Curious, why repeat-x?

This was unnecessary in the shared CSS and was wrongly carried forward to this file as well. My new patch removes it.

Comment 17

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

Attachment: Patch v1.1

Comment 18

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

Attachment: Invert the icons for the subview-originating button as well as add the arrow icon.

Unbitrotted. This seems fine, except that the footer doesn't get the same kind of background when you open the help menu. I also can't see the help button. Was that intentional?

Comment 19

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

Attachment: Patch v1.2

This adds back the blue that got removed as a regression from bug 957460. I'll file a follow-up bug to add in an inverted help icon and the arrow, once we have the inverted help icon asset.

Comment 20

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

Comment on attachment 8369560 [details] [diff] [review]
Patch v1.2

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

r=me with either answer to the question below

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +523,5 @@
>    height: 16px;
>  }
>  
> +#PanelUI-footer > #PanelUI-footer-inner.panel-multiview-anchor,
> +#PanelUI-footer > #PanelUI-footer-inner.panel-multiview-anchor > #PanelUI-help,

Is the extra child selector necessary?

Comment 21

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

> +#PanelUI-footer > #PanelUI-footer-inner.panel-multiview-anchor > #PanelUI-help,
is not necessary. I've removed it.

Comment 22

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

Attachment: Patch v1.2 (r+'d)

Comment 23

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

Comment on attachment 8369600 [details] [diff] [review]
Patch v1.2 (r+'d)

>+++ b/browser/themes/shared/customizableui/panelUIOverlay.inc.css
>@@ -518,22 +518,33 @@ panelview toolbarseparator,
>+#PanelUI-footer.panel-multiview-anchor,
>+#PanelUI-footer.panel-multiview-anchor > #PanelUI-help,

These selectors also need updating.

Comment 24

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

Fixed the selectors and landed on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/dfb245b7efb3

Comment 25

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

Comment on attachment 8369600 [details] [diff] [review]
Patch v1.2 (r+'d)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): no bug, new feature
User impact if declined: subview anchoring won't be as noticeable
Testing completed (on m-c, etc.): locally, landed on fx-team
Risk to taking this patch (and alternatives if risky): none expected
String or IDL/UUID changes made by this patch: none

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

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

Attachment: Patch for Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature, need uplift
User impact if declined: inconsistent styling for subview buttons
Testing completed (on m-c, etc.): on m-c for a while now
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

note: this needs to be uplifted with the patch for bug 968251

Comment 28

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/0949e63d93c2

Comment 29

[Bogdan Maris, Desktop Test Engineering]

I can also confirm that this is verified.