274036 - [Mac] Scrollbars should look disabled when there's nowhere to scroll

Status: VERIFIED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Uri Bernstein (Google)]

Bug 271974 was fixed for Windows (and Linux?). However, the issue still exists
on Mac OS X: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a6)
Gecko/20041208 Firefox/1.0+

Attachment 159832 [details] (attached to bug 76197) is a good testcase. Also the bugzilla
query form (https://bugzilla.mozilla.org/query.cgi) - the "status",
"resolution", "severity" and "priority" listboxes shouldn't have an active
scrollbar.

Notice that this issue is much more annoying on Mac than it was on Windows, due
to the fact that the aqua scrollbars are more visually prominent, and that they
can actually be scrolled (a bit) in this situation, producing no results.

I wasn't sure in what component to put this, so if someone has a better idea
than Firefox/General, please move it.

Comment 1

[[e-mail]]

Sounds like we need to add a disabled attribute to nsINativeScrollbar.idl and
corresponding methods in nsNativeScrollbarFrame.cpp and nsNativeScrollbar.cpp
(and also QT's nsScrollbar.cpp). I can fix the first two but obviously I have no
idea what the other two files will need.

Comment 2

[[e-mail]]

Attachment: layout and widget changes only

Comment 3

[[e-mail]]

Comment on attachment 168506 [details] [diff] [review]
layout and widget changes only

This request is just to verify the correctness (or otherwise) of my approach.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 168506 [details] [diff] [review]
layout and widget changes only

looks alright...

Comment 5

[Mike Pinkerton (not reading bugmail)]

is there  a separate bug filed for the actual work on the scrollbar to notice
the attribute, or should that be done as a part of this one?

Comment 6

[[e-mail]]

Bug 76197 actually set the attribute, and covered the work to update the Modern
theme's CSS to notice the attribute; the Classic theme would previously already
have noticed the attribute using -moz-appearance native theme code. This bug
exists to cover work needed for native scrollbars to notice the attribute.

Comment 7

[Mike Pinkerton (not reading bugmail)]

okies

Comment 8

[Uri Bernstein (Google)]

requesting blocking-aviary1.1.

Comment 9

[Simon Fraser [no longer active]]

What's the status of this bug?

Comment 10

[[e-mail]]

(In reply to comment #9)
>What's the status of this bug?
Someone needs to finish my patch by fixing nsNativeScrollbar.cpp

Comment 11

[Simon Fraser [no longer active]]

Attachment: Testcase

Comment 12

[Simon Fraser [no longer active]]

I think this should be fixed in the native scrollbar code. Disabling the
scrollbar when there is nowhere to scroll seems like a platformy behavior that
might differ depending on skin or platform.

Comment 13

[Simon Fraser [no longer active]]

Attachment: Fix carbon and cocoa scrollbars

This patch removes some arbitrary max and view size values that were being used
if the view size or max range was 0 (which were there since the first rev of
the file). It also fixes cocoa scrollbars to automatically disable when not
scrollable; carbon scrollbars seem to do the right thing on their own.

Comment 14

[Simon Fraser [no longer active]]

Comment on attachment 186693 [details] [diff] [review]
Fix carbon and cocoa scrollbars

This (and the layout code that calls it) needs a little more work to deal with
the "too small" scrollbar case.

Comment 15

[Simon Fraser [no longer active]]

Attachment: Testcase (with "too small" case)

Comment 16

[Simon Fraser [no longer active]]

Attachment: Revised patch

Actually that last patch was mostly good, other than a possible divide by zero,
fixed in this patch. I verified that scrollbars with this patch behave just as
they should for small sizes.

Comment 17

[[e-mail]]

The issue with just watching maxpos is that it won't work if someone writes
<xul:scrollbar disabled="true">, which IIRC worked in Windows and Linux Classic
for some time and was more recently fixed for Modern too.

Comment 18

[Simon Fraser [no longer active]]

(In reply to comment #17)
> The issue with just watching maxpos is that it won't work if someone writes
> <xul:scrollbar disabled="true">, which IIRC worked in Windows and Linux Classic
> for some time and was more recently fixed for Modern too.

Doesn't that translate into a SetEnabled() on the native scrollbar? My patch
makes that work for cocoa scrollbars, and it looks like the carbon scrollbar
already handles enabled state.

Can you attach a testcase that demonstrates the problem?

Comment 19

[[e-mail]]

Attachment: XUL scrollbar test case

For an HTML version of the test case, use this URL:
data:text/html,%3Ctextarea%20rows=5%20cols=8%20style="overflow:scroll"%3E123456789


(In reply to comment #18)
>Doesn't that translate into a SetEnabled() on the native scrollbar?
http://lxr.mozilla.org/mozilla/ident?i=SetEnabled
didn't turn up anything relevant. Who is supposed to call what?

Comment 20

[Simon Fraser [no longer active]]

(In reply to comment #19)
> (In reply to comment #18)
> >Doesn't that translate into a SetEnabled() on the native scrollbar?
> http://lxr.mozilla.org/mozilla/ident?i=SetEnabled
> didn't turn up anything relevant. Who is supposed to call what?

Sorry, the method is nsIWidget::Enable(PRBool).

Comment 21

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 186702 [details] [diff] [review]
Revised patch

r=pink

Comment 22

[Simon Fraser [no longer active]]

(In reply to comment #19)
> Created an attachment (id=186755) [edit]
> XUL scrollbar test case

With my patches, all 3 scrollbars look disabled (in Camino).

> For an HTML version of the test case, use this URL:
> data:text/html,%3Ctextarea%20rows=5%20cols=8%20style="overflow:scroll"%3E123456789

With my patches, the vertical scrollbar is disabled, and the horizontal
scrollbar is enabled).

Comment 23

[[e-mail]]

(In reply to comment #20)
>(In reply to comment #19)
>>(In reply to comment #18)
>>>Doesn't that translate into a SetEnabled() on the native scrollbar?
>>http://lxr.mozilla.org/mozilla/ident?i=SetEnabled
>>didn't turn up anything relevant. Who is supposed to call what?
>Sorry, the method is nsIWidget::Enable(PRBool).
I guess lxr doesn't like .mm files, so I still couldn't determine the caller.

(In reply to comment #22)
>(In reply to comment #19)
>>Created an attachment (id=186755)
>>XUL scrollbar test case
>With my patches, all 3 scrollbars look disabled (in Camino).
For comparison, the Suite only disables the middle scrollbar, as it a) defaults
the maxpos to 100 and b) doesn't disable explicit scrollbars with a maxpos of 0
- both the CSS and the native theme code only work from the disabled attribute.

Comment 24

[Simon Fraser [no longer active]]

(In reply to comment #23)
> (In reply to comment #22)
> >(In reply to comment #19)
> >>Created an attachment (id=186755) [edit]
> >>XUL scrollbar test case
> >With my patches, all 3 scrollbars look disabled (in Camino).
> For comparison, the Suite only disables the middle scrollbar, as it
> a) defaults the maxpos to 100 and

I don't see this 100 being passed onto the native scrollbar code. All 3
scrollbars in the test are given a view size of 0, and a maxpos of 0.

> b) doesn't disable explicit scrollbars with a maxpos of 0

Right, but I think this is a toolkit (i.e. OS) behavior, not necessarily a suite
behavior.

> - both the CSS and the native theme code only work from the disabled attribute.

Can you imagine any situation in which it would be incorrect to disable a
scrollbar that is inherently unscrollble?

Comment 25

[Karsten Düsterloh]

Suite trunk debug build of 2005-06-21 +1:00.

FWIW: with patch 186702 in, all scrollbars in attachment 159832 [details] are disabled.
But the XUL testcase 186755 still shows three active scrollbars... The data/HTML
testcase of comment 19, though, has an enabled (needed) horizontal scrollbar and
a disabled vertical one.

Comment 26

[Simon Fraser [no longer active]]

Attachment: Revised patch, fixes carbon scrollbar disabling

This patch differs from the last in that, for Carbon scrollbars, it disables
the scrollbar when there is nowhere to scroll, and it ensures that
nsNativeScrollbar doesn't inherit the defult mMax=1 from its base class (thus
making the thumb disappear correctly). With this patch, carbon and cocoa
scrollbars behave identically.

Comment 27

[Karsten Düsterloh]

All three testcases look good now (suite trunk build under Tiger).

Comment 28

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 186896 [details] [diff] [review]
Revised patch, fixes carbon scrollbar disabling

	virtual void		GetRectForMacControl(nsRect &outRect);
+  virtual ControlPartCode GetControlHiliteState();
+

whee. tabs & spaces. Should detab this file when landing.

r=pink

Comment 29

[Simon Fraser [no longer active]]

Seeking 1.8b3 approval. This is a safe fix that addresses a mac scrollbar
usability issue.

Comment 30

[chris hofmann]

Comment on attachment 186896 [details] [diff] [review]
Revised patch, fixes carbon scrollbar disabling

a=chofmann

Comment 31

[Simon Fraser [no longer active]]

Checked in.

Comment 32

[Uri Bernstein (Google)]

Verifed fixed with
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050628
Firefox/1.0+

Thanks, everybody!