Closed Bug 935508 Opened 6 years ago Closed 6 years ago

Implement native theming of <input type=number>

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 4 obsolete files)

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
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
Attached patch patch for mac (obsolete) — Splinter Review
Attachment #8337842 - Flags: review?(dholbert)
Attached patch patch for mac (obsolete) — Splinter Review
Attachment #8337842 - Attachment is obsolete: true
Attachment #8337842 - Flags: review?(dholbert)
Attachment #8337867 - Flags: review?(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.)
Attachment #8337867 - Flags: review?(dholbert) → feedback+
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?)
(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!
Attached patch patch for mac (obsolete) — Splinter Review
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.
Attachment #8337867 - Attachment is obsolete: true
Attachment #8338212 - Flags: review?(roc)
"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 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.
Attachment #8338212 - Flags: review?(roc) → review-
Attached patch themeSplinter Review
Attachment #8338212 - Attachment is obsolete: true
Attachment #8340999 - Flags: review?(roc)
(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.
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?
Flags: needinfo?(dholbert)
Clearing needinfo while I check whether a typo (":" vs. ";") in forms.css is responsible.
Flags: needinfo?(dholbert)
Attachment #8341542 - Attachment is obsolete: true
Attachment #8341552 - Flags: review?(roc)
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!
Attachment #8341552 - Flags: review?(roc) → review-
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()...
Attachment #8341552 - Flags: review- → review?(roc)
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.
Attachment #8341552 - Flags: review?(roc) → review+
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.
Attachment #8340999 - Flags: checkin-
Attachment #8341552 - Flags: checkin+
Those failures were nothing to do with this bug. Relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/45decd50b118
Blocks: 946184
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
https://hg.mozilla.org/mozilla-central/rev/45decd50b118
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 957562
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?
Flags: needinfo?(jwatt)
Oh, I meant to revert that - well spotted!
Flags: needinfo?(jwatt)
Patch in bug 949891.
Depends on: 966861
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.