Horizontal and vertical overlay scrollbars should overlap on 10.8

VERIFIED FIXED in Firefox 23

Status

()

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mstange, Assigned: areinald.bug)

Tracking

Trunk
mozilla24
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox21 unaffected, firefox22 unaffected, firefox23 verified, firefox24 verified, firefox25 verified, firefox-esr17 unaffected)

Details

(Whiteboard: [lion-scrollbars=])

Attachments

(7 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
In other words, part 3 of bug 636564 should be deactivated on 10.8.
Created attachment 750408 [details]
What Chrome does.

Markus, what do you mean by "they should overlap"? It sounds to me like you are talking about the bottom-right invisible square that the scrollbars don't pass. However even on 10.8 that is the proper behavior. See what Chrome does in the screenshot I added.
Flags: needinfo?(mstange)
Oh, I take it back. :) Looks like Chrome is not demonstrating proper behavior either. Textedit does indeed overlap the scrollbars. I never noticed that before.
Flags: needinfo?(mstange)
Created attachment 750410 [details]
What Textedit does.
Created attachment 750411 [details]
What Safari does.

Interesting. Safari however, also doesn't overlap scrollbars. Therefore I am at a lost if we should or should not overlap ours.

I would say we shouldn't, to be consistent with Safari, but both are acceptable options.
(Reporter)

Comment 5

5 years ago
Oh, interesting, I never bothered to check other browsers ;-)

I'd say Webkit just hasn't adopted the 10.8 behavior yet, but that's just a guess.

The only time when I notice this is when both scrollbars are visible and one is hovered. The 10.8 rectangle hover effect looks ridiculous if the scrollbar stops short of the viewport end.
I agree it looks odd. Although having the scrollbars overlap doesn't look terribly pleasing either. I suppose this should be up to UX then...
(Reporter)

Comment 7

5 years ago
Agreed.
Keywords: uiwanted
Neither appearance really looks ideal but overlapping is probably what we want since it is more consistent with the rest of the OS.

Comment 9

5 years ago
Agreed: a wide gutter stopping short of the edge looks a whole lot worse than overlapping indicators.

As a side note in case this gets implemented, Apple's behavior for hovering over the the bottom-right corner is to widen the vertical scrollbar.
Whiteboard: [lion-scrollbars=]
I'm late to the party, but wanted to add that part 3 of bug 636564 was deliberately landed at the time because it matched what Safari was doing. I figured a follow up bug could answer the question whether or not we should have this behavior. So, thanks for filing this bug! :-)
(Assignee)

Updated

5 years ago
Assignee: nobody → areinald
Status: NEW → ASSIGNED
(Assignee)

Comment 11

5 years ago
Created attachment 752883 [details] [diff] [review]
If value of useOverlayScrollbars is 2, then overlap H and V scrollbars in the corner

Next step is to set this value depending on the OS version.
Attachment #752883 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 752883 [details] [diff] [review]
If value of useOverlayScrollbars is 2, then overlap H and V scrollbars in the corner

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +3733,5 @@
>      }
>      AdjustScrollbarRectForResizer(mOuter, presContext, hRect, hasResizer, false);
>    }
>  
> +  if (LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars) != 2) {

I'd like to know from Steven if we should be using an enum (or similar) here, instead of a hard-coded value of 2. Maybe a comment explaining this would be enough?
Attachment #752883 - Flags: review?(spohl.mozilla.bugs) → review?(smichaud)
Comment on attachment 752883 [details] [diff] [review]
If value of useOverlayScrollbars is 2, then overlap H and V scrollbars in the corner

This is incomplete by itself -- currently LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars) can't return '2' (only '1' or '0').  So it's not really reviewable by itself.
Attachment #752883 - Flags: review?(smichaud)
(Assignee)

Comment 14

5 years ago
Created attachment 752972 [details] [diff] [review]
Allow overlay scrollbars to overlap on 10.8 or later

Relying on useOverlayScrollbars : 0 for old style, 1 for overlay not overlap, 2 for overlay and overlap.
Attachment #752883 - Attachment is obsolete: true
Attachment #752972 - Flags: review?(smichaud)
Comment on attachment 752972 [details] [diff] [review]
Allow overlay scrollbars to overlap on 10.8 or later

Actually I should have r-ed the previous patch, too.

On the face of it, we're redefining LookAndFeel::eIntID_UseOverlayScrollbars, which can be used across platforms, to deal with a problem that (as far as I know) only arises on OS X.  Let me see if I can come up with a better approach.
Attachment #752972 - Flags: review?(smichaud) → review-
I really think we should use a separate setting for this -- something like eIntID_AllowOverlayScrollbarOverlap.

Windows Metro is (currently) the only other platform that needs to use overlay scrollbars, and as far as we know their horizontal and vertical scrollbars never overlap.  So currently we only have this choice (whether or not to allow the overlay scrollbars to overlap) on OS X (and specifically on Mountain Lion).

But the question of whether or not to use overlay scrollbars is logically separate from the question of their "style".  Currently the only "style" question we ask is whether or not we should allow the scrollbars to overlap.  But we may have more "style" options in the future -- in which case we'd change the name of the eIntID_AllowOverlayScrollbarOverlap setting and add more possible values.
(Assignee)

Comment 17

5 years ago
Created attachment 753408 [details] [diff] [review]
Allow overlay scrollbars to overlap on 10.8 or later

As per smichaud advice, add a separate selector to hold the desired overlap value.
Attachment #752972 - Attachment is obsolete: true
Attachment #753408 - Flags: review?(smichaud)
Comment on attachment 753408 [details] [diff] [review]
Allow overlay scrollbars to overlap on 10.8 or later

Looks good to me.
Attachment #753408 - Flags: review?(smichaud) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Reporter)

Comment 19

5 years ago
Why did you make the change to nsLookAndFeel::UseOverlayScrollbars()? Personally, I prefer the previous style.
(Assignee)

Comment 20

5 years ago
I would prefer too if we were to return a bool. But there was an implicit conversion from bool to int when we put the result in aResult, and so I changed the return type of UseOverlayScrollbars(), hence its code too.

For returning ints, I'd rather use something like return (a ? b : c) but not sure people like it here.
(Assignee)

Comment 21

5 years ago
But I might change the whole patch as I notice that in the combined patch for bug 636564 there are many more places where the addition of the useOverlayScrollbars setting is reflected (search for "overlay" into the patch). I don't understand why, but maybe I should just mimic what has been done around the "overlay".
(Reporter)

Comment 22

5 years ago
(In reply to André Reinald from comment #20)
> I would prefer too if we were to return a bool. But there was an implicit
> conversion from bool to int when we put the result in aResult

But then I'd do the explicit conversion when setting aResult, and not in the method. That's what I was going to do for a different system metric once, but I was asked to keep the implicit conversion, see bug 448767 comment 4.

(In reply to André Reinald from comment #21)
> But I might change the whole patch as I notice that in the combined patch
> for bug 636564 there are many more places where the addition of the
> useOverlayScrollbars setting is reflected (search for "overlay" into the
> patch). I don't understand why, but maybe I should just mimic what has been
> done around the "overlay".

I think this was changed in bug 868396.
(Assignee)

Comment 23

5 years ago
The other reason why I left it as int was the first version of my patch returned 0 1 or 2 to account for the normal / overlay / overlay + overlap looks.

Ok, then I'll change back the method result type, and make the explicit conversion when setting aResult with the a ? 1 : 0
(Assignee)

Comment 24

5 years ago
Created attachment 753482 [details] [diff] [review]
Allow overlay scrollbars to overlap on 10.8 or later

Take into account mstange comments.
Attachment #753408 - Attachment is obsolete: true
Attachment #753482 - Flags: review?(smichaud)
Comment on attachment 753482 [details] [diff] [review]
Allow overlay scrollbars to overlap on 10.8 or later

This also looks good to me.
Attachment #753482 - Flags: review?(smichaud) → review+
(Reporter)

Comment 26

5 years ago
Thanks!
Stephen Horlander agreed with this approach in comment #8.
Keywords: uiwanted
https://hg.mozilla.org/mozilla-central/rev/12178e8e457b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
status-firefox21: --- → unaffected
status-firefox22: --- → unaffected
status-firefox23: --- → affected
status-firefox24: --- → fixed
status-firefox-esr17: --- → unaffected

Updated

5 years ago
Depends on: 877857

Comment 30

5 years ago
I filed issue 878480 before I saw this report. As I understand now, the overlapping is expected.

Please feel free to close issue 878480. although the overlapping looks strange ...
Comment on attachment 753482 [details] [diff] [review]
Allow overlay scrollbars to overlap on 10.8 or later

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 636564
User impact if declined: Overlay scrollbars wouldn't overlap and wouldn't have a native OSX lion feel.
Testing completed (on m-c, etc.): Has been on m-c for several days. I've also confirmed that this is fixed in recent nightlies.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: none
Attachment #753482 - Flags: approval-mozilla-aurora?
Attachment #753482 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Requesting checkin for aurora.
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/a9028805825f
status-firefox23: affected → fixed
Keywords: checkin-needed
Blocks: 880753
Please provide detail steps to reproduce this.
Flags: needinfo?(mstange)
STR:
1. Resize browser window until both vertical and horizontal scrollbars are displayed.
2. Scroll to the bottom right corner.

Expected:
Scrollbars overlap.
Flags: needinfo?(mstange)
The attachment from comment 3 shows the expected behavior.
Created attachment 774881 [details]
Firefox 24 Aurora with unexpected result

User Agent :Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20130712 Firefox/24.0
Build ID: 20130712004003 (latest Aurora)

Tested it using steps provided in comment 35 on MAC 10.8 OS, but I don't see desired result.

User Agent :Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 
Firefox/23.0
Build ID: 20130711122148 ( FX 23 B5)

Tested it using steps provided in comment 35 on MAC 10.8 OS, but I don't see desired result.
Created attachment 774883 [details]
Firefox 23 B5 with unexpected result

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
:Samvedana, your setup uses the old-style scrollbars. This usually happens for one of two reasons. To rule them out, please verify the following:

1. Make sure that you don't have an external mouse connected to your system.
2. Make sure that under System Preferences > General, you have either "Automatically based on mouse or trackpad" or "When scrolling" selected for "Show scroll bars"
Flags: needinfo?(samvedana.gohil)
And :Samvedana, since you're verifying other overlay scrollbar-related bugs as well, please make sure that you always verify them with overlay scrollbars enabled (not the old-style scrollbars).
Keywords: verifyme
QA Contact: samvedana.gohil
Created attachment 778153 [details]
scrollbar

User Agent : Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0
Build ID: 20130715155216

User Agent : Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20130718 Firefox/25.0
Build ID: 20130718030201

User Agent : Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20130718 Firefox/24.0
Build ID:  20130718004004

Tested this on latest nightly. I am getting attached (scrollbar)result for all above builds. 

Is this okay? I am confused with overlay scrollbar.
Flags: needinfo?(samvedana.gohil) → needinfo?(mstange)

Updated

5 years ago
Flags: needinfo?(spohl.mozilla.bugs)
Yes, that's the expected behavior.
Flags: needinfo?(spohl.mozilla.bugs)

Updated

5 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
status-firefox23: fixed → verified
status-firefox24: fixed → verified
status-firefox25: --- → verified
Resolution: --- → WORKSFORME

Updated

5 years ago
Keywords: verifyme
Status: RESOLVED → VERIFIED
Flags: needinfo?(mstange)
Resolution: WORKSFORME → FIXED
You need to log in before you can comment on or make changes to this bug.