Closed Bug 846883 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file, 2 obsolete files)

We need to add support for native theming of <input type=range> on OS X.
Attached patch WIP patch (obsolete) — Splinter Review
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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #720075 - Attachment is obsolete: true
Attachment #725501 - Flags: review?(roc)
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.
Attached patch patchSplinter Review
(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.
Attachment #725501 - Attachment is obsolete: true
Attachment #725501 - Flags: review?(roc)
Attachment #725647 - Flags: review?(roc)
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())
Attachment #725647 - Flags: review?(roc) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/d4c1d68f2d9f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 855999
You need to log in before you can comment on or make changes to this bug.