Closed Bug 274036 Opened 17 years ago Closed 16 years ago

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

Categories

(Core Graveyard :: Widget: Mac, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: uriber, Assigned: sfraser_bugs)

References

()

Details

Attachments

(3 files, 4 obsolete files)

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.
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.
Assignee: firefox → sfraser_bugs
Component: General → Widget: Mac
Product: Firefox → Core
QA Contact: firefox.general
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.
Attachment #168506 - Flags: superreview?(roc)
Attachment #168506 - Flags: review-
Comment on attachment 168506 [details] [diff] [review]
layout and widget changes only

looks alright...
Attachment #168506 - Flags: superreview?(roc) → superreview+
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?
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.
requesting blocking-aviary1.1.
Flags: blocking-aviary1.1?
Assignee: sfraser_bugs → joshmoz
QA Contact: mac
What's the status of this bug?
(In reply to comment #9)
>What's the status of this bug?
Someone needs to finish my patch by fixing nsNativeScrollbar.cpp
Attached file Testcase (obsolete) —
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.
Assignee: joshmoz → sfraser_bugs
Attached patch Fix carbon and cocoa scrollbars (obsolete) — Splinter Review
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.
Attachment #168506 - Attachment is obsolete: true
Attachment #186693 - Flags: superreview?(bryner)
Attachment #186693 - Flags: review?(pinkerton)
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.
Attachment #186693 - Attachment is obsolete: true
Attachment #186693 - Flags: superreview?(bryner)
Attachment #186693 - Flags: review?(pinkerton)
Attachment #186692 - Attachment is obsolete: true
Attached patch Revised patch (obsolete) — Splinter Review
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.
Attachment #186702 - Flags: superreview?(bryner)
Attachment #186702 - Flags: review?(pinkerton)
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.
(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?
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?
(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 on attachment 186702 [details] [diff] [review]
Revised patch

r=pink
Attachment #186702 - Flags: review?(pinkerton) → review+
(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).
Status: NEW → ASSIGNED
(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.
(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?
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.
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.
Attachment #186702 - Attachment is obsolete: true
Attachment #186896 - Flags: superreview?(bryner)
Attachment #186896 - Flags: review?(pinkerton)
All three testcases look good now (suite trunk build under Tiger).
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
Attachment #186896 - Flags: review?(pinkerton) → review+
Attachment #186702 - Flags: superreview?(bryner)
Attachment #186896 - Flags: superreview?(bryner) → superreview+
Seeking 1.8b3 approval. This is a safe fix that addresses a mac scrollbar
usability issue.
Flags: blocking1.8b3?
Attachment #186896 - Flags: approval1.8b3?
Comment on attachment 186896 [details] [diff] [review]
Revised patch, fixes carbon scrollbar disabling

a=chofmann
Attachment #186896 - Flags: approval1.8b3? → approval1.8b3+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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!
Status: RESOLVED → VERIFIED
Flags: blocking-aviary1.1?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.