Closed Bug 873012 Opened 9 years ago Closed 9 years ago

Horizontal and vertical overlay scrollbars should overlap on 10.8

Categories

(Core :: Layout, defect)

All
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox21 --- unaffected
firefox22 --- unaffected
firefox23 --- verified
firefox24 --- verified
firefox25 --- verified
firefox-esr17 --- unaffected

People

(Reporter: mstange, Assigned: areinald.bug)

References

Details

(Whiteboard: [lion-scrollbars=])

Attachments

(7 files, 3 obsolete files)

In other words, part 3 of bug 636564 should be deactivated on 10.8.
Attached image 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)
Attached image 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.
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...
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.
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: nobody → areinald
Status: NEW → ASSIGNED
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)
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.
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+
Keywords: checkin-needed
Why did you make the change to nsLookAndFeel::UseOverlayScrollbars()? Personally, I prefer the previous style.
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.
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".
(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.
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
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+
Thanks!
Stephen Horlander agreed with this approach in comment #8.
Keywords: uiwanted
https://hg.mozilla.org/mozilla-central/rev/12178e8e457b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 877857
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
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.
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.
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
Attached image 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)
Flags: needinfo?(spohl.mozilla.bugs)
Yes, that's the expected behavior.
Flags: needinfo?(spohl.mozilla.bugs)
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → WORKSFORME
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.