Closed Bug 904561 Opened 6 years ago Closed 6 years ago

[10.7] Scrollbars shouldn't grow on hover on OSX 10.7 (only on 10.8 and up)

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
firefox23 --- unaffected
firefox24 + verified
firefox25 + verified
firefox26 + verified
firefox-esr17 --- unaffected

People

(Reporter: bogdan_maris, Assigned: spohl)

References

Details

(Whiteboard: [lion-scrollbars+])

Attachments

(3 files, 1 obsolete file)

Reproducible on the Firefox 24 Beta 2 (BuildID: 20130812173056):
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20100101 Firefox/24.0
Reproducible on the latest Aurora (BuildID: 20130812004004):
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:25.0) Gecko/20100101 Firefox/25.0
Reproducible on the latest Nightly (BuildID: 20130812030209): 
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:26.0) Gecko/20100101 Firefox/26.0

Steps to reproduce:
1. Start Firefox.
2. Open a page where you can scroll.
3. Scroll using the mouse.
4. Move the cursor to the scrollbar when it`s displayed.

Expected results: The scrollbar is properly displayed.

Actual results: The scrollbar moves to the left.
Bogdan, are you referring to the fact that the scrollbar is 'growing', i.e. becoming wider when hovered? Is what you're seeing different from Safari? Screenshots would be appreciated. Thanks!
Flags: needinfo?(bogdan.maris)
I posted a screencast that will show the issue. It will need Quicktime software to play it. Note that this behavior is specific only for Firefox. Chrome, Safari and Opera does not show this issue.
Flags: needinfo?(bogdan.maris)
I notice from Bogdan's video that the scrollbar does actually move (left a few pixels), but only some of the time.
Oh, right, on 10.7 scrollbars didn't actually grow! We need to limit the size change to 10.8 and above.
Blocks: 636564
Summary: [10.7] Scrollbar moves when focused → [10.7] Scrollbars shouldn't grow on hover on OSX 10.7 (only on 10.8 and up)
Whiteboard: [lion-scrollbars+]
Assigning to myself. Need to install 10.7 to verify my patch and check against native apps.
Assignee: nobody → spohl.mozilla.bugs
Duplicate of this bug: 906848
Markus, I've been trying to figure out how you've determined the parameters that are passed to CUIDraw for our scrollbars when hovered. I can successfully set conditional breakpoints in gdb that are hit when the @"widget" key in the dictionary has value @"kCUIWidgetOverlayScrollBar". However, I'm unable to figure out how to make the breakpoint conditional on the mouse hovering over the scrollbar. I was thinking I could write gdb breakpoint command lists to print out the contents of the NSDictionary in $edx and continue without breaking (essentially a form of printf inside CUIDraw, rather than a breakpoint). But I've been unable to figure out how to set local variables in command lists (maybe not supported?). As my next step, I was going to use an interpose library to print the contents of the NSDictionary, but I wanted to check with you if there's an easier way. You must have figured out somehow that @"state" had value of @"rollover" on OSX 10.8 when the scrollbar is hovered, and I'm basically curious to hear how you've figured that out.
Flags: needinfo?(mstange)
(In reply to Stephen Pohl [:spohl] from comment #7)
> However, I'm unable to figure out how to make the breakpoint conditional on
> the mouse hovering over the scrollbar.

I didn't do anything that sophisticated. I just kept doing "c" and "po $rdx" until I hit the call for scrollbar drawing.

It's easier to hit it if you keep the gdb window focused and only enable the breakpoint if you've hovered the scrollbar in the window you're targeting (which stays in the background). Like this, for example: http://tests.themasta.com/gdbcuidrawoverlayscrollbar.webm
Flags: needinfo?(mstange)
Attached patch Patch (obsolete) — Splinter Review
Attachment #793589 - Flags: review?(smichaud)
Status: NEW → ASSIGNED
Comment on attachment 793589 [details] [diff] [review]
Patch

This basically looks fine to me, though I haven't tested it.

Are we using just one style for scrollbars (one appropriate for the hovered state on 10.8 and up), and adjusting it when it isn't hovered (or we're on 10.7 and below)?

+        if (!nsCocoaFeatures::OnMountainLionOrLater()) {
+          if (isHorizontal) {
+            macRect.origin.y += 2.0;
+          } else {
+            macRect.origin.x += 3.0;
+          }
+        }

Where do these numbers come from?  Trial and error?
Attachment #793589 - Flags: review?(smichaud) → review+
(In reply to Steven Michaud from comment #10)
> Are we using just one style for scrollbars (one appropriate for the hovered
> state on 10.8 and up), and adjusting it when it isn't hovered (or we're on
> 10.7 and below)?

That's correct. We wouldn't get here on OSX below version 10.7 though, because nsLookAndFeel::UseOverlayScrollbars() would already be false (since the features is for 10.7+).

> 
> +        if (!nsCocoaFeatures::OnMountainLionOrLater()) {
> +          if (isHorizontal) {
> +            macRect.origin.y += 2.0;
> +          } else {
> +            macRect.origin.x += 3.0;
> +          }
> +        }
> 
> Where do these numbers come from?  Trial and error?

Yes, trial and error. :-(
Attached patch PatchSplinter Review
Added comment that numbers were obtained by trial and error, as requested on IRC. Carrying over r+.
Attachment #793589 - Attachment is obsolete: true
Attachment #793637 - Flags: review+
Forgot about RTL before.
Attachment #793653 - Flags: review?(smichaud)
Attachment #793653 - Flags: review?(smichaud) → review+
https://hg.mozilla.org/mozilla-central/rev/58d1b62f613c
https://hg.mozilla.org/mozilla-central/rev/67775ddf0d15
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment on attachment 793637 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 636564
User impact if declined: Overlay scrollbars on OSX 10.7 would grow when hovered, which doesn't match the native behavior.
Testing completed (on m-c, etc.): Has been on m-c for ~1 day. Local testing shows that the issue is fixed.
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none
Attachment #793637 - Flags: approval-mozilla-beta?
Attachment #793637 - Flags: approval-mozilla-aurora?
Comment on attachment 793653 [details] [diff] [review]
Followup for RTL support

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 636564
User impact if declined: Overlay scrollbars on OSX 10.7 in RTL mode would display incorrectly when hovered.
Testing completed (on m-c, etc.): Has been on m-c for ~1 day. Local testing shows that the issue is fixed.
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none
Attachment #793653 - Flags: approval-mozilla-beta?
Attachment #793653 - Flags: approval-mozilla-aurora?
Attachment #793637 - Flags: approval-mozilla-beta?
Attachment #793637 - Flags: approval-mozilla-beta+
Attachment #793637 - Flags: approval-mozilla-aurora?
Attachment #793637 - Flags: approval-mozilla-aurora+
Attachment #793653 - Flags: approval-mozilla-beta?
Attachment #793653 - Flags: approval-mozilla-beta+
Attachment #793653 - Flags: approval-mozilla-aurora?
Attachment #793653 - Flags: approval-mozilla-aurora+
Keywords: verifyme
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:25.0) Gecko/20100101 Firefox/25.0

Verified as fixed on Firefox 24 beta 5 (buildID: 20130822154523) and latest Aurora (buildID: 20130823004003).
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:25.0) Gecko/20100101 Firefox/25.0

Verified as fixed with latest Aurora 25.0a2 (Build ID: 20130909004001).
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed on latest Aurora 26.0a2 (buildID: 20131009004001).
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.