[Affected versions]: - Firefox 46 beta 6 - latest Aurora 47.0a2 - latest Nightly 48.0a1 [Affected platforms]: - Ubuntu 12.04 32-bit - Ubuntu 14.04 64-bit [Steps to reproduce]: 1. Start Firefox 2. Open Show your bookmarks [Expected result]: - Separators are less obvious, thinner. [Actual result]: - Separators are thick and ugly. [Regression range]: - This does affect Firefox after enabling GTK3 builds. [Additional notes]: - Screenshot added to URL section - Firefox 45.0.1 RC is not affected since it's using GTK2.

Seems a bit odd that border-top and border-bottom are set in different files. https://bugzilla.mozilla.org/show_bug.cgi?id=1211892#c31 I don't know why a native separator is not used here.

Is this just happening in the bookmarks panel, and maybe other Customizable UI panels like the developer tools button's panel? Or also elsewhere, like in the main menus?

(In reply to :Gijs Kruitbosch from comment #2) > Is this just happening in the bookmarks panel, and maybe other Customizable > UI panels like the developer tools button's panel? Or also elsewhere, like > in the main menus? Yes so this can be seen with other panels as well: Developer Tools, History and Bookmarks. Also separators from Bookmarks sidebar but no option from Menu bar is affected.

[Tracking Requested - why for this release]: User-visible regression of Firefox's visual appearance.

It affects also Fedora builds. The problem is that we don't have the light border: http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsLookAndFeel.cpp#171

(In reply to Martin Stránský from comment #5) > It affects also Fedora builds. The problem is that we don't have the light > border: > http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsLookAndFeel. > cpp#171 Does this mean this is a widget/GTK rather than a browser/themes issue, then?

(In reply to :Gijs Kruitbosch from comment #6) > (In reply to Martin Stránský from comment #5) > > It affects also Fedora builds. The problem is that we don't have the light > > border: > > http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsLookAndFeel. > > cpp#171 > > Does this mean this is a widget/GTK rather than a browser/themes issue, then? It depends. It can be fixed in theme to use real separators (like the ones in menu) or fix the light border color. But the theme obviously expects that the light border is really light to make the 3D effect.

The CSS box-shadow which is used by Gtk themes is not exported and can't be read by application. According to the Gtk folks the only way how to get the light color properly (or at least to get some approx value) is to call gtk_render_background() and read the bottom color value. This color can be opaque so we need to fill background with window-background color.

Looks like removal of the native separator was http://hg.mozilla.org/mozilla-central/rev/6879e47ff29b#l4.13 That also introduced the non-native hsla(210,4%,10%,.15) shading for the top border http://hg.mozilla.org/mozilla-central/rev/6879e47ff29b#l6.110 Perhaps a similar highlight for the bottom border would be appropriate, if native separators are not suitable. Widget code could fudge a different ThreeDHighlight but it wouldn't be a native color. That makes it hard to pick the "right" color. A translucent color may be best, though there may be risk in making it translucent because, where it was defined in CSS2, translucent colors didn't exist AFAIK, and so there may be assumptions that it is opaque. The described use case is safe for translucent colors if there is a suitable background, and backgrounds in CSS2 included the borders. https://www.w3.org/TR/CSS2/ui.html#system-colors: ThreeDHighlight Highlight color for three-dimensional display elements. https://developer.mozilla.org/en-US/docs/Web/CSS/color_value: ThreeDHighlight The color of the lighter (generally outer) of the two borders facing the light source for 3-D elements that appear 3-D due to two concentric layers of surrounding border. All system colors are deprecated in CSS3: https://drafts.csswg.org/css-color-3/#css2-system "The CSS2 System Color values have been deprecated in favor of the CSS3 UI ‘appearance’ property. If you want to emulate the look of a user interface related element or control, please use the ‘appearance’ property instead of attempting to mimic a user interface element through a combination of system colors." but the section linked from "appearance" doesn't exist. Seems the first thing to investigate is whether -moz-appearance: menuseparator would be suitable. Part 3 of https://bugzilla.mozilla.org/show_bug.cgi?id=938578#c3 may be a partial answer, but I don't know whether that is trying to explain why custom styling is important.

(In reply to Martin Stránský from comment #9) > The CSS box-shadow which is used by Gtk themes is not exported and can't be > read by application. According to the Gtk folks the only way how to get the > light color properly (or at least to get some approx value) is to call > gtk_render_background() and read the bottom color value. This color can be > opaque so we need to fill background with window-background color. Looks like some themes (like the default one in gtk3.20) don't have any shadows so this approach won't work. What about a hotfix which will assign an opposite color to the light one? like light = (0.5-dark)+0.5 I know it's feeble but still better than light = dark which we have now.

(In reply to Karl Tomlinson (ni?:karlt) from comment #10) > Seems the first thing to investigate is whether -moz-appearance: > menuseparator would be suitable. If I remember rightly the background colors are hardcoded, so using gtk-theme-specific colors seems like it won't work very well... (In reply to Martin Stránský from comment #11) > (In reply to Martin Stránský from comment #9) > > The CSS box-shadow which is used by Gtk themes is not exported and can't be > > read by application. According to the Gtk folks the only way how to get the > > light color properly (or at least to get some approx value) is to call > > gtk_render_background() and read the bottom color value. This color can be > > opaque so we need to fill background with window-background color. > > Looks like some themes (like the default one in gtk3.20) don't have any > shadows so this approach won't work. What about a hotfix which will assign > an opposite color to the light one? like > > light = (0.5-dark)+0.5 > > I know it's feeble but still better than light = dark which we have now. I'm really confused though... on Windows and OS X these separators are 1px high. That seems like the desired outcome here. I'm assuming that's what happened under GTK 2 as well. Why are they now 2px high even though comment #10 implies we're using moz-appearance: none on them?

(In reply to :Gijs Kruitbosch from comment #12) > (In reply to Martin Stránský from comment #11) > > (In reply to Martin Stránský from comment #9) > > > The CSS box-shadow which is used by Gtk themes is not exported and can't be > > > read by application. According to the Gtk folks the only way how to get the > > > light color properly (or at least to get some approx value) is to call > > > gtk_render_background() and read the bottom color value. This color can be > > > opaque so we need to fill background with window-background color. > > > > Looks like some themes (like the default one in gtk3.20) don't have any > > shadows so this approach won't work. What about a hotfix which will assign > > an opposite color to the light one? like > > > > light = (0.5-dark)+0.5 > > > > I know it's feeble but still better than light = dark which we have now. > > I'm really confused though... on Windows and OS X these separators are 1px > high. That seems like the desired outcome here. I'm assuming that's what > happened under GTK 2 as well. Why are they now 2px high even though comment > #10 implies we're using moz-appearance: none on them? Finally managed to get my VMs set up again (one of my machines died). So it seems that under gtk2 the separators *were* already using 2px, and we were using the builtin border on one side and a themed one on the other. :-\ Considering this seems to only affect the non-menuseparator-appearance ones in the panel I imagine the simplest fix will just be to fix this on the Firefox side, though it does seem like the ThreeDHighlight color should be fixed. "Deprecated" in CSS2 doesn't really mean much, system colors like that end up being used fairly frequently in the Firefox theme to get styling that obeys GTK and Windows theming. We can't very well use menuseparator appearance here because the background is -moz-dialog, and there's no guarantee that dialog and menu backgrounds aren't different. We should just hardcode both the top and bottom color on linux.

Comment on attachment 8738983 [details] [diff] [review] fix separators in panel UI on Linux, #fff is likely completely wrong for dark themes. Can we just remove the bottom border here? Might make sense on Windows too.

This patch tries co calculate a light border color as a complement to the dark one. It's not a complete solution to this bug, rather a workaround for the missing piece. Although IMHO it's still better than just copy the dark color there.

(In reply to Dão Gottwald [:dao] from comment #15) > Comment on attachment 8738983 [details] [diff] [review] > fix separators in panel UI on Linux, > > #fff is likely completely wrong for dark themes. > > Can we just remove the bottom border here? Might make sense on Windows too. Hah, I hadn't even seen the one on Windows against the lighter grey background there. But sure, we can just kill it off.

Comment on attachment 8739000 [details] [diff] [review] light border color calculation This will, for example, select yellow for light when dark is blue. I think that is not what we want. But picking the right color here is difficult. The problems stem from the fact that themes don't in general use two borders with light and dark sides. Now we have chrome using colors like ThreeDShadow to draw frames around text entry boxes, even though those frames do no have light and dark sides. That actually seems to work well because we have chosen a color from the theme, instead of a random shade. My guess is that something like hsla(x, 0%, 100%, 0.15) would work OK in this situation, but it is an accident that ThreeDHighlight is being used in this situation, and that color may not work in other situations because hsla(x, 0%, 100%, 0.15) would not be a color from the theme. It is more visible on dark themes than light themes. I think we can wait to see what the use case is before trying to select a color for ThreeDHighlight, but I guess we could try hsla(x, 0%, 100%, 0.15) now, if you'd like to. menu.css has: menuseparator { -moz-appearance: menuseparator !important; margin: 2px 0; border-top: 1px solid ThreeDShadow; border-bottom: 1px solid ThreeDHighlight; } The borders are only used if -moz-appearance is changed, but similar arguments to those re -moz-appearance: menuseparator not being suitable because the background color is not MenuBackground, could also be applied to ThreeDShadow/ThreeDHighlight. Perhaps this rule should just use a translucent shade and highlight.

(In reply to Dão Gottwald [:dao] from comment #15) > Can we just remove the bottom border here? Might make sense on Windows too. On dark themes the shade on the top border will be almost invisible, and so a highlight like hsla(x, 0%, 100%, 0.15) may be useful.

(In reply to Karl Tomlinson (ni?:karlt) from comment #18) > Perhaps this rule should just use a translucent shade and highlight. Maybe even a gradient to produce a curved trough on hi-dpi displays, but that's beyond the scope of this bug. We don't know whether the theme uses hard or soft separators, because -moz-appearance is not in use.

(In reply to Karl Tomlinson (ni?:karlt) from comment #19) > (In reply to Dão Gottwald [:dao] from comment #15) > > Can we just remove the bottom border here? Might make sense on Windows too. > > On dark themes the shade on the top border will be almost invisible, and so a > highlight like hsla(x, 0%, 100%, 0.15) may be useful. Dão, what do you think?

It's a native background color, so we should use a native border-top color and still remove border-bottom.

Getting late for 46 here, but since it's just css I think we could still potentially take it in beta. We are heading for the beta 11 build on Thursday. If that doesn't seem possible let's wontfix for 46.

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #24) > Getting late for 46 here, but since it's just css I think we could still > potentially take it in beta. We are heading for the beta 11 build on > Thursday. If that doesn't seem possible let's wontfix for 46. I don't know, it depends on whether Dão thinks the patch can get r+.

Comment on attachment 8739396 [details] [diff] [review] only use 1 separator border and use a native color on Linux, We also use a native background panel background color on Windows as far as I remember. Seems like we should use ThreeDShadow or ThreeDLightShadow there too. Even better would be if this would work across the board so we could get rid of the CSS variable.

OS X does not use a native color for the panel background, so I kept the CSS variable, but made the default ThreeDShadow, which seems to be fine on my win8 machine, at least - and then kept the hardcoded semi-transparent version in the OS X stylesheet.

If you request uplift I could take this for tomorrow's build

Note that this patch has implications for winnt builds also, which are more likely to be obvious with dark themes. There is risk in taking this for uplift. If it is worth the risk, then I would advise testing with dark nt themes.

(In reply to Karl Tomlinson (ni?:karlt) from comment #31) > Note that this patch has implications for winnt builds also, which are more > likely to be obvious with dark themes. > > There is risk in taking this for uplift. If it is worth the risk, then I > would advise testing with dark nt themes. We don't really support custom themes. However, I tested with some high contrast themes, and the patch doesn't seem to work well for them. I haven't figured out why yet because the devtools are being no help whatever - in effect, on Windows ThreeDShadow should already be set by toolkit CSS, and the patch should be a no-op, but that isn't what happens and I don't know why. I'll keep needinfo until I figure this out; might want to back this out. In the meantime, this is not going to make 46, which is a shame, but there we are.

Backout: remote: https://hg.mozilla.org/integration/fx-team/rev/b40daaf11247 The issues I saw in the previous comments are caused by each respective browser.css file defining this exact variable, too. Backing out so I can rethink how this should work.

This is the same patch as before but without defining the variable, as we're defining it in browser.css already. It uses ThreeDShadow on Linux, ThreeDLightShadow on Windows, and the same hsla custom value on OS X as before but with .14 instead of .15 alpha (I doubt anyone can tell, tbh, but technically it's different.). I think this looks and works fine, but I didn't want to just assume that and only back out half the patch. What do you think?

Hi Karl, Gijs, I'd like to wontfix this for Fx47. Do you have any concerns? Do you think the risk to uplift to Beta is low?

(In reply to Ritu Kothari (:ritu) from comment #37) > Hi Karl, Gijs, I'd like to wontfix this for Fx47. Do you have any concerns? > Do you think the risk to uplift to Beta is low? I don't think it's too bad, but I would prefer we focus on other serious gtk3 issues instead. The current situation is ugly, but not unusably so - unlike some of the other gtk3 things on file (bug 1262136 in particular, but also e.g. bug 1244305, bug 1269523, bug 1268462).

wontfix on 47 is OK with me. I don't know the risk associated with the patch.

(In reply to :Gijs Kruitbosch from comment #38) > (In reply to Ritu Kothari (:ritu) from comment #37) > > Hi Karl, Gijs, I'd like to wontfix this for Fx47. Do you have any concerns? > > Do you think the risk to uplift to Beta is low? > > I don't think it's too bad, but I would prefer we focus on other serious > gtk3 issues instead. The current situation is ugly, but not unusably so - > unlike some of the other gtk3 things on file (bug 1262136 in particular, but > also e.g. bug 1244305, bug 1269523, bug 1268462). Thanks Gijs and KarlT. This now a wontfix for Fx47. I will try to follow up on the other bugs you listed Gijs.

I've managed to reproduce this bug on Nightly 48.0a1 (2016-03-31); (Build ID : 20160331030231) on Linux, 64 Bit. This Bug is now verified as fixed on Latest Firefox Developer Edition 48.0a2 (2016-05-05) Build ID: 20160505004017 User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0 OS: Ubuntu 16.04 ; Linux 4.4.0-21-generic x86-64