Closed Bug 1602230 Opened 2 years ago Closed 2 years ago

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

Categories

(WebExtensions :: Frontend, defect, P2)

73 Branch
defect

Tracking

(firefox-esr68 unaffected, firefox72 verified, firefox73 verified, firefox74 verified)

VERIFIED FIXED
mozilla74
Tracking Status
firefox-esr68 --- unaffected
firefox72 --- verified
firefox73 --- verified
firefox74 --- verified

People

(Reporter: eros_uk, Assigned: bgrins)

References

(Blocks 2 open bugs, Regressed 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(5 files, 3 obsolete files)

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

Component: Untriaged → Frontend
Product: Firefox → WebExtensions

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

Flags: needinfo?(eros_uk)

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.

Flags: needinfo?(eros_uk)

(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.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression
Regressed by: 1576946

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 ?

Flags: needinfo?(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>
Flags: needinfo?(emilio)

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

Assignee: nobody → ntim.bugs

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)

Flags: needinfo?(ntim.bugs)
Duplicate of this bug: 1599404
See Also: → 1602924

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

Flags: needinfo?(ntim.bugs)
Assignee: ntim.bugs → nobody

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?

Flags: needinfo?(ntim.bugs)
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Assignee: ntim.bugs → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(ntim.bugs)
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Assignee: ntim.bugs → nobody
Status: ASSIGNED → NEW

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
Attachment #9116256 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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

regression fix for 72.0b8

Attachment #9116256 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Assignee: ntim.bugs → nobody
Status: ASSIGNED → NEW
QA Whiteboard: [qa-triaged]

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.

(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.

Blocks: 1602924
See Also: 1602924

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(jmathies)
Assignee: nobody → ntim.bugs
Attachment #9114887 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee: ntim.bugs → nobody
Status: ASSIGNED → NEW

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

Flags: needinfo?(jmathies)
Priority: -- → P2
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(bgrinstead)

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.

Flags: needinfo?(ntim.bugs)

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

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?

Flags: needinfo?(bgrinstead) → needinfo?(jcristau)
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED

Redirecting to Ryan for 73.

Flags: needinfo?(jcristau) → needinfo?(ryanvm)

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

Flags: needinfo?(ryanvm)

This patch is for 73 beta

This patch is for 73 beta

Depends on D59249

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:
Attachment #9119606 - Flags: approval-mozilla-beta?
Attachment #9119605 - Flags: approval-mozilla-beta?

(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 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.

Attachment #9119605 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9119606 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9116760 - Attachment description: Bug 1602230 - Use a div with textContent instead of xul:label with [value] for toolbarbutton-badges → Bug 1602230 - Use a label with textContent instead of xul:label with [value] for toolbarbutton-badges
Attachment #9116760 - Attachment description: Bug 1602230 - Use a label with textContent instead of xul:label with [value] for toolbarbutton-badges → Bug 1602230 - Use an html:label with textContent instead of xul:label with [value] for toolbarbutton-badges
Pushed by bgrinstead@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

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?

Flags: needinfo?(bgrinstead)
Attached image 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

(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.

(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.

Flags: needinfo?(bgrinstead)
Blocks: 1609979
Blocks: 1610063

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.

Status: RESOLVED → VERIFIED
Attachment #9119606 - Attachment is obsolete: true
Attachment #9119605 - Attachment is obsolete: true
Summary: BrowserAction Badge Text shows only 3 characters → BrowserAction Badge Text shows only 3 characters and causes the toolbarbutton to expand with long strings
Regressions: 1621370
You need to log in before you can comment on or make changes to this bug.