1391017 - "Bookmarks Toolbar Items" icon is too small, when in Customization mode

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Itiel]

Attachment: Small Bookmarks Toolbar Items icon

STR:
1. Open Nightly
2. Enter Customization mode, and drag'n'drop the Bookmarks Toolbar Items somewhere on the toolbar

ER:
The "Bookmarks Toolbar Items" icon should appear with a normal size.

AR:
It doesn't. A very small icon is visible.

See attached.

Tell me if pinpointing the regression range is needed.

Comment 1

[:Gijs (he/him)]

(In reply to ItielMaN from comment #0)
> Tell me if pinpointing the regression range is needed.

I think it'd be helpful if you could, yes please.

Comment 2

[Itiel]

(In reply to :Gijs from comment #1)
> (In reply to ItielMaN from comment #0)
> > Tell me if pinpointing the regression range is needed.
> 
> I think it'd be helpful if you could, yes please.

2017-08-16T22:57:04: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=500a01cf896dbb9883cb85f162552e20b8facfc0&full=1
2017-08-16T22:57:05: DEBUG : Found commit message:
Bug 1377184 - Consistently use the custom toolbar button styling in all browser toolbars. r=johannh

MozReview-Commit-ID: DvMorv7HhDu

2017-08-16T22:57:05: INFO : The bisection is done.
2017-08-16T22:57:05: INFO : Stopped

Regression range is 2017-08-10 to 2017-08-11.

Comment 3

[:Gijs (he/him)]

Bryan, is there an SVG asset to replace the ancient one we're still using?

Comment 4

[bbell]

Attachment: use this icon, example

It should use the same icon we use to refer to it in the Library.

Comment 5

[bbell]

Attachment: toolbar-16.svg

This is the asset needed in case you can't find it in the app.

Comment 6

[:Gijs (he/him)]

That icon highlights a toolbar. This is the icon for the movable set of items that by default appear in the bookmarks toolbar. Given that in customize mode, the icon can appear on other toolbars or the palette or the panel if it's moved there - but outside customize mode, that item shows your bookmarks, I think this would be very confusing.

To be clear, what we're talking about is the icon that appears in the bookmarks bar on a clean profile. You can drag that item, and put the bookmarks that appear in the bookmark toolbar by default somewhere else (like on the main toolbar, or the tabstrip, or...).

Comment 7

[bbell]

Attachment: tiny.png

We are talking about this tiny tiny icon right? A thing that we show as a place holder just in Customization mode?

Comment 8

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: icon.png

(In reply to bbell from comment #7)
> Created attachment 8898998 [details]
> tiny.png
> 
> We are talking about this tiny tiny icon right? A thing that we show as a
> place holder just in Customization mode?

Yes. This icon is also used in "Bookmarks" button menu, but it's in normal size there, see attached screenshot. It's also not yet updated to Photon design.

Comment 9

[:Gijs (he/him)]

Re-needinfo'ing given I don't think the attached .svg is what we want. :-)

Comment 10

[Dão Gottwald [:dao]]

I think we should just use the generic star icon here.

Comment 11

[Dão Gottwald [:dao]]

Attachment: Bug 1391017 - Use a .bookmark-item placeholder for the bookmarks toolbar items in the toolbar while customizing.

Review commit: https://reviewboard.mozilla.org/r/171156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171156/

Comment 12

[:Gijs (he/him)]

Comment on attachment 8899831 [details]
Bug 1391017 - Use a .bookmark-item placeholder for the bookmarks toolbar items in the toolbar while customizing.

https://reviewboard.mozilla.org/r/171156/#review176700

Thanks!

If we want a different icon, we can sub one in pretty easily in a followup.

::: browser/themes/shared/toolbarbutton-icons.inc.css:183
(Diff revision 1)
>  
>  #home-button {
>    list-style-image: url("chrome://browser/skin/home.svg");
>  }
>  
> -#bookmarks-menu-button {
> +#bookmarks-menu-button,

It wouldn't be OK to reuse the same icon because it'd be confusing to have them both as customizable toolbar buttons or in the overflow menu, but the bookmarks menu button never actually uses this icon. So while we're here, can you remove the selector suggesting it does, and the `#bookmarks-menu-button[starred]` styling, and normalize the "real" `#bookmarks-menu-button > .toolbarbutton-icon` selector to match the others?

Fair warning: we probably also need to update https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_themes_icons.js and https://dxr.mozilla.org/mozilla-central/source/browser/base/content/theme-vars.inc.css .

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8899831 [details]
Bug 1391017 - Use a .bookmark-item placeholder for the bookmarks toolbar items in the toolbar while customizing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171156/diff/1-2/

Comment 14

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #12)
> Fair warning: we probably also need to update
> https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/
> test/browser/browser_ext_themes_icons.js and
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/theme-
> vars.inc.css .

This wasn't necessary but I cleaned it up anyway.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b45c8f18e246
Use a .bookmark-item placeholder for the bookmarks toolbar items in the toolbar while customizing. r=Gijs

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 17

[Itiel]

Fixed on latest Nightly, but the new icon seems to be darker than other Photon buttons. Is that intended?

Comment 18

[Dão Gottwald [:dao]]

(In reply to ItielMaN from comment #17)
> Fixed on latest Nightly, but the new icon seems to be darker than other
> Photon buttons. Is that intended?

nope

Comment 19

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

^
|
I created follow up about that change in bug #1393507.

About this bug, I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 57.0a1 (2017-08-24), so I'm marking this bug as VERIFIED. Thanks.