Closed Bug 1401735 Opened 7 years ago Closed 7 years ago

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

Categories

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

57 Branch
x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- verified
firefox58 --- verified

People

(Reporter: Virtual, Assigned: johannh)

References

Details

(4 keywords, Whiteboard: [reserve-photon-visual])

Attachments

(7 files)

[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.
Has Regression Range: --- → yes
Has STR: --- → yes
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.
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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Whiteboard: [photon-visual] [triage] → [reserve-photon-visual]
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify?
Priority: -- → P1
Flags: qe-verify? → qe-verify+
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.
Attachment #8910679 - Flags: review?(gijskruitbosch+bugs) → review+
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].
Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4e1c7dcc09d7 Move the toolbarbutton badge slightly down in the bookmarks toolbar. r=Gijs
Iteration: 57.3 - Sep 19 → ---
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jhofmann)
(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
Summary: Extension button notification in Bookmarks Toolbar is truncated after landing patch from bug #1389966 → Extension button notification/badge in Bookmarks Toolbar is truncated after landing patch from bug #1389966
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"
Attachment #8910679 - Flags: approval-mozilla-beta?
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.
Attachment #8910679 - Flags: approval-mozilla-beta?
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
Flags: needinfo?(jhofmann)
Attachment #8910679 - Flags: approval-mozilla-beta?
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
Attachment #8910679 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: