935508 - Implement native theming of <input type=number>

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Watt [:jwatt]]

We'll need to implement native theming of <input type=number>.

Note to self:

NS_THEME_SPINNER
NS_THEME_SPINNER_UP_BUTTON
NS_THEME_SPINNER_DOWN_BUTTON
NS_THEME_SPINNER_TEXTFIELD

Comment 1

[Jonathan Watt [:jwatt]]

Fennec's handling for bringing up the number pad when a user selects an <input type=number> is here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoInputConnection.java?rev=8f4b134f50fc#573

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch for mac

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch for mac

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8337867 [details] [diff] [review]
patch for mac

>+++ b/content/html/content/src/HTMLInputElement.cpp
>+          bool oldNumberControlSpinTimerSpinsUpValue =
>+                 mNumberControlSpinTimerSpinsUp;
>           switch (numberControlFrame->GetSpinButtonForPointerEvent(
>                     aVisitor.mEvent->AsMouseEvent())) {
>           case nsNumberControlFrame::eSpinButtonUp:
>             mNumberControlSpinTimerSpinsUp = true;

This "mNumberControlSpinTimerSpinsUp" variable doesn't seem to exist in m-c:
http://mxr.mozilla.org/mozilla-central/search?string=mNumberControlSpinTimerSpinsUp

Same for StartNumberControlSpinButtonTimer, StopNumberControlSpinButtonTimer in the contextual code of your patch lower down.

Presumably these are all added in another patch in your patch-stack? This patch would be meaningful with knowledge of that patch.

>+    if (aVisitor.mEvent->message == NS_FOCUS_CONTENT ||
>+        aVisitor.mEvent->message == NS_BLUR_CONTENT) {
>+      // If the frame is native themed then the whole thing needs to be
>+      // repainted, not just the nested text field:
>+      nsIFrame* frame = GetPrimaryFrame();
>+      if (frame) {
>+        frame->InvalidateFrame();
>+      }

(I don't understand the "not just the nested textfield" part of the comment here - I don't see where (in the contextual code) we would otherwise be invalidating just the textfield. Maybe that'd be clearer with knowledge of the other patch mentioned above, though)

Also, should this be "if (frame && frame->IsThemed())", if this really only applies when we're using native theming? (or are you talking about subcomponents being themed here? In which case maybe we should be using a function like SpinnerStateChanged which has an internal IsThemed() check on a subcomponent)

>+++ b/widget/xpwidgets/nsNativeTheme.cpp
>@@ -69,16 +70,25 @@ nsNativeTheme::GetContentState(nsIFrame*
>   if (frameContent->IsElement()) {
>     flags = frameContent->AsElement()->State();
>+
>+    // <input type=number> needs special handling since its nested native
>+    // anonymous <input type=text> takes focus for it.
>+    if (aWidgetType == NS_THEME_TEXTFIELD && frameContent->IsHTML()) {
>+      nsNumberControlFrame *numberControlFrame = do_QueryFrame(aFrame);
>+      if (numberControlFrame && numberControlFrame->IsFocused()) {
>+        flags |= NS_EVENT_STATE_FOCUS;
>+      }
>+    }

do_QueryFrame is a bit expensive -- maybe s/isHTML()/isHTML(nsGkAtoms::input)/ to ensure we only bother taking this code-path for <input> elements (?)

Anyway, I'll punt on the rest of this to roc, since (per IRC) I don't really know how native theming is supposed to work (and what I know now is just from what jwatt explained to me in IRC), so I wouldn't feel comfortable r+'ing this. (I'm not explicitly requesting review from him yet, in case you want to address any of the above before requesting review from him.)

Comment 5

[Daniel Holbert [:dholbert]]

Note also that the forms.css tweak in this patch breaks <input type="number">, for people on non-mac platforms who've enabled the pref.

It'd be nice to structure the per-platform patches here in such a way that that the control doesn't break at intermediate points in the patch queue, if possible. (maybe not a huge deal if that's non-trivial, though... Maybe if the windows and linux native-theming patches are ordered first, before the forms.css tweak, then every intermediate point would be OK?)

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> This "mNumberControlSpinTimerSpinsUp" variable doesn't seem to exist in m-c:
> 
> Same for StartNumberControlSpinButtonTimer, StopNumberControlSpinButtonTimer
> in the contextual code of your patch lower down.
> 
> Presumably these are all added in another patch in your patch-stack? This
> patch would be meaningful with knowledge of that patch.

Yes. Bug 935506, bug 935501 and bug 943047.

That said this should be reviewable if I provide more context in the diff. I'll do that in the updated patch.

> (I don't understand the "not just the nested textfield" part of the comment
> here - I don't see where (in the contextual code) we would otherwise be
> invalidating just the textfield. Maybe that'd be clearer with knowledge of
> the other patch mentioned above, though)

Updated the comment to make it clearer.

> Also, should this be "if (frame && frame->IsThemed())"

Yes. Updated.

> do_QueryFrame is a bit expensive -- maybe
> s/isHTML()/isHTML(nsGkAtoms::input)

Done.

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Note also that the forms.css tweak in this patch breaks <input
> type="number">, for people on non-mac platforms who've enabled the pref.
> 
> It'd be nice to structure the per-platform patches here in such a way that
> that the control doesn't break at intermediate points in the patch queue, if
> possible. (maybe not a huge deal if that's non-trivial, though... Maybe if
> the windows and linux native-theming patches are ordered first, before the
> forms.css tweak, then every intermediate point would be OK?)

Reordering would be a pain, and this is all behind a pref so I don't think it matters too much. I can maybe land them all at once though.

Thanks, Daniel!

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch for mac

Given lots of context to this diff which hopefully should be enough to review despite the patches in the three bugs mentioned in the previous comment not having landed yet.

Comment 8

[Tim Nguyen :ntim]

"If you want to apply your own style to the buttons, set |-moz-appearance:none| on ::-moz-number-spin-box, and then |display:block| on ::-moz-number-spin-up and ::-moz-number-spin-down."

This makes styling quite complicated, can't those 2 be applicated automatically when the pseudo element is styled ?

Comment 9

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

Comment on attachment 8338212 [details] [diff] [review]
patch for mac

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

::: layout/style/forms.css
@@ +932,5 @@
> + * on ::-moz-number-spin-up and ::-moz-number-spin-down. The value "block" is
> + * recommended because flex containers coerce their children to be
> + * block-flavored (or wrap them in anonymous blocks, in some cases). "   since that avoids the buttons getting wrapped in an anonymous
> + * flex item that will prevent the setting of the "flex" property on these
> + * pseudo-elements from working.

These seems very hard for authors to use. Do we really need to support custom styling of the buttons?

::: widget/cocoa/nsNativeThemeCocoa.mm
@@ +2164,5 @@
> +        // The spinner for <input type=number> is a grandchild. Check for the
> +        // nsNumberControlFrame grandparent to get some state:
> +        if (aFrame->GetParent() && aFrame->GetParent()->GetParent()) {
> +          numberControlFrame =
> +            do_QueryFrame(aFrame->GetParent()->GetParent());

I believe do_QueryFrame(nullptr) returns null, so you can drop the aFrame->GetParent()->GetParent() check in the if clause.

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: theme

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> These seems very hard for authors to use. Do we really need to support
> custom styling of the buttons?

Note that the pseudo-elements are marked as CSS_PSEUDO_ELEMENT_IS_CHROME_ONLY in nsCSSPseudoElementList.h, so this is not exposed to content right now.

I think we do want to support custom styling of the buttons at some point, since authors generally want to be able to completely style form controls to get them to fit in with the look and feel of their site. I implemented that ability, but disabled it for content in bug 930010 because there is currently no agreement on _how_ we should do it. The consensus was that we should wait for Web Components, see if that can be made to meet content authors' needs, and rethink at that point.

I've tidied up the comment in forms.css as requested on IRC.

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: patch using different approach (paint buttons using clipping)

Daniel, I'm having great difficulty figuring out why this patch isn't working. So far I've tracked it back to nsDisplayThemedBackground items not being created for the -moz-number-spin-up/down elements, which I've tracked back to being because reflow is giving them zero height. However I'm finding the flexbox reflow code pretty impenetrable when it comes to figuring out _why_ flex reflow is failing to give these element's frames the height that it should be giving them. Would you have any ideas here?

Comment 13

[Jonathan Watt [:jwatt]]

Clearing needinfo while I check whether a typo (":" vs. ";") in forms.css is responsible.

Comment 14

[Jonathan Watt [:jwatt]]

Attachment: patch using clipped painting approach

Comment 15

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

Comment on attachment 8341552 [details] [diff] [review]
patch using clipped painting approach

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

::: layout/forms/nsNumberControlFrame.cpp
@@ +331,5 @@
> +  // If aFrame is a spin button for an <input type=number> then we expect the
> +  // frame of its mContent's great-grandparent to be that input's frame. We
> +  // have to check for this via the content tree because we don't know whether
> +  // extra frames will be wrapped around any of the elements between aFrame and
> +  // the nsNumberControlFrame that we're looking for (e.g. flex wrappers).

But you're not checking for this via the content tree!

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 8341552 [details] [diff] [review]
patch using clipped painting approach

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> > +  // If aFrame is a spin button for an <input type=number> then we expect the
> > +  // frame of its mContent's great-grandparent to be that input's frame. We
> > +  // have to check for this via the content tree because we don't know whether
> > +  // extra frames will be wrapped around any of the elements between aFrame and
> > +  // the nsNumberControlFrame that we're looking for (e.g. flex wrappers).
> 
> But you're not checking for this via the content tree!

What do you mean?

  if (aFrame->GetContent()->GetParent() &&
      aFrame->GetContent()->GetParent()->GetParent() &&
      aFrame->GetContent()->GetParent()->GetParent()->GetParent()) {
    return do_QueryFrame(aFrame->GetContent()->GetParent()->
                           GetParent()->GetParent()->GetPrimaryFrame());
  }

All of those start with aFrame->GetContent()...

Comment 17

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

Comment on attachment 8341552 [details] [diff] [review]
patch using clipped painting approach

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

Sorry, I don't know what I was thinking.

::: layout/forms/nsNumberControlFrame.cpp
@@ +419,5 @@
>  
> +#define STYLES_DISABLING_NATIVE_THEMING \
> +  NS_AUTHOR_SPECIFIED_BACKGROUND | \
> +  NS_AUTHOR_SPECIFIED_PADDING | \
> +  NS_AUTHOR_SPECIFIED_BORDER

Make this a static const int local variable in ShouldUseNativeStyleForSpinner.

Comment 18

[Jonathan Watt [:jwatt]]

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

I made some style changes to implement some flexbox wizardry that Daniel came up with to stop the spin buttons affecting the height of the input. The goal here was to make sure that <input type=number> visually looks identical to <input type=text>, but with the addition of a couple of spin buttons. I think that's important since platform libraries and existing browser implementations of number spinners behave this way. Without the style changes I couldn't find a way to meet this goal.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f23fd773d7d1

https://tbpl.mozilla.org/php/getParsedLog.php?id=31507132&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=31506475&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=31506497&tree=Mozilla-Inbound

Comment 20

[Jonathan Watt [:jwatt]]

Those failures were nothing to do with this bug. Relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/45decd50b118

Comment 21

[Jonathan Watt [:jwatt]]

One small follow-up to put back the |height:100%| on ::-moz-number-wrapper in forms.css since we need that for vertical centering if content styles the input to increase its height.

https://hg.mozilla.org/integration/mozilla-inbound/rev/759dfb425bbb

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45decd50b118

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/759dfb425bbb

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8341552 [details] [diff] [review]
patch using clipped painting approach

>diff --git a/layout/forms/nsNumberControlFrame.h b/layout/forms/nsNumberControlFrame.h
>-  virtual bool IsLeaf() const MOZ_OVERRIDE { return true; }
>+  virtual bool IsLeaf() const MOZ_OVERRIDE { return false; }

jwatt: do you recall why (or if) this^^ IsLeaf change was necessary?

Comment 25

[Jonathan Watt [:jwatt]]

Oh, I meant to revert that - well spotted!

Comment 26

[Jonathan Watt [:jwatt]]

Patch in bug 949891.