846883 - Add support for native theming of <input type=range> on OS X

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Jonathan Watt [:jwatt]]

We need to add support for native theming of <input type=range> on OS X.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: WIP patch

This mostly works now, but there are still some issues to work out with regards to positioning the thumb correctly when it is dragged by mouse/touch events. (The native widget keeps its thumb well inside the bounds that unthemed range uses, so I need to figure out how to obtain the min/max position from the theme to map the pointer correctly.)

There are also some invalidation issues due to the way that range currently relies on reflow of the thumb div, which is ignored when native theming is on. I'll fix and land another way of doing that (separately from this bug) before landing anything from this bug though.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

With this patch we still get the CSS focus ring (undesirable because the Mac theme shows its own focus highlight) but I'll fix that in a separate bug.

Comment 3

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

Comment on attachment 725501 [details] [diff] [review]
patch

Review of attachment 725501 [details] [diff] [review]:
-----------------------------------------------------------------

What's the plan for other platforms? They might affect what we want to do here.

::: gfx/src/nsThemeConstants.h
@@ +198,5 @@
>  #define NS_THEME_SCALE_TICK                               117
>  
> +// HTML's <input type=range>
> +#define NS_THEME_RANGE                                    120
> +#define NS_THEME_RANGE_THUMB                              121

Be more specific about which frames should have these appearance values.

::: layout/forms/nsRangeFrame.cpp
@@ +125,5 @@
>                                 const nsDisplayListSet& aLists)
>  {
> +  DisplayBorderBackgroundOutline(aBuilder, aLists);
> +  // We want all events to target us, not our children, so we don't create
> +  // display list items for our childner during event delivery. We also don't

children

@@ +127,5 @@
> +  DisplayBorderBackgroundOutline(aBuilder, aLists);
> +  // We want all events to target us, not our children, so we don't create
> +  // display list items for our childner during event delivery. We also don't
> +  // create them if we are being themed, since the theme draws the entire
> +  // slider control.

I'm not sure what this comment pertains to. You can have nsITheme::WidgetIsContainer return false to automatically suppress rendering of any children.

@@ +350,5 @@
> +    nsRefPtr<gfxContext> notUsedGfxCtx =
> +      new gfxContext(gfxPlatform::GetPlatform()->ScreenReferenceSurface());
> +    nsRenderingContext notUsedCtx;
> +    notUsedCtx.Init(PresContext()->DeviceContext(), notUsedGfxCtx);
> +    bool notUsedCanOverride;

Use nsIPresShell::GetReferenceRenderingContext

@@ +357,5 @@
> +      GetMinimumWidgetSize(&notUsedCtx, this, NS_THEME_RANGE_THUMB, &size,
> +                           &notUsedCanOverride);
> +    thumbSize.width = presContext->DevPixelsToAppUnits(size.width);
> +    thumbSize.height = presContext->DevPixelsToAppUnits(size.height);
> +    MOZ_ASSERT(thumbSize.width > 0 && thumbSize.height > 0);

Should the thumbFrame have the NS_THEME_RANGE_THUMB appearance? I think it probably should.

::: widget/cocoa/nsNativeThemeCocoa.mm
@@ +2289,5 @@
> +      bool isVertical = !IsRangeHorizontal(aFrame);
> +      DrawScale(cgContext, macRect, eventState, isVertical, reverseDir,
> +                value, min, max, aFrame);
> +    }
> +      break;

Put the break before the }

::: widget/xpwidgets/nsNativeTheme.cpp
@@ +306,5 @@
>    }
>  
> +  /**
> +   * Range appearance should be the same for the nsRangeFrame and its children.
> +   * nsRangeFrame owns the logic and will tell us what we should do.

I don't understand the first sentence here.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> What's the plan for other platforms? They might affect what we want to do
> here.

They're pretty similar. No major changes as far as I know yet.

> Should the thumbFrame have the NS_THEME_RANGE_THUMB appearance? I think it
> probably should.

It does, in the following patches for GTK (followed by a patch for Windows), but since it's not used here I separated that out as being logically separate.

> Put the break before the }

I'm following the existing convention in that function, but sure.

> > +   * Range appearance should be the same for the nsRangeFrame and its children.
> > +   * nsRangeFrame owns the logic and will tell us what we should do.
> 
> I don't understand the first sentence here.

Yeah, I guess that doesn't make sense until the anonymous children are given their widget types in the GTK patches. Moved it out.

Comment 5

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

Comment on attachment 725647 [details] [diff] [review]
patch

Review of attachment 725647 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsRangeFrame.cpp
@@ +331,5 @@
>    nsRect rangeContentRect = GetContentRectRelativeToSelf();
>    nsSize thumbSize;
> +  
> +  if (!IsThemed()) {
> +    nsIFrame* thumbFrame = mThumbDiv->GetPrimaryFrame();

Swap branches so this is if (IsThemed())

Comment 6

[Jonathan Watt [:jwatt]]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4c1d68f2d9f

Comment 7

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #2)
> With this patch we still get the CSS focus ring (undesirable because the Mac
> theme shows its own focus highlight) but I'll fix that in a separate bug.

Filed bug 851777.

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d4c1d68f2d9f