1979037 - User-created toolbarseparator elements in the toolbar are now invisible (on macOS and potentially other OSes)

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P3)

Description

[:Gijs (he/him)]

Reporting via the (tagged) upset comments on bug 1891271.

STR:

1. add toolbarseparator to the navbar by manipulating browser.uiCustomization.state (see also this superuser item).
2. restart

ER:
visible separator

AR:
no separator

regression window on macOS: https://hg-edge.mozilla.org/mozilla-central/pushloghtml?fromchange=536dc07bdeaed3ba520c5f9171ef4752df8525f4&tochange=3bf656ec4a648238bfbcf41f0b446464af1c102d

probably bug 1971443 on macOS; unclear right now if this is a problem on other OSes and/or what the regressor would be, as bug 1891271 comment 4 and 5 did not provide that information and I am not in a position to check myself right now (please update the platform info + summary if you can). It's possible bug 1809372 is also related.

Greg or Emilio, do either of you have cycles to look at this?

Comment 1

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #0)

Greg or Emilio, do either of you have cycles to look at this?

Comment 2

[Gregory Pappas [:gregp]]

At least on Linux, this is not a recent regression. Using mozregression, I learned that these separators became invisible on Linux in this range https://hg-edge.mozilla.org/mozilla-central/pushloghtml?fromchange=76f22f29fcf9cd40a92a7bcf6b0a11cc782936b8&tochange=23ee4ac2d048de0aac3fa27ce7eb0925c1903096

Likely bug 1866022.

Should we just wontfix this? These separators haven't been officially supported for over a decade and have been in varying degrees of broken across platforms for years, it seems.

Comment 3

[:Gijs (he/him)]

(In reply to Gregory Pappas [:gregp] from comment #2)

At least on Linux, this is not a recent regression. Using mozregression, I learned that these separators became invisible on Linux in this range https://hg-edge.mozilla.org/mozilla-central/pushloghtml?fromchange=76f22f29fcf9cd40a92a7bcf6b0a11cc782936b8&tochange=23ee4ac2d048de0aac3fa27ce7eb0925c1903096

Likely bug 1866022.

Yeah, that removed the moz-appearance.

Should we just wontfix this? These separators haven't been officially supported for over a decade and have been in varying degrees of broken across platforms for years, it seems.

I don't have a strong opinion. bug 1971526 suggests that we want (perhaps for other reasons than user-customizable separators) toolkit styles for them. We still have user-customizable separators in the bookmarks toolbar. Roughly I see 3 options:

1. wontfix this and do nothing
2. wontfix this and actively remove support for customizable toolbar separators from CUI.
3. fix this bug
4. fix this bug and revert the decision in bug 985308 and whatnot (needs product/UX input)

Cost-wise, I think (1) is the least work, and (2) is more work than (3), and (4) is quite expensive. Benefit-wise, (2) and (1) are obviously roughly equivalent for users but (1) leaves tech debt and "explaining how this works" debt and all the rest of it. (3) similarly leaves some debt but is consistent with the past 11 years. (4) feels like a bad cost/benefit trade-off unless there's some connect.m.o item I'm unaware of with a gazillion votes that suggest this is definitely what users want most for Firefox, which seems unlikely.

If we have other reasons to want toolkit styles for separators than fixing bug 1971526 here for separators in a way that fixes the regression seems like the best trade-off, but I'm open to strong opinions from you or Emilio that I'm wrong about some of the cost/benefit assertions above.

Comment 4

[Gregory Pappas [:gregp]]

I think we should actively remove support for customizable toolbar separators from CUI. Even before these visual regressions, the feature did not work well. In customize mode, they're very hard to drag. They also can't be dropped onto the customization palette, the separator just disappears forever if you try.

None of this is hard to fix, but if there's already been a conscious product decision to not surface these separators in the UI, they shouldn't be kept around in a zombie state

Comment 5

[Gregory Pappas [:gregp]]

Although I'm not familiar enough with the internals of CUI to know if totally removing support is without downsides. Would the now unrecognized customizableui-special-separator items be silently ignored, or would we have to add a migration to avoid breaking profiles?

Comment 6

[Emilio Cobos Álvarez [:emilio]]

I don't have a strong opinion on the CUI stuff, but doing bug 1971526 is easy and it's worth cleaning up a bit, so I just sent patches there.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

With the patches from that bug the toolbar separators are visible. They are a bit too tall if you place them around in the toolbar tho.

Something like this would be useful:

diff --git a/browser/themes/shared/toolbarbuttons.css b/browser/themes/shared/toolbarbuttons.css
index e7761dcd5301..6694b04e9a35 100644
--- a/browser/themes/shared/toolbarbuttons.css
+++ b/browser/themes/shared/toolbarbuttons.css
@@ -580,6 +580,7 @@ toolbar:not(#TabsToolbar) > #personal-bookmarks {

 toolbarseparator {
   color: var(--toolbarseparator-color);
+  padding-block: var(--toolbarbutton-inner-padding);

   #PlacesToolbarItems > & {
     /* Make separators wider so they're easier to drag. Make them also a bit

But not sure we want to do that by default. Happy to r+ such a patch tho, I guess :)

Comment 8

[bughit]

regressed on win10 and win11 in 141

Comment 9

[:Gijs (he/him)]

(In reply to Gregory Pappas [:gregp] from comment #5)

Although I'm not familiar enough with the internals of CUI to know if totally removing support is without downsides. Would the now unrecognized customizableui-special-separator items be silently ignored, or would we have to add a migration to avoid breaking profiles?

We'd have to be pretty careful because the special nodes are, uh, specialcased in a few different places, unfortunately (there's a helper that should be used everywhere but I'm fairly sure it isn't). Apart from that, I think it should gracefully fall back.

But stepping back, I think if we're doing bug 1971526 I would prefer to not remove support - it's more work than either that bug or the suggestion in comment 7, and the only thing we'll achieve is annoy people who have gone to fairly great lengths to get a particular experience (and are therefore presumably attached to that experience). That doesn't mean we should or do officially support this, it's just better to spend our time doing other things than "properly" remove support entirely.

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Oh, I think I added the line in comment 7 in bug 1971526, so should be working now. Can you confirm?

Comment 11

[:Gijs (he/him)]

Yep, the separator appears for me on macOS and Windows. Thanks!