Closed Bug 1283637 Opened 3 years ago Closed 3 years ago

[10.12] Overlay scrollbar thumbs are wide / fat even when the scrollbar is not hovered

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- verified
firefox50 --- verified

People

(Reporter: mstange, Assigned: mstange)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

They should be thin, until hovered.
I'll need to test this some more, but I think the idea is right.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Comment on attachment 8766980 [details]
Bug 1283637 - Don't specify the 'state' key in the CoreUI options for overlay scrollbars unless the scrollbar is actually hovered.

https://reviewboard.mozilla.org/r/61688/#review59522

Just one question below, making sure that there wasn't an oversight during the refactor of RenderWithCoreUILegacy into RenderWithCoreUI.

::: widget/cocoa/nsNativeThemeCocoa.mm:2751
(Diff revision 1)
>            }
>            macRect.size.width -= 4;
>          }
>        }
>        const BOOL isOnTopOfDarkBackground = IsDarkBackground(aFrame);
> -      // Scrollbar thumbs have a too high minimum width when rendered through
> +      NSMutableDictionary* options = [NSMutableDictionary dictionaryWithObjectsAndKeys:

Is it no longer the case that scrollbar thumbs have "a too high minimum width when rendered through NSAppearance on 10.10"? Is this due to the change to |GetWidgetBorder|?
Attachment #8766980 - Flags: review?(spohl.mozilla.bugs) → review+
There never was a "too high minimum width" - what was happening was that all scrollbars were rendering as hovered if rendered through NSAppearance, because the "state" key was present in the dict. Now that we no longer set the "state" key for non-hovered scrollbars, we correctly draw them at their thin size even through NSAppearance.
Ah, makes sense. Thanks!
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb5c1b4fc5d
Don't specify the 'state' key in the CoreUI options for overlay scrollbars unless the scrollbar is actually hovered. r=spohl
https://hg.mozilla.org/mozilla-central/rev/3bb5c1b4fc5d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
No longer depends on: 1285532
Comment on attachment 8766980 [details]
Bug 1283637 - Don't specify the 'state' key in the CoreUI options for overlay scrollbars unless the scrollbar is actually hovered.

Approval Request Comment
[Feature/regressing bug #]: macOS 10.12 Sierra
[User impact if declined]: Scrollbars will look fat
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low; just small change to how we ask the system to render scrollbars + tweaking of internal spacing
[String/UUID change made/needed]: none
Attachment #8766980 - Flags: approval-mozilla-aurora?
Comment on attachment 8766980 [details]
Bug 1283637 - Don't specify the 'state' key in the CoreUI options for overlay scrollbars unless the scrollbar is actually hovered.

Improve 10.12 support, taking it.
Attachment #8766980 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing.
Flags: needinfo?(mstange)
Attached patch patch for betaSplinter Review
Flags: needinfo?(mstange)
Comment on attachment 8777484 [details] [diff] [review]
patch for beta

Approval Request Comment: See comment 8. This was approved for aurora but failed to land before the merge to beta.
Attachment #8777484 - Flags: approval-mozilla-beta?
Comment on attachment 8777484 [details] [diff] [review]
patch for beta

Review of attachment 8777484 [details] [diff] [review]:
-----------------------------------------------------------------

Per comment #9, improve 10.12 support, take it in 49 beta.
Attachment #8777484 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:48.0) Gecko/20100101 Firefox/48.0

I have reproduced this issue on MacOS Sierra 10.12 Beta 8 (16A313a) using Firefox DevEdition 48.0a2 (id: 20160530004020).

This is VERIFIED FIXED on the latest Nightly (id: 20160831030224) and on the latest Beta (id: 20160829102229) builds.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.