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

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Toolbars and Customization
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified, firefox30 verified)

Details

(Whiteboard: [Australis:P4][good first verify][testday-20140509])

Attachments

(4 attachments, 4 obsolete attachments)

Created attachment 762164 [details]
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.
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P4]
Do we have large inverted icons and the arrow graphic for this stuff somewhere?
Flags: needinfo?(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.
Flags: needinfo?(shorlander)
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.
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).
Whiteboard: [Australis:M?][Australis:P4] → [Australis:M?][Australis:P4][good-first-bug][lang=css][mentor=mconley]
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 ?
Flags: needinfo?(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.
Assignee: nobody → dulanja33
Flags: needinfo?(mconley)
Status: NEW → ASSIGNED
We will need the white arrow too. Michael, can you provide the inverted white side-arrow?
Flags: needinfo?(mmaslaney)
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?
Flags: needinfo?(mconley)

Comment 9

4 years ago
(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
ntim has totally nailed the approach.
Flags: needinfo?(mconley)
Created attachment 8366184 [details]
subView-arrow.zip
Flags: needinfo?(mmaslaney)
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.
Assignee: dulanja33 → jaws
Attachment #8369024 - Flags: review?(gijskruitbosch+bugs)
Whiteboard: [Australis:M?][Australis:P4][good-first-bug][lang=css][mentor=mconley] → [Australis:P4]
(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

4 years ago
(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

4 years ago
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/Secure-Glyph@2x.png
>    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/subView-arrow-back-inverted@2x.png  (../shared/customizableui/subView-arrow-back-inverted@2x.png)

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. :-)
Attachment #8369024 - Flags: review?(gijskruitbosch+bugs) → feedback+
(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.
Created attachment 8369512 [details] [diff] [review]
Patch v1.1
Attachment #8369024 - Attachment is obsolete: true
Attachment #8369512 - Flags: review?(gijskruitbosch+bugs)

Comment 18

4 years ago
Created attachment 8369519 [details] [diff] [review]
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?

Updated

4 years ago
Attachment #8369512 - Attachment is obsolete: true
Attachment #8369512 - Flags: review?(gijskruitbosch+bugs)
Created attachment 8369560 [details] [diff] [review]
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.
Attachment #8369519 - Attachment is obsolete: true
Attachment #8369560 - Flags: review?(gijskruitbosch+bugs)
Depends on: 967110

Comment 20

4 years ago
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?
Attachment #8369560 - Flags: review?(gijskruitbosch+bugs) → review+
> +#PanelUI-footer > #PanelUI-footer-inner.panel-multiview-anchor > #PanelUI-help,
is not necessary. I've removed it.
Created attachment 8369600 [details] [diff] [review]
Patch v1.2 (r+'d)
Attachment #8369560 - Attachment is obsolete: true
Attachment #8369600 - Flags: review+
Keywords: checkin-needed

Comment 23

4 years ago
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.
Fixed the selectors and landed on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/dfb245b7efb3
Keywords: checkin-needed
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
Attachment #8369600 - Flags: approval-mozilla-aurora?
Attachment #8369600 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/dfb245b7efb3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Depends on: 968251
Blocks: 968263
Attachment #8369600 - Flags: approval-mozilla-aurora+
No longer blocks: 968263
Depends on: 968263
status-firefox29: --- → fixed
status-firefox30: --- → affected
status-firefox29: fixed → affected
status-firefox30: affected → fixed
Created attachment 8375613 [details] [diff] [review]
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
Attachment #8375613 - Flags: approval-mozilla-aurora?
Attachment #8375613 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/0949e63d93c2
status-firefox29: affected → fixed

Updated

4 years ago
Depends on: 979499

Updated

4 years ago
Whiteboard: [Australis:P4] → [Australis:P4][good first verify]

Updated

4 years ago
Status: RESOLVED → VERIFIED
Whiteboard: [Australis:P4][good first verify] → [Australis:P4][good first verify][testday-20140509]
I can also confirm that this is verified.
status-firefox29: fixed → verified
status-firefox30: fixed → verified
You need to log in before you can comment on or make changes to this bug.