1401735 - Extension button notification/badge in Bookmarks Toolbar is truncated after landing patch from bug #1389966

Status: VERIFIED FIXED

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

Description

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

[Tracking Requested - why for this release]: Visible extension compatibility regression

STR:
1. Install uBlock Orgin or Decentraleyes
2. Move extension button to Bookmark Toolbar (if it's not already there)
3. Go to some website page with ads or with CDN resources
and see that all extension button notification in Bookmarks Toolbar is truncated.

Comment 1

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

Attachment: good - Compact Density.png

Comment 2

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

Attachment: good - Normal Density.png

Comment 3

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

Attachment: good - Touch Density.png

Comment 4

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

Attachment: bad - Compact Density.png

Comment 5

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

Attachment: bad - Normal Density.png

Comment 6

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

Attachment: bad - Touch Density.png

Comment 7

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

Maybe simple fix for this issue could be retuning Bars and Toolbars height,
like making Tab Bar and Address Bar shorter,
while making Bookmarks Toolbar taller.

So size of buttons in Bookmarks Toolbar will be more consistent with size of buttons in Tab Bar and in Address Bar.

I don't like compare Firefox to Chromium skins like Chrome, Opera and Vivali,
but look there how it looks like, even in Normal Density mode.

P.S. When specs are non-living and unpractical, they should be (I would even say have to be) changed.

Comment 8

[:Gijs (he/him)]

I don't know why you needinfo'd me. You didn't ask any questions. I'm not on the visual team.

FWIW, I personally don't even think this regression is bad enough to track 57. It only happens with add-ons with badges, and only if the user moves those add-ons into the bookmarks toolbar. We still have worse bugs to fix. It's also not clear if you're testing with or without the changes from bug 1401523.

I'm sure the visual team will look at this bug in triage and carefully consider what to do with it.

Comment 9

[Johann Hofmann [:johannh]]

(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #7)
> Maybe simple fix for this issue could be retuning Bars and Toolbars height,
> like making Tab Bar and Address Bar shorter,
> while making Bookmarks Toolbar taller.
> 
> So size of buttons in Bookmarks Toolbar will be more consistent with size of
> buttons in Tab Bar and in Address Bar.
> 
> I don't like compare Firefox to Chromium skins like Chrome, Opera and Vivali,
> but look there how it looks like, even in Normal Density mode.
> 
> P.S. When specs are non-living and unpractical, they should be (I would even
> say have to be) changed.

It's your own opinion that this spec is unpractical, there are other people who like the bookmark toolbar sizing. Someone has to make a decision. This decision has been made and if you really want to change the height, please open a bug that covers this specifically instead of piggy-backing on regressions.

Thank you for finding this regression. I'd suggest we just push the badge down a little more to solve this. As Gijs mentioned, this is really an edge case.

Comment 10

[Johann Hofmann [:johannh]]

Attachment: Bug 1401735 - Move the toolbarbutton badge slightly down in the bookmarks toolbar.

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

Comment 11

[:Gijs (he/him)]

Comment on attachment 8910679 [details]
Bug 1401735 - Move the toolbarbutton badge slightly down in the bookmarks toolbar.

https://reviewboard.mozilla.org/r/182138/#review187518

Thanks!

I considered whether we could basically use a variable for the vertical positioning, but it seems the styling lives in toolkit so that feels a bit weird. The descendant selectors are a bit sad... it would be nice, I think, if we set the density on toolbars as well as root, and then it would be feasible to have child selectors off the toolbar (for more things than just this!). Anyway, this will do in the short-term.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8910679 [details]
Bug 1401735 - Move the toolbarbutton badge slightly down in the bookmarks toolbar.

https://reviewboard.mozilla.org/r/182138/#review187532

::: browser/themes/shared/toolbarbuttons.inc.css:313
(Diff revision 1)
>    margin-inline-end: 4px;
>  }
>  
> +/* The bookmarks toolbar is smaller than the other toolbars, so we
> + * need to override the badge position to not be cut off. */
> +:root:not([uidensity=touch]) #PersonalToolbar .toolbarbutton-badge {

You can remove :root:not([uidensity=touch]) from this selector, the next rule will override it for [uidensity=touch].

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 8910679 [details]
Bug 1401735 - Move the toolbarbutton badge slightly down in the bookmarks toolbar.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e1c7dcc09d7
Move the toolbarbutton badge slightly down in the bookmarks toolbar. r=Gijs

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/4e1c7dcc09d7

Comment 16

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

I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 57.0a1 (2017-09-22), so I'm marking this bug as VERIFIED.
Thank you very much! \o/

@  Johann Hofmann [:johannh] - Requesting the uplift request. Thanks in advance.

Comment 17

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

(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #16)
> [...]starting in Mozilla Firefox Nightly 58.0a1 (2017-09-22)[...]
^
|
fixed

Comment 18

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

Comment on attachment 8910679 [details]
Bug 1401735 - Move the toolbarbutton badge slightly down in the bookmarks toolbar.

Maybe I will try to do that



Approval Request Comment
[Feature/Bug causing the regression]: Bug #1389966
[User impact if declined]: Extension button notification/badge in Bookmarks Toolbar is truncated (see Attachments to see how it looks like)
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes, STR are in bug 1401735 comment #0
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's trivial fix, which tuned position of the "toolbarbutton-badge"
[String changes made/needed]: Position tuning for the ".toolbarbutton-badge"

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8910679 [details]
Bug 1401735 - Move the toolbarbutton badge slightly down in the bookmarks toolbar.

Please let an engineer make the approval request, as they're better equipped to provide the necessary information and will be responsible if something goes wrong.

Comment 20

[Johann Hofmann [:johannh]]

Comment on attachment 8910679 [details]
Bug 1401735 - Move the toolbarbutton badge slightly down in the bookmarks toolbar.

Dao beat me to it. While I appreciate your enthusiasm, needinfo'ing the patch author is the right action if you want to see something uplifted. You can usually trust that an engineer will request uplift for regression fixes within a few days of landing without needinfo, though.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1389966
[User impact if declined]: Extension button badges are slightly cut off when the button is moved to the bookmarks toolbar.
[Is this code covered by automated tests?]: No, it's CSS only.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Trivial CSS change.
[String changes made/needed]: None

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910679 [details]
Bug 1401735 - Move the toolbarbutton badge slightly down in the bookmarks toolbar.

Polish photon, taking it
should be in 57b3

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b9fd8bc899fa