873012 - Horizontal and vertical overlay scrollbars should overlap on 10.8

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

In other words, part 3 of bug 636564 should be deactivated on 10.8.

Comment 1

[Josiah Bruner [:jsbruner]]

Attachment: 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.

Comment 2

[Josiah Bruner [:jsbruner]]

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.

Comment 3

[Josiah Bruner [:jsbruner]]

Attachment: What Textedit does.

Comment 4

[Josiah Bruner [:jsbruner]]

Attachment: 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.

Comment 5

[Markus Stange [:mstange]]

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.

Comment 6

[Josiah Bruner [:jsbruner]]

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...

Comment 7

[Markus Stange [:mstange]]

Agreed.

Comment 8

[Stephen Horlander [:shorlander]]

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

[Jason Goodman]

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.

Comment 10

[Stephen A Pohl [:spohl]]

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! :-)

Comment 11

[André Reinald]

Attachment: 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.

Comment 12

[Stephen A Pohl [:spohl]]

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?

Comment 13

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 14

[André Reinald]

Attachment: 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.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 17

[André Reinald]

Attachment: Allow overlay scrollbars to overlap on 10.8 or later

As per smichaud advice, add a separate selector to hold the desired overlap value.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 753408 [details] [diff] [review]
Allow overlay scrollbars to overlap on 10.8 or later

Looks good to me.

Comment 19

[Markus Stange [:mstange]]

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

Comment 20

[André Reinald]

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.

Comment 21

[André Reinald]

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".

Comment 22

[Markus Stange [:mstange]]

(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.

Comment 23

[André Reinald]

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

Comment 24

[André Reinald]

Attachment: Allow overlay scrollbars to overlap on 10.8 or later

Take into account mstange comments.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 753482 [details] [diff] [review]
Allow overlay scrollbars to overlap on 10.8 or later

This also looks good to me.

Comment 26

[Markus Stange [:mstange]]

Thanks!

Comment 27

[Steven Michaud [:smichaud] (Retired)]

Stephen Horlander agreed with this approach in comment #8.

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12178e8e457b

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/12178e8e457b

Comment 30

[Mehmet]

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 31

[Stephen A Pohl [:spohl]]

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

Comment 32

[Stephen A Pohl [:spohl]]

Requesting checkin for aurora.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a9028805825f

Comment 34

[Samvedana (:Samvedana)]

Please provide detail steps to reproduce this.

Comment 35

[Stephen A Pohl [:spohl]]

STR:
1. Resize browser window until both vertical and horizontal scrollbars are displayed.
2. Scroll to the bottom right corner.

Expected:
Scrollbars overlap.

Comment 36

[Stephen A Pohl [:spohl]]

The attachment from comment 3 shows the expected behavior.

Comment 37

[Samvedana (:Samvedana)]

Attachment: 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.

Comment 38

[Samvedana (:Samvedana)]

Attachment: Firefox 23 B5 with unexpected result

Comment 39

[Stephen A Pohl [:spohl]]

: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"

Comment 40

[Stephen A Pohl [:spohl]]

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).

Comment 41

[Samvedana (:Samvedana)]

Attachment: 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.

Comment 42

[Stephen A Pohl [:spohl]]

Yes, that's the expected behavior.