877335 - Normalize styling for toolbarbutton icons

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[:Gijs (he/him)]

See also: bug 875163 comment 2.

Essentially, we now have two sprites for toolbarbuttons, and disparate styling locations for the items using these sprites. The rules aren't exactly the same, which currently leads to some odd bugs when buttons aren't in their expected locations (eg. when Toolbar.png buttons are in the palette).

I believe we should:
- add another constant for all the buttons whose icons are in the new sprite (next to the ones for Toolbar.png here: http://hg.mozilla.org/projects/ux/log/a7d470940b94/browser/themes/shared/browser.inc)
- fix styling using the constant that's already there to also use the new constant (except the actual sprite assignment, obviously)
- add styling for both constants for the panelUI and customization palette where necessary.

It probably makes sense for this styling to all be in one location, either in browser.css or in the customization css, but not split across these two locations.

Comment 1

[:Gijs (he/him)]

I looked at this again today and noticed there is overlap. :-(

That is, we have panel-UI specific icons for some buttons which are in the toolbar (and, when there, use Toolbar.png), and we do not yet have icons for some of the new buttons that can be used as toolbar buttons, and this seems to be part of why things don't always look right yet (at least on OS X).

I'm still not sure what our desired end state is here. Will all the panelUI-sized versions of the icons go in the current sprite, and all the toolbar-sized ones in the toolbar sprite, or something else? Am I missing the point completely?

Comment 2

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #1)
> I looked at this again today and noticed there is overlap. :-(
> 
> That is, we have panel-UI specific icons for some buttons which are in the
> toolbar (and, when there, use Toolbar.png), and we do not yet have icons for
> some of the new buttons that can be used as toolbar buttons, and this seems
> to be part of why things don't always look right yet (at least on OS X).
> 
> I'm still not sure what our desired end state is here. Will all the
> panelUI-sized versions of the icons go in the current sprite, and all the
> toolbar-sized ones in the toolbar sprite, or something else? Am I missing
> the point completely?

We just need to have the same icons in Toolbar.png and menuPanel-icons.png OR we need a mega sprite that has all the icons. I personally like keeping them separate because it makes it easier to maintain and there should ideally be less shifting of coordinates if we need to add or remove something.

A few things to mention: 

- The styles for Toolbar icons and Panel icons are different
- We have two potential sizes of icons in the panel: large regular items (e.g. History) or small split button items (e.g. Cut/Copy/Paste)

And finally do we want to include other toolbar items like dropmarkers and overflow arrows? Currently they are separate.

Comment 3

[:Gijs (he/him)]

Attachment: Patch reorganizing some of this

This fixes bug 878126 and bug 878128, and all the other icons that don't show up in retina mode. There are many articles online on how to test retina with a non-retina display, but not for the inverse, so I haven't gotten round to checking that. Will try to get a dmg and test it on my old mac in a second, after putting up some before/after screenshots.

I've also added a height to the PanelUI-contents in case it's empty (which it was for my testing), because otherwise you don't have much of a droptarget. I then noticed that dragging to the area seemed to be broken. That's obviously a separate bug which I'll file separately as well.

Comment 4

[:Gijs (he/him)]

Attachment: New retina screenshot

Comment 5

[:Gijs (he/him)]

Attachment: Old retina screenshot

Comment 6

[:Gijs (he/him)]

This works on non-retina as well. Filed bug 878452 about the dnd issue with the panel.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 756985 [details] [diff] [review]
Patch reorganizing some of this

>--- a/browser/themes/shared/browser.inc
>+++ b/browser/themes/shared/browser.inc
>@@ -1,3 +1,6 @@
> %filter substitution
> 
> %define primaryToolbarButtons #back-button, #forward-button, #reload-button, #stop-button, #home-button, #print-button, #downloads-button, #downloads-indicator, #bookmarks-menu-button, #new-tab-button, #new-window-button, #fullscreen-button, #zoom-out-button, #zoom-in-button, #sync-button, #feed-button, #alltabs-button, #tabview-button, #webrtc-status-button, #social-share-button
>+
>+%define secondaryToolbarButtons #open-file-button, #find-button, #developer-button, #preferences-button, #privatebrowsing-button, #save-page-button, #add-ons-button, #history-panelmenu

What does "secondary" mean here? Why is it useful to have these defined separately if most of those buttons can be located anywhere?

Comment 8

[:Gijs (he/him)]

Attachment: Patch (unbitrotted)

Comment 9

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #7)
> >+%define secondaryToolbarButtons #open-file-button, #find-button, #developer-button, #preferences-button, #privatebrowsing-button, #save-page-button, #add-ons-button, #history-panelmenu
> 
> What does "secondary" mean here? Why is it useful to have these defined
> separately if most of those buttons can be located anywhere?

Because they take icons from the menupanel sprite until we have correctly sized versions of them included in Toolbar(@x2).png. See also the last hunk in panelUIOverlay.inc.css

Comment 10

[Dão Gottwald [:dao]]

So secondaryToolbarButtons is temporary and will not hit mozilla-central?

Comment 11

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #10)
> So secondaryToolbarButtons is temporary and will not hit mozilla-central?

I would assume so, yes. I'd expect to merge the two constants once we have sprite images in Toolbar.png for the toolbar versions of the new buttons. Sorry, I meant to say this in my previous comment. :-)

Comment 12

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

Comment on attachment 757354 [details] [diff] [review]
Patch (unbitrotted)

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

Glad we're cleaning some of this up! :D

I couldn't test this patch, as it seems to be missing the change to browser.inc that adds secondaryToolbarButtons.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +86,5 @@
>    padding: .5em 1em;
>  }
>  
> +#PanelUI-mainView[customizing] > #PanelUI-contents:empty {
> +  height: 50px;

This should probably be the same height as when a single row of buttons are in there, otherwise, with bug 877370, it might jump in height when those placeholder items are added or removed.

Comment 13

[:Gijs (he/him)]

Attachment: Patch v1.2

I've taken out the height: hunk (and the required bit of CustomizeMode.jsm for that) because I think we should fix the placeholders to behave better (both always be there in customization mode, and have correct size/margins (which they do not now, at least on retina OS X), and we can do that in a different bug.

Comment 14

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

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

A few things here - you've gotta include browser.inc for linux and windows' panelUIOverlay.css as well.

Also, on Windows, I'm seeing: http://i.imgur.com/gPg48j3.png

Were there alterations to browser/themes/windows/customizableui/panelUIOverlay.css that are missing? Linux too?

Comment 15

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

I think we want this for M7.

Comment 16

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

It's likely that this is going to be taken care of in bug 875488.

Comment 17

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

Removing the items from M7 that do not block landing on m-c.

Comment 18

[Justin Dolske [:Dolske]]

Sort of an implementation detail (P5), but sounds like some items have missing/broken icons without this?

Comment 19

[:Gijs (he/him)]

A lot of what I wanted to do here has been done by bug 875488 ( https://hg.mozilla.org/projects/ux/rev/1010d94e216b ) so I'm going to mark this fixed.