370439 - Rewrite native scrollbars to use nsITheme

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Colin Barrett [:cbarrett]]

Our native scrollbar implementation currently creates a native scrollbar widget. This causes drawing errors in a number of draw operations such as clipping and opacity. We should be using nsITheme to draw scrollbars.

Comment 1

[Colin Barrett [:cbarrett]]

Attachment: fix v1.0

Known issues:
- Hit testing is not 100% correct, since we're not using the HITheme hit testing API
- Redrawing bugs. Trails are left, perhaps related to above?
- Small scrollbars in form widgets look strange (possibly related to bug 361523)
- Scrollbars not obey window focus (i.e. control does not dim when in the background). Appears to be a larger issue?

Known, but probably not in scope:
- When a user switches styles of scrollbar arrows, the theme draws the new style but the XBL still thinks the hit rects are in the old places. Re-opening the window solves this. toolkit/content/widgets/scrollbar.xml is where the arrow style is set, and it appears to be only done on init. Changing that is probably outside the scope of this bug.
- There is a way to get increment/decrement arrows on the top AND bottom of the scrollbar. It's not an easily selectable pref, it requires using a command line utility. Instead of drawing their chosen arrow style, we default to two arrows on the bottom. Again, changing this is probably outside of the scope of this bug.

Comment 2

[Colin Barrett [:cbarrett]]

> - Hit testing is not 100% correct, since we're not using the HITheme hit
testing API

Not to say it's not possible without having HITheme handle things. We'd just need some CSS with the right metrics.

Another out of scope issue is that the user's preference for a click on the track to mean page down or scroll to here is not respected. This is settable in Appearance.prefpane.

The out of scope issues should have separate bugs?

Comment 3

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

+        nsCOMPtr<nsIDOMElement> rootElement

Don't call it "rootElement", since that normally means document root. I'd just call it scrollbarElement.

+            nsCOMPtr<nsIDocument> doc = do_QueryInterface(xblDoc);

Can't you just store aFrame->GetContent()->GetCurrentDoc() where you use that above, and use it again here?

Seems to me that it would be sightly better to pass in all four button-pressed states to DrawScrollbar and have it figure out which flags to set.

What sort of redrawing bugs are we talking about?

Any progress on making the frame layout match the native scrollbar metrics?

Can you dim the scrollbars using HIThemeDrawTrack? You probably need to check the window focus state somehow and pass that information in. The tricky part may be redrawing the scrollbars when focus state changes...

Comment 4

[Colin Barrett [:cbarrett]]

(In reply to comment #3)
> +        nsCOMPtr<nsIDOMElement> rootElement
> 
> Don't call it "rootElement", since that normally means document root. I'd just
> call it scrollbarElement.

Sure.
 
> +            nsCOMPtr<nsIDocument> doc = do_QueryInterface(xblDoc);
> 
> Can't you just store aFrame->GetContent()->GetCurrentDoc() where you use that
> above, and use it again here?

Sure.

> Seems to me that it would be sightly better to pass in all four button-pressed
> states to DrawScrollbar and have it figure out which flags to set.

Sure. I'm thinking we should use GetAnonymousNodes directly to keep us from iterating four times.
 
> What sort of redrawing bugs are we talking about?

When dragging the thumb upwards, the track isn't getting repainted properly, so the bottom of the thumb is visible. Same sort of behavior for dragging the thumb downwards, but the top of the thumb gets left on the track.

> Any progress on making the frame layout match the native scrollbar metrics?

I'm unsure where to begin. 

The main problem is I don't know where or how the current frame layout is set. Hints here would be greatly appreciated.

The HITheme functions we would use give us the bounds for various parts of the control. They all take an HIThemeTrackDrawInfo parameter, so we would probably want to isolate the code where we take a frame and generate an HIThemeTrackDrawInfo.

> Can you dim the scrollbars using HIThemeDrawTrack? You probably need to check
> the window focus state somehow and pass that information in. The tricky part
> may be redrawing the scrollbars when focus state changes...

Yes, I believe. I think control dimming in the background may be out of scope for this -- it's a regression, yes, but there are a number of other controls (tabs, sliders, popup buttons) that should change state when in the background, but don't.

FWIW, the inIsDisabled param for DrawScrollbar (and DrawProgress) is probably useless. According to the docs, kThemeTrackDisabled contstant we are setting in both those functions only affects sliders. Looking at Interface Builder, there doesn't seem to be a way of disabling scrollbars or progress controls, so this is probably accurate.

We're also using the wrong constants for drawing disabled controls, according to the docs. kThemeStateUnavailable is the correct one for drawing a disabled control. kThemeStateInactive is an active control in a window that doesn't have focus, and kThemeStateUnavailableInactive is a disabled control in a window that doesn't have focus. I googled around for these constants and they seem to be in use according to the docs in the code I could find using them.

Comment 5

[Colin Barrett [:cbarrett]]

The relevant bug for background control dimming is bug 54488, FYI.

Comment 6

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

You should be able to set the part sizes by implementing nsITheme::GetMinimimWidgetSize to return the right sizes for the various parts.

We can skip the dimming issues for now.

I'm not really sure why we're not repainting the track when the thumb moves. Try setting gDumpPaintList=1 in the debugger, drag the thumb, and see what gets dumped in the console.

Comment 7

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

It's possible that the redraw problems are caused by parts not being the right size...

Comment 8

[Colin Barrett [:cbarrett]]

(In reply to comment #6)
> You should be able to set the part sizes by implementing
> nsITheme::GetMinimimWidgetSize to return the right sizes for the various parts.

I don't understand how the current code functions as well as it does -- the entire thumb is correctly hit tested. The parts of the scrollbar that are incorrectly hit tested are the arrows and the endcap (the top 10 or so pixels). This currently is counted as a click in the track, and it should have no effect.

> We can skip the dimming issues for now.

Sure. Although I definitely would like to work on it soon, since it is now a regression.

> I'm not really sure why we're not repainting the track when the thumb moves.
> Try setting gDumpPaintList=1 in the debugger, drag the thumb, and see what gets
> dumped in the console. 

(In reply to comment #7)
> It's possible that the redraw problems are caused by parts not being the right
> size...

It's strange. I'm only really seeing this when I drag the thumb and it's near the top of the track. It works fine on the lower half. Will post gDumpPaintList output.

Comment 9

[[e-mail]]

(In reply to comment #1)
>- There is a way to get increment/decrement arrows on the top AND bottom of the
>scrollbar. It's not an easily selectable pref, it requires using a command line
>utility. Instead of drawing their chosen arrow style, we default to two arrows
>on the bottom. Again, changing this is probably outside of the scope of this bug.
FYI bug 263444 enhanced nsILookAndFeel to make the arrows show independently.

Comment 10

[Colin Barrett [:cbarrett]]

(In reply to comment #9)
> (In reply to comment #1)
> FYI bug 263444 enhanced nsILookAndFeel to make the arrows show independently.

Thanks. I've added a comment to nsLookAndFeel.mm (will be in v2 of the patch) with some more information.

Comment 11

[Colin Barrett [:cbarrett]]

Attachment: fix v2.0

Here is fix v2.

Notes:
- I haven't been able to reproduce the drawing issues. Not sure what was causing them.
- The only real hit testing issue was with the top of the endcap on scrollbars with both arrows at the bottom. The others seem to have been my imagination.

Comment 12

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

+          // There is one other value, 2 (it doesn't have a constant). It enables both arrows on both ends. 
+          // If you google for "AppleScrollBarVariant DoubleBoth" you should find how to enable it.
           NS_WARNING("Not handling all possible ThemeScrollBarArrowStyle values");

Why not? you could just return eMetric_ScrollArrowStyleBothAtEachEnd.

+  nsIAtom *sbattrAtom = NS_NewAtom("sbattr");
+  nsIAtom *upTopAtom = NS_NewAtom("scrollbar-up-top");
+  nsIAtom *downTopAtom = NS_NewAtom("scrollbar-down-top");
+  nsIAtom *upBottomAtom = NS_NewAtom("scrollbar-up-bottom");
+  nsIAtom *downBottomAtom = NS_NewAtom("scrollbar-down-bottom");

These should be added to nsWidgetAtomList.

+  nsIContent::AttrValuesArray attributeValues[] = {
+    &upTopAtom,
+    &downTopAtom,
+    &upBottomAtom,
+    &downBottomAtom,
+    nsnull
+  };

This can be static.

I would use an array of four PRInt32s instead of four separate parameters to both of those helper functions. That would shrink the code in GetButtonStatesForScrollbarFrame too.

Would you mind using the "a*" convention for arguments?

+  if (((inUpTopButtonState & NS_EVENT_STATE_ACTIVE) && (inUpTopButtonState & NS_EVENT_STATE_HOVER)) ||

Define a static helper IsScrollbarButtonPressed(PRInt32 aState) {
  return (aState & (NS_EVENT_STATE_ACTIVE | NS_EVENT_STATE_HOVER)) ==
    NS_EVENT_STATE_ACTIVE | NS_EVENT_STATE_HOVER;
}
and use it a few times.

Why aren't you checking inDownTopButtonState?

Comment 13

[Colin Barrett [:cbarrett]]

(In reply to comment #12)
> +          // There is one other value, 2 (it doesn't have a constant). It
> enables both arrows on both ends. 
> +          // If you google for "AppleScrollBarVariant DoubleBoth" you should
> find how to enable it.
>            NS_WARNING("Not handling all possible ThemeScrollBarArrowStyle
> values");
> 
> Why not? you could just return eMetric_ScrollArrowStyleBothAtEachEnd.

I was unsure of expanding the scope of this patch to include this. For some reason, I thought that I would need to touch scrollbar.xml, but reading the code again it doesn't look like I have to. I'll fix this in the next version.

> +  nsIAtom *sbattrAtom = NS_NewAtom("sbattr");
> +  nsIAtom *upTopAtom = NS_NewAtom("scrollbar-up-top");
> +  nsIAtom *downTopAtom = NS_NewAtom("scrollbar-down-top");
> +  nsIAtom *upBottomAtom = NS_NewAtom("scrollbar-up-bottom");
> +  nsIAtom *downBottomAtom = NS_NewAtom("scrollbar-down-bottom");
> 
> These should be added to nsWidgetAtomList.
>
> +  nsIContent::AttrValuesArray attributeValues[] = {
> +    &upTopAtom,
> +    &downTopAtom,
> +    &upBottomAtom,
> +    &downBottomAtom,
> +    nsnull
> +  };
> 
> This can be static.

Alright.

> I would use an array of four PRInt32s instead of four separate parameters to
> both of those helper functions. That would shrink the code in
> GetButtonStatesForScrollbarFrame too.

Alright -- I wasn't sure if that was acceptable, stylewise.

> Would you mind using the "a*" convention for arguments?

In which functions? I used the in* convention to conform with the other functions in the file.
 
> +  if (((inUpTopButtonState & NS_EVENT_STATE_ACTIVE) && (inUpTopButtonState &
> NS_EVENT_STATE_HOVER)) ||
> 
> Define a static helper IsScrollbarButtonPressed(PRInt32 aState) {
>   return (aState & (NS_EVENT_STATE_ACTIVE | NS_EVENT_STATE_HOVER)) ==
>     NS_EVENT_STATE_ACTIVE | NS_EVENT_STATE_HOVER;
> }
> and use it a few times.

Good idea.

> Why aren't you checking inDownTopButtonState?

This patch, as of yet, doesn't support scrollbars in state eMetric_ScrollArrowStyleBothAtEachEnd. I'll update it to do this.

Comment 14

[Colin Barrett [:cbarrett]]

(In reply to comment #13)
> (In reply to comment #12)
> > Would you mind using the "a*" convention for arguments?
> 
> In which functions? I used the in* convention to conform with the other
> functions in the file.

I should be clear: some of the functions are using in* and some are using a*. Other helper functions are using in*, so I went with that.

Comment 15

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

I'd err on the side of a* since that's what most Mozilla code uses.

Comment 16

[Colin Barrett [:cbarrett]]

Attachment: Work in progress fix v3.0

here's a WIP fix with some debugging code to make it easier to see the issue with the thumb sizing mismatch, currently the only bug I'm aware of.

Comment 17

[Colin Barrett [:cbarrett]]

Attachment: fix v4

After over a month since the last review, here's another version. I'm fairly sure that I've fixed the thumb sizing issue with this patch. I've also done a bit of refactoring as a result.

I have left in the visual debugging code, but it's wrapped in a preprocessor macro. Set SCROLLBARS_VISUAL_DEBUG to non-zero to enable it. 

One issue that's left is what I think is a rounding bug in gfx-related code. It might be related to the XXX note in DrawWidgetBackground. Symptoms: In some cases, small scrollbars in content are getting *drawn* one pixel above where they should be. I've been able to reproduce it on http://bugzilla.mozilla.org/query.cgi. It's not on every one, only some. I suspect some kind of rounding error.

The other issue is that with this patch, scrollbars no longer draw in the background state (i.e. the thumb becomes gray, instead of blue) when the window is not key. See bug 54488, which might need to be nominated as a blocker when this lands, given it's a pretty noticeable UI regression.

When testing this patch, be on the look-out for trails off the thumb when scrolling. If you see them, try building with the visual debug define enabled. I used Pixie.app (comes with the Mac OS X developer tools) to zoom in and count pixels in cases like that.

Comment 18

[philippe (part-time)]

I tried to build Camino trunk with this patch, but ended in a build error:
checkout finish: Sun Apr 8 10:46:23 JST 2007
....

/Users/phiw/camino-build/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm: In member function 'void nsNativeThemeCocoa::GetScrollbarDrawInfo(HIThemeTrackDrawInfo&, nsIFrame*, const HIRect&, PRBool)':
/Users/phiw/camino-build/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm:358: error: 'kThemeScrollBarSmall' was not declared in this scope
/Users/phiw/camino-build/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm:358: error: 'kThemeScrollBarMedium' was not declared in this scope
make[6]: *** [nsNativeThemeCocoa.o] Error 1

Comment 19

[Colin Barrett [:cbarrett]]

What version of Mac OS X are you on?

Comment 20

[philippe (part-time)]

(In reply to comment #19)
> What version of Mac OS X are you on?
> 

10.4.9 ppc, building with GCC 4.01+MacOS10.3.9SDK

Comment 21

[Colin Barrett [:cbarrett]]

Attachment: fix v4.1

Removed modern constant names, so it should work on 10.3.9, now.

Comment 22

[philippe (part-time)]

Ok, v4.1 builds now.
But I can't seem to use the scrollbar at all (I've build Camino with the patch). Looking at this textarea the scroll thumb is locked at the top of the window.
Maybe it is a Camino issue ? I'll make a Minefield build later to check.

Comment 23

[Colin Barrett [:cbarrett]]

What about the scrollbars on the content area itself? Do those work?

Comment 24

[philippe (part-time)]

(In reply to comment #23)
Testing with the Cc list box at the top of the page here, I get the same problem:
click on the arrows: no scrolling.
click/hold the thumb an drag: no movement.
Scrolling a long page like this with keyboard or scrollwheel does of course work.
If I switch tabs, and come back to this page, the thumb on the main scrollbar has jumped down to the bottom of the window, where it should be, as the focus is on the textarea.
 

Comment 25

[philippe (part-time)]

Ok, the issues in my previous comments are apparently a Camino only problem.
With a patched Minefield, I can't reproduce those (both builds use an identical .mozconfig).
The only issue I've seen in Minefield is the background state that you mentioned in comment 17. No trails of the thumb, so far, and all the dupes on bug 187435 work correctly (Yay !).
Also fixed are the remaining issues in bug 343830.

Comment 26

[[e-mail]]

Comment on attachment 260949 [details] [diff] [review]
fix v4.1

> scrollbar {
>   -moz-appearance: scrollbar;
>-  -moz-binding: url(chrome://global/content/bindings/nativescrollbar.xml#scrollbar);
>-  min-width: 16px;
>+  -moz-binding: url(chrome://global/content/bindings/scrollbar.xml#scrollbar);
Nit: this is the default binding for a scrollbar so you don't need to repeat it.

Comment 27

[Colin Barrett [:cbarrett]]

Changed locally, will be in the next version of the patch. Thanks.

Comment 28

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

+  PRBool isVertical = !(aFrame->GetContent()->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::orient, 
+                                                          nsWidgetAtoms::horizontal, eCaseMatters));

Make this isHorizontal to reduce the number of negations in the code.

+      if (IsScrollbarButtonPressed(buttonStates[0]))
+        aTdi.trackInfo.scrollbar.pressState |= kThemeTopOutsideArrowPressed;

These could be simplified using a helper like
  static UInt8 ConvertToPressState(PRInt32 aButtonState, UInt8 aPressState) {
    return IsScrollbarButtonPressed(aButtonState) ? aPressState : 0;
  }

then you can write
  aTdi.trackInfo.scrollbar.pressState =
    ConvertToPressState(buttonStates[0], kThemeTopOutsideArrowPressed) |
    ConvertToPressState(buttonStates[1], kThemeTopInsideArrowPressed) |
etc

+      if (IsScrollbarButtonPressed(buttonStates[0]) || IsScrollbarButtonPressed(buttonStates[2]))

This could use buttonStates[0] | buttonStates[2].

Otherwise, looks good.

Comment 29

[Josh Aas]

Attachment: fix v4.2

This fixes Camino. This does not include any changes requested in this bug since v4.1. Colin - you'll need to use this patch as a baseline and re-do your local changes and then address roc's comments.

If you don't want to pull a new tree, you only need to add the changes to "themes/classic/global/mac/nativescrollbars.css" to your tree, the rest of the patch is the same as v4.1.

Comment 30

[philippe (part-time)]

(In reply to comment #29)
> Created an attachment (id=261090) [details]
> fix v4.2
> 
> This fixes Camino. This does not include any changes requested in this bug
> since v4.1. Colin - you'll need to use this patch as a baseline and re-do your
> local changes and then address roc's comments.

Thanks Josh, the scrollbars now work in Camino.

One (small) issue: that patch removes the additional Camino only ruleblocks in forms.css.
Most of what was left is probably more or less obsolete by now. 
One block is still needed though:
/* make sure nothing paints for the menulist button, since the button is
  painted as part of the menulist. */
select > input[type="button"],
select > input[type="button"]:active {
  background: transparent !important;
  -moz-appearance: none !important;
  border: none !important;
}
This was added in bug 350973 to remove the additional dropmarker on select widgets.

Comment 31

[Stuart Morgan]

The entire layout/style/forms.css change snuck in by accident; that wasn't intended to be part of the patch.

Comment 32

[Josh Aas]

Yes - Colin - do not include the forms.css part of that patch, I didn't mean to get that in the diff!

Comment 33

[Josh Aas]

Attachment: fix v4.3

This includes the Camino fix, addresses my offline review comments, and addresses roc's comments.

As for Neil's comment #26, that change breaks this patch according to Colin so it isn't in this patch.

Comment 34

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

Comment on attachment 261196 [details] [diff] [review]
fix v4.3

looks good ... except for all those extra blank line additions. Don't add them.

Comment 35

[Josh Aas]

Fixed on trunk. I left the spaces in because in those files we put two lines between functions.

We might want test cases for this like we do for pinstripe buttons.

Comment 36

[Stuart Morgan]

Have follow-ups been filed for the regressions this patch introduces that were mentioned in comments 1 & 2 (especially 2)?

Comment 37

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

Attachment: patch to fix reftest failure

This caused a reftest failure on Mac. Actually it exposed an existing bug in the call to GetAbsPosClipRect from FinishAndStoreOverflow. This patch fixes that bug.

Comment 38

[Colin Barrett [:cbarrett]]

The reftest failure for 372037 seems to be related to drawing into canvas. I loaded the testcases manually and it doesn't look any different to me.

Comment 39

[[e-mail]]

(In reply to comment #33)
>As for Neil's comment #26, that change breaks this patch according to Colin so
>it isn't in this patch.
Sorry about that, scrollbars are the exception to the rule...