Closed Bug 1289510 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Theme, defect)

50 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 51
Tracking Status
firefox49 --- unaffected
firefox50 + verified
firefox51 + verified

People

(Reporter: dao, Assigned: rakhisharma)

References

Details

(Keywords: regression)

Attachments

(2 files)

[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.
Flags: needinfo?(Rakhish1994)
This is on Linux. I haven't tested this on Windows.
OS: Unspecified → Linux
Tracking 50+ for this regression. Adding ni on Florin to check Windows to see if it occurs there as well.
Flags: needinfo?(florin.mezei)
Flags: needinfo?(florin.mezei) → needinfo?(andrei.vaida)
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.
Flags: needinfo?(andrei.vaida)
OS: Linux → All
Hardware: Unspecified → All
Flags: needinfo?(Rakhish1994)
There's a typo in browser/themes/linux/browser.css. 'background-color: var(--toolbarbutton-hover-background);' needs to use background instead.
Assignee: nobody → Rakhish1994
Status: NEW → ASSIGNED
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.
Attachment #8776402 - Flags: review?(jaws) → review-
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/
Attachment #8776402 - Flags: review- → review?(jaws)
Attached image 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.
Attachment #8777084 - Flags: review-
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
Attachment #8776402 - Flags: review?(jaws)
(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?
Flags: needinfo?(jaws)
Tracking 51+.
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/
Attachment #8776402 - Attachment description: Bug 1289510 - Bookmarks toolbar buttons have weird hover styling when using a lightweight theme. ? → Bug 1289510 - Bookmarks toolbar buttons have weird hover styling when using a lightweight theme. .
Attachment #8776402 - Flags: review- → review?(jaws)
Flags: needinfo?(jaws)
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!
Attachment #8776402 - Flags: review?(jaws) → review+
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.
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
https://hg.mozilla.org/mozilla-central/rev/e61b48ca0d63
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Can/should we uplift this to 50 (seems it's also affected)?
Flags: needinfo?(Rakhish1994)
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
Flags: needinfo?(Rakhish1994)
Attachment #8776402 - Flags: approval-mozilla-aurora?
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+
Attachment #8776402 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1302086
Flags: qe-verify+
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.
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!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.