1602230 - BrowserAction Badge Text shows only 3 characters and causes the toolbarbutton to expand with long strings

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P2)

Description

[erosman]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:73.0) Gecko/20100101 Firefox/73.0

Steps to reproduce:

Recently in Nightly (as well as Developer Edition, I have heard) BrowserAction Badge Text shows only 3 characters (instead of 4).

Furthermore, icon shifts left when the badge text changes from nothing to characters. The shift was not present before.

Ref:
https://github.com/erosman/support/issues/107
https://github.com/foxyproxy/firefox-extension/issues/51

Comment 1

[Miruna Curtean]

Hello,
Could you please provide some detailed steps as well?
Am I correct in understanding that these extensions were references in the github issues?
https://addons.mozilla.org/en-US/firefox/addon/foxyproxy-standard/?src=search
https://addons.mozilla.org/en-US/firefox/addon/foxytab/?src=search

Thanks

Comment 2

[erosman]

To test, you need any extension that usually creates multi-character badges (many only have numbers that usually don't go higher than 3 digits).

Alternatively, from webextensions-examples
https://github.com/mdn/webextensions-examples/tree/master/tabs-tabs-tabs

Make the following change to:
https://github.com/mdn/webextensions-examples/blob/master/tabs-tabs-tabs/background.js#L12

browser.browserAction.setBadgeText({text: '1234' + length.toString()});

Install & test.

Comment 3

[Miruna Curtean]

(In reply to erosman from comment #2)

Make the following change to:
https://github.com/mdn/webextensions-examples/blob/master/tabs-tabs-tabs/background.js#L12

browser.browserAction.setBadgeText({text: '1234' + length.toString()});

Install & test.

Thank you for making this easier to check with that modification.
Indeed in comparison to Firefox Release 71 where the Badge of the loaded tabs-tabs-tabs extension displayed all 4 characters, on FF Developer edition 72.0b4 (20191206183317), FF Beta 72.0b4 (20191206183317) and Firefox Nightly 73.0a1 (20191209215019) only three of the characters are displayed.
Since it looks like even 72 branch versions are affected I have marked that field as such.
Tested on Windows Pro 10 64-bit, Ubuntu 18.04.3 LTS and macOS catalina 10.05.

Comment 4

[Miruna Curtean]

Attachment: Badge text 3 characters.gif

Comment 5

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5e6b050c9427da9ae37ca4c7c8000ba5b47ceea8&tochange=1a85c571a4645798ba4d9295ba2504acac149ac8

Comment 6

[Tim Nguyen :ntim]

It looks like adding max-width: 28px on .toolbarbutton-badge substracts the padding when applying the max-width. So the width only reaches 24px rather than the specified max-width: 28px;. box-sizing: content-box seems to fix the issue but it also increases the applied min-width by taking in account the padding (so from 14px to 18px). It's also possible to leave box-sizing to border-box and increase the max-width to 32px.

I could write a patch to fix this by fiddling with the min/max-width, however I don't think box-sizing is supposed to affect how min/max-width are applied. Emilio, do you know whether this is expected ?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

box-sizing definitely changes how all widths and heights are computed:

data:text/html,<div style="max-width: 100px; padding: 10px; border: 10px; background: green; height: 100px"></div><div style="box-sizing: border-box; max-width: 100px; padding: 10px; border: 10px; background: green; height: 100px"></div>

Comment 8

[Tim Nguyen :ntim]

Hmm, I guess this was a XUL quirk then. Good to know, thanks!

Comment 9

[Tim Nguyen :ntim]

Attachment: Bug 1602230 - Fix .toolbarbutton-badge max-width.

Comment 10

[erosman]

Tim: Please also check the icon jump/shift mentioned in the first post by setting the badge to '' and then change to '1234' (although the issue may have been solved by fixing the width)

Comment 12

[Tim Nguyen :ntim]

I'll split off the shifting in bug 1602924, since it requires more significant changes.

Comment 13

[Brian Grinstead [:bgrins]]

After looking into this, I think it's not easy to fix this to just "show 4 characters" without also fixing the shifting which is currently spun off in Bug 1602924.

The first thing I've looked at here is to stop using <xul:label value="123">. The [value] variation is not playing nicely at all with CSS sizing. Using text=badge instead of value=badge here https://searchfox.org/mozilla-central/rev/c61720a7d0c094d772059f9d6a7844eb7619f107/toolkit/content/widgets/toolbarbutton.js#42,64 will make it a regular -moz-box xul element which does help make things more sane. Even with that change the sizing isn't working quite right (a large badge is expanding the parent grid). So then also looking at additionally:

• possibly switch the xul label to a div or to ::after pseudo element with content: attr(badge) to use absolute positioning.
• stop using <stack> / CSS grid altogether

The tricky part is in the 'badge is too long' case we still want the start of the string to be visible so using some combination of absolute positioning / hidden overflow / right: 0 won't work. I think it makes sense to come up with a web-content test case to get the affect we want (to get rid of additional complexity we are hitting from XUL / -moz-box), and then port that into the Custom Element. I started playing with it at https://glitch.com/edit/#!/past-mat / https://past-mat.glitch.me/ but I'm not sure how much time I'll have to look further.

Unless if you can think of a way to do this in the short term, what do you think about switchig this to use <legacy-stack> in the meantime? And, is that only available on beta or could we use it in m-c as well?

Comment 14

[Tim Nguyen :ntim]

Attachment: Bug 1602230 - Use legacy-stack for toolbarbutton badge. r=bgrins

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 9116256 [details]
Bug 1602230 - Use legacy-stack for toolbarbutton badge. r=bgrins

Beta/Release Uplift Approval Request

• User impact if declined: Extension button badges hide essential information
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): restores things the way they were before bug 1576946
• String changes made/needed: none

Comment 16

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9116256 [details]
Bug 1602230 - Use legacy-stack for toolbarbutton badge. r=bgrins

regression fix for 72.0b8

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/443a94cad323

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: Bug 1602230 - Use an html:label with textContent instead of xul:label with [value] for toolbarbutton-badges

This both allows proper CSS sizing (it respects max-width), and allows for hidden overflow for long badges.
Using xul:label with textContent fixes the former but doesn't support the latter, so a div is used instead.

This does not fix the shifting issue for long badges (see Bug 1602924), but using an HTML element will allow
this to be positioned absolutely in order to not affect the toolbarbutton sizing.

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #13)

After looking into this, I think it's not easy to fix this to just "show 4 characters" without also fixing the shifting which is currently spun off in Bug 1602924.

FWIW, I think I found a way to handle this separately by using a div with overflow: hidden instead of a xul:label.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 21

[Shane Caraveo (:mixedpuppy)]

@bgrins or @ntim, who should own this to get the patch landed?

Comment 22

[Tim Nguyen :ntim]

I'm busy until the end of January, so I won't be able to finish this until then. I left some info in https://bugzilla.mozilla.org/show_bug.cgi?id=1602924#c3 if someone wants to take a look.

Comment 23

[Miruna Curtean]

As this has been marked fixed on 72, QA has verified that the restore to the previous functionality has been successful.
Tested on Windows 10 Pro 64-bit, macOS Catalina10.15 and Ubuntu 18.04.3 LTS on Firefox Nightly 72.0a1 (20191111100226) and FF Beta 72.0 (20200103162918).
Using the tabs-tabs-tabs test extension modified to show a badge text, it can be noticed that:

1. The extension badge test now shows a maximum of 4 characters.
2. Trying to display a longer text that 4 characters is not working, as expected.
3. This applies for numbers, letters and special characters.
   Thank you

Comment 24

[Brian Grinstead [:bgrins]]

Tim and I have been discussing in Bug 1602924 and I think we should go ahead and use legacy-stack on 73 to give time / stability to come up with a true fix here (see https://bugzilla.mozilla.org/show_bug.cgi?id=1602924#c4).

Julian, I'd like to land two backouts that we landed in 72 beta in 73 beta as well:

1. Restoring nsStackFrame (https://bugzilla.mozilla.org/show_bug.cgi?id=1599783#c39)
2. The patch from this bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1602230#c16)

What's your preferred workflow? Should I move/copy the patch from Bug 1599783 here and then do two separate beta uplift requests? Or are the previous approval-mozilla-beta+'s sufficient and we need to get it pushed to 73? Something else?

Comment 25

[Julien Cristau [:jcristau] (back April 22)]

Redirecting to Ryan for 73.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

If you can attach a patch here after verifying that things backout cleanly, I'd prefer that.

Comment 27

[Brian Grinstead [:bgrins]]

Attachment: Bug 1602230 - Backed out changeset 1a85c571a464 (bug 1576946)

This patch is for 73 beta

Comment 28

[Brian Grinstead [:bgrins]]

Attachment: Bug 1602230 - Use legacy-stack for toolbarbutton badge

This patch is for 73 beta

Depends on D59249

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 9119606 [details]
Bug 1602230 - Use legacy-stack for toolbarbutton badge

Beta/Release Uplift Approval Request

• User impact if declined: Extension button badges hide the fourth character with long strings and have shifting layout
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See Comment 2
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Restores things the way they were before bug 1576946. This stack was also uplifted to 72 beta. We are working on a fix to land in nightly 74 so that we don't need to land this on m-c.
• String changes made/needed:

Comment 30

[Brian Grinstead [:bgrins]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)

If you can attach a patch here after verifying that things backout cleanly, I'd prefer that.

I've attached versions of the two patches referenced in Comment 24 built and tested locally against 73.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9119605 [details]
Bug 1602230 - Backed out changeset 1a85c571a464 (bug 1576946)

Reverts us back to pre-bug 1576946 behavior like we did for Fx72. Approved for 73.0b3.

Comment 32

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/51efa4d13103
https://hg.mozilla.org/releases/mozilla-beta/rev/c79e93ea23c9

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90cf7c4f4474
Use an html:label with textContent instead of xul:label with [value] for toolbarbutton-badges r=Gijs

Comment 34

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/90cf7c4f4474

Comment 35

[Miruna Curtean]

Attachment: Badge icon text cropped.gif

Hello,

I've checked this on Windows 10-64 and Ubuntu 18.04 on FF Beta 73.0b5 (20200115020958) and FF Nightly latest version 74.0a1 (20200115213621).
While on Beta 4 characters are displayed successfully when loading the tabs-tabs-tabs modified extension, on Nightly the last number/letter is cropped from view, showing only partially.
Could you please look into that?

Comment 36

[erosman]

Attachment: badge.jpg

tested on: 74.0a1 (2020-01-16) (64-bit)
There is also a weird behaviour when setting text (not numbers) as per the blue badge in the attachment

Comment 37

[Tim Nguyen :ntim]

(In reply to erosman from comment #36)

Created attachment 9121454 [details]
badge.jpg

tested on: 74.0a1 (2020-01-16) (64-bit)
There is also a weird behaviour when setting text (not numbers) as per the blue badge in the attachment

We should probably add white-space: nowrap; to fix this.

Comment 38

[Brian Grinstead [:bgrins]]

(In reply to Miruna Curtean from comment #35)

Created attachment 9121301 [details]
Badge icon text cropped.gif

Hello,

I've checked this on Windows 10-64 and Ubuntu 18.04 on FF Beta 73.0b5 (20200115020958) and FF Nightly latest version 74.0a1 (20200115213621).
While on Beta 4 characters are displayed successfully when loading the tabs-tabs-tabs modified extension, on Nightly the last number/letter is cropped from view, showing only partially.
Could you please look into that?

Yeah that positioning looks a bit off. Hopefully we can fix that by adjusting some of the CSS sizing.

(In reply to erosman from comment #36)

Created attachment 9121454 [details]
badge.jpg

tested on: 74.0a1 (2020-01-16) (64-bit)
There is also a weird behaviour when setting text (not numbers) as per the blue badge in the attachment

Thanks for the report! Tim's suggestion in Comment 37 seems to fix this, so I'll make a patch to do that.

I'm going to handle both of these issues in a follow up bug since this one has gotten pretty complicated with the uplifts and the different landing on m-c.

Comment 39

[Miruna Curtean]

I'm setting this as verified fixed as the work for the cropping issue and the text wrapping on the extension icon are being dealt with in https://bugzilla.mozilla.org/show_bug.cgi?id=1610063 and respectively https://bugzilla.mozilla.org/show_bug.cgi?id=1609979.