1602924 - Refactor toolbarbutton-badge to get more consistent rendering of long badges

Status: NEW

(WebExtensions :: Frontend, task, P3)

Description

[Tim Nguyen :ntim]

See bug 1602230

Comment 1

[Jim Mathies [:jimm]]

Hey Tim, do you plan to fix this?

Comment 2

[Brian Grinstead [:bgrins]]

I won't have time to work on this before the new year. I put up a patch in Bug 1602230 that should unblock this I think, but I'm not positive. I'm also not sure if/when it will land. So if we for sure want this and Bug 1602230 fixed in 73 then we would need to back out the nsStackFrame removal in mozilla-central (currently backed out only on beta) and use legacy-stack as we did for beta in https://bugzilla.mozilla.org/show_bug.cgi?id=1602230#c15.

Tim, what do you think about that option?

Comment 3

[Tim Nguyen :ntim]

(In reply to (Mostly unavailable through Jan 1) Brian Grinstead [:bgrins] from comment #2)

I put up a patch in Bug 1602230 that should unblock this I think, but I'm not positive. I'm also not sure if/when it will land.

This patch should unblock things, in addition to your patch (which makes abspos work) and removing the negative margins from the badge, adding the following to .toolbarbutton-badge seems to fix things up:

position: absolute;
top: 0;
inset-inline-end: 0;

My concern is more about the time-consuming/tedious nature of getting it to a shippable state since there are the following things to do:

• updating+testing all different types of badges to make them work as before... (CFR feature callout badges, warning and success update badges, #pageAction-panel-addSearchEngine, potentially others...)
• potentially having automated tests to update (although this should be more trivial)

Doing this will be a really nice clean up though, since it'll remove special-case negative margins for different types of badges.

So if we for sure want this and Bug 1602230 fixed in 73 then we would need to back out the nsStackFrame removal in mozilla-central (currently backed out only on beta) and use legacy-stack as we did for beta in https://bugzilla.mozilla.org/show_bug.cgi?id=1602230#c15.
Tim, what do you think about that option?

I'd prefer having a proper fix for 73 (like using abspos), but given it isn't risk-free, I guess that would be fine.

I won't have time to handle this until the end of the month though. Is a proper fix and/or using legacy-stack on 73 you could look at?

Thanks and happy new year!

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim (blocking ni? until Jan 22th) from comment #3)

I'd prefer having a proper fix for 73 (like using abspos), but given it isn't risk-free, I guess that would be fine.

I won't have time to handle this until the end of the month though. Is a proper fix and/or using legacy-stack on 73 you could look at?

At this point I think switching to legacy-stack for 73 is our best option. As you mention there's a lot of testing that needs to be done here and I'd prefer to not have to also uplift the changes (and then any regression fixes) to beta.

If we want to go that route, then we need to land https://phabricator.services.mozilla.com/D56658 and https://bugzilla.mozilla.org/attachment.cgi?id=9116256 to 73 beta, right?

Comment 5

[Brian Grinstead [:bgrins]]

The bug as originally filed isn't a problem anymore, right? Bug 1602230 fixed the stretching issue. I wonder if this should really be duped with Bug 1610063 where Comment 3 is the proper fix for the issue described there. What do you think?

Comment 6

[Tim Nguyen :ntim]

I think the current state makes sense, this bug can be used for refactoring, bug 1610063 can be used to fix the issue.

Comment 7

[Ethan Tseng [:ethan]]

Tim, should we update the priority of this bug as P3 (the same as bug 1610063), and could we remove the regression keyword?

Comment 8

[Tim Nguyen :ntim]

(In reply to Ethan Tseng [:ethan] from comment #7)

Tim, should we update the priority of this bug as P3 (the same as bug 1610063), and could we remove the regression keyword?

Yeah, makes sense, thanks!