1289510 - Bookmarks toolbar buttons have weird hover styling when using a lightweight theme

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

[Tracking Requested - why for this release]:

The hover styling is broken in various ways:

- missing background texture
- text color changes when it shouldn't
- text-shadow disappears

Note that these things did make sense with the native toolbar button appearance because that comes with a solid background.

Comment 1

[Dão Gottwald [:dao]]

This is on Linux. I haven't tested this on Windows.

Comment 2

[Marcia Knous [:marcia]]

Tracking 50+ for this regression. Adding ni on Florin to check Windows to see if it occurs there as well.

Comment 3

[Cornel Ionce [:noni] [Hubs QA]]

I've investigated this matter on Windows 10 x64 and noticed the following:
- the background texture is indeed not the desired one (specs in bug 734326)
- the button edges are sharp (rounded in specs)
- text color is not changing as it does on Ubuntu.

Here is a screenshot with the hovered state on Windows OS: http://i.imgur.com/bMRPses.png

Please let me know if I can provide further assistance regarding this matter.

Comment 4

[Rakhi(:rakhisharma)]

Attachment: Bug 1289510 - Bookmarks toolbar buttons have weird hover styling when using a lightweight theme. .

Review commit: https://reviewboard.mozilla.org/r/68184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68184/

Comment 5

[Rakhi(:rakhisharma)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6dbf7838c18

Comment 6

[Dão Gottwald [:dao]]

There's a typo in browser/themes/linux/browser.css. 'background-color: var(--toolbarbutton-hover-background);' needs to use background instead.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8776402 [details]
Bug 1289510 - Bookmarks toolbar buttons have weird hover styling when using a lightweight theme. .

https://reviewboard.mozilla.org/r/68184/#review65362

::: browser/themes/linux/browser.css:151
(Diff revision 1)
>  toolbarbutton.bookmark-item:not(.subviewbutton):hover:not([open]) {
>    background-color: var(--toolbarbutton-hover-background);

Thanks dao for catching that. Rakhi, this line should get fixed as part of this patch. After you fix this line, you should be able to remove the `background` property in the `:-moz-lwtheme` rule you're adding below. Please test it to make sure.

::: browser/themes/windows/browser.css:485
(Diff revision 1)
>    box-shadow: var(--toolbarbutton-active-boxshadow);
>    background: var(--toolbarbutton-active-background);
>  }
>  
> +toolbarbutton.bookmark-item:not(.subviewbutton):hover:not([disabled="true"]):-moz-lwtheme {
> +  border-color: var(--toolbarbutton-hover-bordercolor);

This border-color property doesn't seem to be necessary because it is already defined above by the selector for
`toolbarbutton.bookmark-item:not(.subviewbutton):hover:not([disabled="true"]):not([open])`

However, it looks like the border is very faint and that is because the background is extending underneath the border. In the rule above for `toolbarbutton.bookmark-item:not(.subviewbutton)`, can you add `background-clip: padding-box`? The navbar already uses `background-clip: padding-box;` on toolbarbuttons.

Comment 8

[Rakhi(:rakhisharma)]

Comment on attachment 8776402 [details]
Bug 1289510 - Bookmarks toolbar buttons have weird hover styling when using a lightweight theme. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68184/diff/1-2/

Comment 9

[Rakhi(:rakhisharma)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37a669f7d84a

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot of patch

This still doesn't look quite right. Here's a screenshot of the home button hovered next to a button from the bookmarks toolbar items. It looks like the border of the bookmarks toolbar items is a bit darker than the one from the home button.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8776402 [details]
Bug 1289510 - Bookmarks toolbar buttons have weird hover styling when using a lightweight theme. .

https://reviewboard.mozilla.org/r/68184/#review65742

Comment 12

[Rakhi(:rakhisharma)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> Created attachment 8777084 [details]
> Screenshot of patch
> 
> This still doesn't look quite right. Here's a screenshot of the home button
> hovered next to a button from the bookmarks toolbar items. It looks like the
> border of the bookmarks toolbar items is a bit darker than the one from the
> home button.

ok, so Doesn't there is need of adding  border-color: var(--toolbarbutton-hover-bordercolor); here? that I did in last patch comment #4? As you already pointed out in commenet #7 that the border color is already defined by the selector 
`toolbarbutton.bookmark-item:not(.subviewbutton):hover:not([disabled="true"]):not([open])`
But It doesn't seems working in lwtheme? Please see the linux diff also, I have added border-color there too and it seems working. I think I need to add border color for window lwtheme also?

Comment 13

[Marcia Knous [:marcia]]

Tracking 51+.

Comment 14

[Rakhi(:rakhisharma)]

Comment on attachment 8776402 [details]
Bug 1289510 - Bookmarks toolbar buttons have weird hover styling when using a lightweight theme. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68184/diff/2-3/

Comment 15

[Rakhi(:rakhisharma)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=236b4b29377a

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8776402 [details]
Bug 1289510 - Bookmarks toolbar buttons have weird hover styling when using a lightweight theme. .

https://reviewboard.mozilla.org/r/68184/#review67302

Looks good!

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8776402 [details]
Bug 1289510 - Bookmarks toolbar buttons have weird hover styling when using a lightweight theme. .

https://reviewboard.mozilla.org/r/68184/#review67314

::: browser/themes/windows/browser.css:468
(Diff revision 3)
> +  background-origin: padding-box !important;
> +  background-clip: padding-box !important;

It looks like you needed !important here because the rules for `toolbarbutton.bookmark-item:not(.subviewbutton):hover:active:not([disabled="true"])` and `toolbarbutton.bookmark-item:not(.subviewbutton):hover:not([disabled="true"]):not([open])` set the `background` shorthand property and overwrite what you're setting here.

I don't see where `--toolbarbutton-hover-background` is defined for the #personal-bookmarks, only #navbar.

Instead of this change, please move these two rules to the two selectors below after the background property is defined. We shouldn't need them if the buttons are hovered, active, or open.

Comment 18

[Rakhi(:rakhisharma)]

Comment on attachment 8776402 [details]
Bug 1289510 - Bookmarks toolbar buttons have weird hover styling when using a lightweight theme. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68184/diff/3-4/

Comment 19

[Rakhi(:rakhisharma)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a064f4ecd3b9

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/e61b48ca0d637b83d2b81cbb9b2f8713338a92fb
Bug 1289510 - Bookmark toolbar items should match navbar toolbar item hover style when lightweight themes are applied. r=jaws

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e61b48ca0d63

Comment 22

[Andrew Overholt [:overholt]]

Can/should we uplift this to 50 (seems it's also affected)?

Comment 23

[Rakhi(:rakhisharma)]

Comment on attachment 8776402 [details]
Bug 1289510 - Bookmarks toolbar buttons have weird hover styling when using a lightweight theme. .

Approval Request Comment
[Feature/regressing bug #]: bug 734326.
[User impact if declined]:  Bookmarks toolbar buttons on hover have different styling when using a lightweight theme
[Describe test coverage new/current, TreeHerder]:Fixed hover styling.
[Risks and why]: minor changes. Styles for hover effect updated.
[String/UUID change made/needed]:no

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8776402 [details]
Bug 1289510 - Bookmarks toolbar buttons have weird hover styling when using a lightweight theme. .

Fixes a recent regression with hover styling, Aurora50+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd4b4df3eb2a

Comment 26

[Carmen]

I was able to reproduce both issues using Nightly 50.0a1 (2016-07-26) for Ubuntu 16.04 LTS x64 and Nightly 50.0a1 (2016-07-29) for Windows 10 Pro x64.

The fix is now verified using Firefox Beta 50.0b4 and latest Aurora 51.0a2 (2016-10-06), across same platforms.

Comment 27

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this bug using Fx 50.0a1, build ID:  20160726080520, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 50.0b4, build ID: 20161003155957 and Fx 51.0a2, build ID: 20161006004005, on Windows 10 x64, mac OS X 10.10.5 and Ubuntu 14.04 LTS. The hover effect has the intended styling.

Cheers!