635240 - Implement the layout for <input type='number'>

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Mounir Lamouri (:mounir)]

Something like nsNumberControlFrame with a <input type='text'> and spin buttons inside.

Sadly, I wrote a patch for that 6 months ago but I lost it in a disk crash :(

Comment 1

[Mounir Lamouri (:mounir)]

I'm going to split all <input type=number> layout work as much as possible in different patches and bugs. This one will track them.

Comment 2

[Tim Nguyen :ntim]

The mozilla team should seriously rewrite xul to make it much more similar to HTML. XUL should share HTML elements instead of using their own. It'll be much easier for implementation then since you'll only need to implement the element for XUL to make it available for HTML. It'll make Gecko base much lighter and better.

Comment 3

[Tim Nguyen :ntim]

What you could do to implement the layout part is by adding this css :
input[type="number"] {
    -moz-binding:url(chrome://global/content/bindings/numberbox.xml#numberbox);
}
input[type="number"] * {vertical-align:top;}
Then make the spin buttons work, and maybe add a pseudo element for them.

The CSS code seem to be sufficient for XUL but not for HTML.

Comment 4

[Mounir Lamouri (:mounir)]

jwatt is going to take this.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: WIP

This is a work-in-progress patch. This works to a large extent, but only the anonymous content one level deep is created. Once that is resolved (working on that in bug 917386) then this mostly just requiring style tweaking to the various pseudo-elements, and some up/down arrows for the up/down anonymous divs for when we have -moz-appearance:none.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: WIP

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 8

[Daniel Holbert [:dholbert]]

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

>+++ b/layout/forms/moz.build
>@@ -29,16 +29,17 @@ CPP_SOURCES += [
>     'nsMeterFrame.cpp',
>     'nsProgressFrame.cpp',
>     'nsRangeFrame.cpp',
>     'nsSelectsAreaFrame.cpp',
>     'nsTextControlFrame.cpp',
>+    'nsNumberControlFrame.cpp',
> ]

moz.build file lists are supposed to be sorted; insert into the sorted position in this list (between nsMeterFrame and nsProgressFrame).

(I sorta thought the build system enforced this, but I guess not, if you can successfully build with this new file at the end.)

>diff --git a/layout/forms/nsNumberControlFrame.cpp b/layout/forms/nsNumberControlFrame.cpp
>+nsNumberControlFrame::DestroyFrom(nsIFrame* aDestructRoot)
>+{
>+  NS_ASSERTION(!GetPrevContinuation() && !GetNextContinuation(),
>+               "nsNumberControlFrame should not have continuations; if it does we "
>+               "need to call RegUnregAccessKey only for the first.");

Nit: drop the period at the end of the assertion-message.

(When the assertion fails, the assertion infra will already print ":" at the end, and ".:" looks gross.)

>+nsNumberControlFrame::Reflow(nsPresContext* aPresContext,
>+                             nsHTMLReflowMetrics& aDesiredSize,
>+                             const nsHTMLReflowState& aReflowState,
>+                             nsReflowStatus& aStatus)
>+{
[...]
>+  NS_ASSERTION(!mFrames.FirstChild()->GetNextSibling(),
>+               "We only expect one direct child frame");

This assertion needs to be tweaked so we don't crash while evaluating it, if mFrames.FirstChild() is null.  (which seems at least as likely as having too many child frames).

Depending on whether we're allowed to be childless, we either need to add "!mFrames.FirstChild() ||"  or "mFrames.FirstChild() &&" to the beginning of the condition.

(Should we be allowed to have 0 child frames here? It looks like that might be allowed, from the allowances for outerWrapperFrame to be null-from-display:none in several places. If that's allowed, we *definitely* need the assertion to be able to handle that situation without crashing. :) and we need test coverage for that situation.)

>+  nsresult rv =
>+    ReflowAnonymousContent(aPresContext, aDesiredSize, aReflowState);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsIFrame* outerWrapperFrame = mOuterWrapper->GetPrimaryFrame();
>+  if (outerWrapperFrame) {

The "outerWrapperFrame" querying here & in ReflowAnonymousContent seem a bit redundant.

Maybe we should just do the lookup here (in Reflow()), and pass it to ReflowAnonymousContent, and also only call ReflowAnonymousContent if it's non-null? (since ReflowAnonymousContent does nothing in the null case)

>+  nscoord computedHeight = aReflowState.ComputedHeight();
>+  if (computedHeight == NS_AUTOHEIGHT) {
>+    computedHeight =
>+      outerWrapperFrame ? outerWrapperFrame->GetSize().height : 0;
>+  }
>+  aDesiredSize.width = aReflowState.ComputedWidth() +
>+                         aReflowState.mComputedBorderPadding.LeftRight();
>+  aDesiredSize.height = computedHeight +
>+                          aReflowState.mComputedBorderPadding.TopBottom();

So "height:auto" just gives us 0 height, instead of shrinkwrapping the outerWrapperFrame's desired height? Seems like it might be worth doing that.

>+  // computation of the ascent wrt the input height
>+  nscoord lineHeight = computedHeight;
>+  float inflation = nsLayoutUtils::FontSizeInflationFor(this);
>+  nsRefPtr<nsFontMetrics> fontMet;
>+  rv = nsLayoutUtils::GetFontMetricsForFrame(this, getter_AddRefs(fontMet),
>+                                             inflation);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  // now adjust for our borders and padding
>+  aDesiredSize.ascent =
>+         nsLayoutUtils::GetCenteredFontBaseline(fontMet, lineHeight) +
>+           aReflowState.mComputedBorderPadding.top;

Fix super-indentation there.

Also, lineHeight appears to just be an alias for computedHeight. Not sure it's worth having two variables.

Also, why are we trying to independently compute a baseline here rather than just using the already-computed baseline of our outerWrapperFrame? (or its first child, maybe) (adjusted into our coordinate space, of course)  Might just need to have a 

>+nsresult
>+nsNumberControlFrame::
>+  ReflowAnonymousContent(nsPresContext* aPresContext,
>+                         nsHTMLReflowMetrics& aParentDesiredSize,
>+                         const nsHTMLReflowState& aParentReflowState)
>+{

nsHTMLReflowMetrics& aParentDesiredSize is unused - get rid of that parameter.

(though: while you're at it -- you might want to promote the local variable 'desiredSize' to be an outparam, so that our parent can shrinkwrap us and/or use our baseline, as suggested above.)

>+  nsresult rv = NS_OK;
>+
>+  nsIFrame* outerWrapperFrame = mOuterWrapper->GetPrimaryFrame();
>+
>+  if (outerWrapperFrame) { // display:none?

As noted above, I think we should just probably skip this method entirely if outerWrapperFrame is null.
(This means we can just declare 'nsresult rv' at the point that it's used, too, instead of up top.)

>+    // The width/height of our content box, which is the available width/height
>+    // for our anonymous content:
>+    nscoord inputFrameContentBoxWidth = aParentReflowState.ComputedWidth();
>+    nscoord inputFrameContentBoxHeight = aParentReflowState.ComputedHeight();
>+    if (inputFrameContentBoxHeight == NS_AUTOHEIGHT) {
>+      inputFrameContentBoxHeight = 0;
>+    }

inputFrameContentBoxHeight is unused (since we use NS_UNCONSTRAINED avail height). Drop it.

>+    nsHTMLReflowState wrapperReflowState(aPresContext, aParentReflowState,
>+                                         outerWrapperFrame,
>+                                         nsSize(inputFrameContentBoxWidth,
>+                                                NS_UNCONSTRAINEDSIZE));
[...]
>+    rv = ReflowChild(outerWrapperFrame, aPresContext, desiredSize,
>+                     wrapperReflowState, xoffset, yoffset, 0, status);

Assert that the status is NS_COMPLETE, since we gave unconstrained available height, like we do in nsRangeFrame here:
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsRangeFrame.cpp#361

>+nsresult
>+nsNumberControlFrame::SetFormProperty(nsIAtom* aName,
>+                                      const nsAString& aValue)
>+{
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsNumberControlFrame::GetFormProperty(nsIAtom* aName, nsAString& aValue) const
>+{
>+  return NS_OK;
>+}
>+
>+void
>+nsNumberControlFrame::SetFocus(bool aOn, bool aRepaint)
>+{
>+}

Why are these no-ops? (I don't know enough about these methods offhand to know)  Worth at least a comment justifying a no-op impl, if it's reasonable.

>+nsresult
>+nsNumberControlFrame::MakeAnonymousElement(nsIContent** aResult,
>+                                           nsTArray<ContentInfo>& aElements,
>+                                           nsIAtom* aTagName,
>+                                           nsCSSPseudoElements::Type aPseudoType)
>+{
>+  // Get the NodeInfoManager and tag necessary to create the anonymous divs.
>+  nsCOMPtr<nsIDocument> doc = mContent->GetDocument();
>+
>+  nsCOMPtr<nsINodeInfo> nodeInfo;
>+  nodeInfo = doc->NodeInfoManager()->GetNodeInfo(aTagName, nullptr,
>+                                                 kNameSpaceID_XHTML,
>+                                                 nsIDOMNode::ELEMENT_NODE);
>+  NS_ENSURE_TRUE(nodeInfo, NS_ERROR_OUT_OF_MEMORY);
>+  nsresult rv = NS_NewHTMLElement(aResult, nodeInfo.forget(),
>+                                  dom::NOT_FROM_PARSER);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsRefPtr<nsStyleContext> newStyleContext;
>+
>+  if (aPseudoType != nsCSSPseudoElements::ePseudo_NotPseudoElement) {
>+    // Associate the pseudo-element with the anonymous child
>+    newStyleContext =
>+      PresContext()->StyleSet()->ResolvePseudoElementStyle(mContent->AsElement(),
>+                                                           aPseudoType,
>+                                                           StyleContext());

s/StyleContext()/mStyleContext/ (unless there's a chance that they're different?)

(I know other code uses StyleContext() here, but I don't think (?) there's a good reason for that. Note that we use mContent instead of GetContent(), in the same function-call. Basically, there's no reason to use public-accessors internally.)

Maybe file a followup on fixing the other instances of this in similar code, to keep us consistent?

>+  if (!aElements.AppendElement(ContentInfo(*aResult, newStyleContext))) {
>+    return NS_ERROR_OUT_OF_MEMORY;

Technically nsTArray::AppendElement is infallible now, so this will never fail, but I guess it doesn't matter much.

>+nsresult
>+nsNumberControlFrame::CreateAnonymousContent(nsTArray<ContentInfo>& aElements)
>+{
>+  nsresult rv;
>+
>+  // We create an anonymous tree for our input element that is sturctured as

typo: "structured"

>+  // follows:
>+  //
>+  // input
>+  //   div      - outer wrapper display:inline-flex

s/display:inline-flex/with "display:inline-flex"/

Also: depending on how strongly we enforce this display type, do we really need the "display:none" null-checks for this wrapper mentioned elsewhere in this patch?
(If we *don't* enforce this display type, maybe add "...by default" at the end of this comment here, or something.)
(OK, I just found forms.css chunk - looks like we enforce it with 


>+  nsTArray<ContentInfo>& outerWrappersChildren =
>+    aElements.LastElement().mChildren;
>+
>+  // Create the ::-moz-number-text pseuto-element:

s/pseuto/pseudo/ (Happens several times)

>+void
>+nsNumberControlFrame::AppendAnonymousContentTo(nsBaseContentList& aElements,
>+                                               uint32_t aFilter)
>+{
>+  // Only one direct anonymous child:
>+  aElements.MaybeAppendElement(mOuterWrapper);
>+}
>+

Drop the bonus newline at the end of the file there.

>diff --git a/layout/forms/nsNumberControlFrame.h b/layout/forms/nsNumberControlFrame.h
>+
>+class nsNumberControlFrame MOZ_FINAL : public nsContainerFrame
>+                                     , public nsIFormControlFrame
>+                                     , public nsIAnonymousContentCreator
>+{

Add a brief header comment before the class decl, explaining what this class is for.

Also -- general comment -- this class definition needs a generous sprinkling of MOZ_OVERRIDE (on all the 'virtual' and NS_IMETHOD method decls).

>+  NS_DECL_QUERYFRAME_TARGET(nsNumberControlFrame)

Is this actually a queryframe target? (i.e. do you need to do_QueryFrame from nsIFrame* to nsNumberControlFrame anywhere?)

If not, I'm pretty sure you can drop this line.  If so, note that it won't actually work unless you add "NS_QUERYFRAME_ENTRY(nsNumberControlFrame)" to the queryframe block in the .cpp file. (currently there is no such line)

>+  nsNumberControlFrame(nsStyleContext* aContext);

Constructor should be private (and NS_NewNumberControlFrame should be declared as a friend).

>+  virtual bool IsFrameOfType(uint32_t aFlags) const
>+  {
>+    return nsContainerFrame::IsFrameOfType(aFlags &
>+      ~(nsIFrame::eReplaced | nsIFrame::eReplacedContainsBlock));
>+  }

Does eReplacedContainsBlock really make sense here? (Do we contain a block?)

(Do you know why we actually need this?  I guess we kinda return this all over the place for non-block-flavored things, so it's probably correct here, but I don't know why offhand.)

>+
>+#endif // nsNumberControlFrame_h__
>+

Drop bonus newline at the end of this file, too.

>+++ b/layout/reftests/forms/input/number/number-similar-to-text-unthemed.html
>@@ -0,0 +1,8 @@
>+<!DOCTYPE html>
>+<html>
>+  <body>
>+    <input type="number" style="-moz-appearance:none; width:200px;">
>+    <!-- div to cover spin box area -->

Copy this comment into the reference case, too (to explain its corresponding random abspos div)

>diff --git a/layout/reftests/forms/input/number/reftest.list b/layout/reftests/forms/input/number/reftest.list
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/forms/input/number/reftest.list
>@@ -0,0 +1,12 @@
>+
>+# sanity checks:

Drop blank line @ beginning of this file.

>+# not valid on Android/B2G where type=number looks like type=text
>+skip-if(Android||B2G) != not-other-type-unthemed-1.html not-other-type-unthemed-1a-notref.html
>+skip-if(Android||B2G) != not-other-type-unthemed-1.html not-other-type-unthemed-1b-notref.html

Is it intentional that type=number looks like type=text on those platforms? (If so, we should probably assert that it does indeed match, with another test that's android/B2G-only.)

Also: A lot of these tests have "-moz-appearance:none;" -- is that necessary? (and do we need tests without that?)

>diff --git a/layout/style/forms.css b/layout/style/forms.css
>+/* anonymous wrapper that is not exposed via a pseudo-element */
>+input[type=number] > div {

...but it is exposed via input[type=number] > div, yes? (can authors style it that way?)

It'd be worth including reftests to ensure that this is unstylable (assuming that's the intention), and also that the pseudo-elements *are* author-stylable.

>+  /* Prevent styling that would change the type of frame we construct. */
>+  display: inline-flex !important;

Worth including a comment somewhere explaining why we need "inline-flex" rather than "flex". (Or, if it's all the same, might as well use "flex" and save a few characters.)

>+input[type=number]::-moz-number-spin-box {
>+  display: flex;
>+  flex: 0 8px;
>+  flex-direction: column;

nit: swap the ordering of those last 2 lines.

(flex-direction is a property of a "display:flex" element, so it should be grouped with "display:flex".  On the other hand, "flex: 0 8px" is a property of a *child* of a display:flex element, so it has nothing to do with the fact that we happen to be "display:flex".)

>+input[type=number]::-moz-number-spin-up {
>+  display: flex;
[...]
>+input[type=number]::-moz-number-spin-down {
>+  display: flex;

I don't think these pseudo-elements have children, so they shouldn't be "display:flex". (It doesn't buy you anything.)

>+input[type="number"] > div > div > div:hover {
>+  /* give some indication of hover state for the up/down buttons */
>+  background-color: lightblue;
>+}

This div > div > div:hover selector is a bit hacky & dependent on the exact parent hierarchy. (and probably un-tested, so easy to regress if we do tweak the hierarchy)

I'd rather we select these elements with something more targeted, like:
  input[type=number]::-moz-number-spin-up:hover,
  input[type=number]::-moz-number-spin-down:hover {
if that works.

>diff --git a/layout/style/nsCSSPseudoElementList.h b/layout/style/nsCSSPseudoElementList.h
> // HTML5 Forms pseudo elements
>+CSS_PSEUDO_ELEMENT(mozNumberOuterWrapper, ":-moz-number-wrapper", 0)

This ^ pseudo-element is unused. (looks like it's left over from an earlier version of the patch).  (You use NotPseudoElement for the wrapper frame right now.)

Comment 9

[Daniel Holbert [:dholbert]]

Sorry for the two sentences that trailed off. :) Finishing them here:
> Might just need to have a

...have a "desiredSize" variable that we pass in to the child, whose baseline we can inspect.

> (OK, I just found forms.css chunk - looks like we enforce it with 

...enforce it with !important.) I guess it's worth gracefully handling the case where someone might set it to "display:none" in forms.css. (though I don't think (?) authors should be able to do that; worth verifying with a test.)

Comment 10

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> (Should we be allowed to have 0 child frames here? It looks like that might
> be allowed, from the allowances for outerWrapperFrame to be
> null-from-display:none in several places.

I don't see a reason to disallow authors from specifying display:none, and besides that it would be hard to do if we want authors to be able to change 'display' to other non-display-none types. So yes, we should allow 0 child frames IMO.

> So "height:auto" just gives us 0 height, instead of shrinkwrapping the
> outerWrapperFrame's desired height? Seems like it might be worth doing that.

I actually have that in my current version of the patch, and I thought I'd done that before requesting review. I guess not, sorry.

> Also, lineHeight appears to just be an alias for computedHeight. Not sure
> it's worth having two variables.

I guess that's true for the current code, but I find it easier to read so I've left this as-is for now.

> Also, why are we trying to independently compute a baseline here rather than
> just using the already-computed baseline of our outerWrapperFrame? (or its
> first child, maybe) (adjusted into our coordinate space, of course)  Might
> just need to have a 

I don't think that works when the nsNumberControlFrame and outerWrapperFrame both have their height set (by style rules) to different values.

> nsHTMLReflowMetrics& aParentDesiredSize is unused - get rid of that
> parameter.
> 
> (though: while you're at it -- you might want to promote the local variable
> 'desiredSize' to be an outparam, so that our parent can shrinkwrap us and/or
> use our baseline, as suggested above.)

I don't think that works in the general case as noted above, so I've just removed this param.

> Why are these no-ops? (I don't know enough about these methods offhand to
> know)  Worth at least a comment justifying a no-op impl, if it's reasonable.

I'll move these out to the patches later in the queue where they're used. They're just stubs for now.
 
> s/StyleContext()/mStyleContext/ (unless there's a chance that they're
> different?)
> 
> (I know other code uses StyleContext() here, but I don't think (?) there's a
> good reason for that. Note that we use mContent instead of GetContent(), in
> the same function-call. Basically, there's no reason to use public-accessors
> internally.)

One advantage of sometimes having things this way is when you want to catch accesses in the debugger and the debugger's hardware breakpoints are not working well. GetRect() vs mRect has been a paint point in that regard for me on several occasions in the past. That said, lldb is working a lot better in that regard than Xcode's old gcc was been in the past. I've made the change as requested.

> Maybe file a followup on fixing the other instances of this in similar code,
> to keep us consistent?

Filed bug 926398.

> Technically nsTArray::AppendElement is infallible now, so this will never
> fail, but I guess it doesn't matter much.

I'll leave it to the person who removes the return value to remove this check. (Seems like those changes should go together.)

> s/display:inline-flex/with "display:inline-flex"/
> 
> Also: depending on how strongly we enforce this display type, do we really
> need the "display:none" null-checks for this wrapper mentioned elsewhere in
> this patch?
> (If we *don't* enforce this display type, maybe add "...by default" at the
> end of this comment here, or something.)
> (OK, I just found forms.css chunk - looks like we enforce it with 

As noted above, I don't think we should enforce it, so I've removed the !important from forms.css and added "...by default" to this comment.

> >+  NS_DECL_QUERYFRAME_TARGET(nsNumberControlFrame)
> 
> Is this actually a queryframe target? (i.e. do you need to do_QueryFrame
> from nsIFrame* to nsNumberControlFrame anywhere?)

In later patches, yes. I'll move this there.

> If not, I'm pretty sure you can drop this line.  If so, note that it won't
> actually work unless you add "NS_QUERYFRAME_ENTRY(nsNumberControlFrame)" to
> the queryframe block in the .cpp file. (currently there is no such line)

Yes, I have that in the later patch. I just missed the other line when juggling content between the various patches in my queue.

> >+  virtual bool IsFrameOfType(uint32_t aFlags) const
> >+  {
> >+    return nsContainerFrame::IsFrameOfType(aFlags &
> >+      ~(nsIFrame::eReplaced | nsIFrame::eReplacedContainsBlock));
> >+  }
> 
> Does eReplacedContainsBlock really make sense here? (Do we contain a block?)
> 
> (Do you know why we actually need this?  I guess we kinda return this all
> over the place for non-block-flavored things, so it's probably correct here,
> but I don't know why offhand.)

I just used this based on what other similar frame types do. It's also not immediately obvious to me why they are doing that though.

> >+# not valid on Android/B2G where type=number looks like type=text
> >+skip-if(Android||B2G) != not-other-type-unthemed-1.html not-other-type-unthemed-1a-notref.html
> >+skip-if(Android||B2G) != not-other-type-unthemed-1.html not-other-type-unthemed-1b-notref.html
> 
> Is it intentional that type=number looks like type=text on those platforms?
> (If so, we should probably assert that it does indeed match, with another
> test that's android/B2G-only.)

For the moment, yes, since we bring up a native number picker when the in-page control is focused. I've added a test to check that they look the same there.

> Also: A lot of these tests have "-moz-appearance:none;" -- is that
> necessary? (and do we need tests without that?)

I'd like to have tests that cover both the "-moz-appearance:none;" and the natively themed case. Obviously I need to implement the native theming before the latter tests can be added.

> >diff --git a/layout/style/forms.css b/layout/style/forms.css
> >+/* anonymous wrapper that is not exposed via a pseudo-element */
> >+input[type=number] > div {
> 
> ...but it is exposed via input[type=number] > div, yes? (can authors style
> it that way?)

Only chrome style sheets are allowed to access the anonymous content via direct selectors, so no, content authors cannot access it this way. As it happens though, I've added a pseudo-element for this wrapper div too after discussions with mounir and bz, so I've change this selector line to use the pseudo-element.

> It'd be worth including reftests to ensure that this is unstylable (assuming
> that's the intention), and also that the pseudo-elements *are*
> author-stylable.

Yes, I'll do that in bug 926412 though. Right now the behavior of the pseudo-elements isn't fixed. I really need to get this landed in order to have productive conversations with people to move that forward though.

> >+  /* Prevent styling that would change the type of frame we construct. */
> >+  display: inline-flex !important;
> 
> Worth including a comment somewhere explaining why we need "inline-flex"
> rather than "flex". (Or, if it's all the same, might as well use "flex" and
> save a few characters.)

Actually for now we need 'flex' (I've changed forms.css to use that). I've been trying to debug why inline-flex doesn't work as expected with bz, but it's very hard without him having an easy way to flip the code on and check things in his debugger, which is another reason I want to get this initial implementation landed ASAP.

> >+input[type=number]::-moz-number-spin-up {
> >+  display: flex;
> [...]
> >+input[type=number]::-moz-number-spin-down {
> >+  display: flex;
> 
> I don't think these pseudo-elements have children, so they shouldn't be
> "display:flex". (It doesn't buy you anything.)

The don't have children, but for some reason removing the "display:flex" breaks things. Yet another reason I'd like to get this landed so that I can get some assistance with that (so that you, in this case, can take a look in the debugger, if necessary).

> This div > div > div:hover selector is a bit hacky & dependent on the exact
> parent hierarchy. (and probably un-tested, so easy to regress if we do tweak
> the hierarchy)
> 
> I'd rather we select these elements with something more targeted, like:
>   input[type=number]::-moz-number-spin-up:hover,
>   input[type=number]::-moz-number-spin-down:hover {
> if that works.

The hover thing doesn't work. For now I've removed that and filed bug 926419.

> >diff --git a/layout/style/nsCSSPseudoElementList.h b/layout/style/nsCSSPseudoElementList.h
> > // HTML5 Forms pseudo elements
> >+CSS_PSEUDO_ELEMENT(mozNumberOuterWrapper, ":-moz-number-wrapper", 0)
> 
> This ^ pseudo-element is unused. (looks like it's left over from an earlier
> version of the patch).  (You use NotPseudoElement for the wrapper frame
> right now.)

Actually I've added support for this pseudo-element back as noted above.

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Watt [:jwatt] from comment #10)
> > Also, why are we trying to independently compute a baseline here rather than
> > just using the already-computed baseline of our outerWrapperFrame? (or its
> > first child, maybe) (adjusted into our coordinate space, of course)  Might
> > just need to have a 
> 
> I don't think that works when the nsNumberControlFrame and outerWrapperFrame
> both have their height set (by style rules) to different values.

It should work just fine, as long as you adjust it to be in terms of the nsNumberControlFrame's border box (by just adding the outerWrapperFrame's y-position within the nsNumberControlFrame).

> The don't have children, but for some reason removing the "display:flex"
> breaks things. Yet another reason I'd like to get this landed so that I can
> get some assistance with that (so that you, in this case, can take a look in
> the debugger, if necessary).

I bet these are inline-level by default, and they need to be block-level. (or else they get wrapped in anonymous flex items, and hence their own "flex" values get ignored in their parent flex container.)

Does this work if you use "display:block" on these? I'd bet it does, and I'd prefer that to keep it clearer what the flex container is here.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Watt [:jwatt] from comment #10)
> > So "height:auto" just gives us 0 height, instead of shrinkwrapping the
> > outerWrapperFrame's desired height? Seems like it might be worth doing that.
> 
> I actually have that in my current version of the patch, and I thought I'd
> done that before requesting review. I guess not, sorry.

You did have it, sorry; I misread the ternary condition. You only used 0 as the auto-height in the case where there was no child, which makes sense. :)

Comment 14

[Jonathan Watt [:jwatt]]

Ah, cool, no worries.

Comment 15

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> It should work just fine, as long as you adjust it to be in terms of the
> nsNumberControlFrame's border box (by just adding the outerWrapperFrame's
> y-position within the nsNumberControlFrame).

You're right. Done.

> I bet these are inline-level by default, and they need to be block-level.
> (or else they get wrapped in anonymous flex items, and hence their own
> "flex" values get ignored in their parent flex container.)
> 
> Does this work if you use "display:block" on these? I'd bet it does, and I'd
> prefer that to keep it clearer what the flex container is here.

Ditto.

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Watt [:jwatt] from comment #15)
> > I bet these are inline-level by default, and they need to be block-level.
> > (or else they get wrapped in anonymous flex items, and hence their own
> > "flex" values get ignored in their parent flex container.)
> > 
> > Does this work if you use "display:block" on these? I'd bet it does, and I'd
> > prefer that to keep it clearer what the flex container is here.
> 
> Ditto.

So this actually made a difference? If so, I actually think this is indicative of a flexbox bug.  Flex containers are generally supposed to coerce their child elements to be block-level (using "EnsureBlockDisplay"), except for raw runs of text.

ACTUALLY: it makes sense that this *would* have been a problem in the original patch that I reviewed, but I don't think you need this anymore in the current patch.  In the original patch, the widgets all had the the number frame's style context as their parent, *not* the flex container's style context -- which means they wouldn't get the flex-specific EnsureBlockDisplay() call).

But now that you have that style-context-parentage fixed (by e.g. using outerWrapperCI.mStyleContext as the parent style-context), I'll *bet* you don't actually need this "display: block" anymore.

Could you double-check to see?

Comment 18

[Daniel Holbert [:dholbert]]

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

>diff --git a/layout/base/RestyleManager.cpp b/layout/base/RestyleManager.cpp
[...]
>+    // Get content for nearest nsNumberControlFrame:
>+    nsIFrame* f = aFrame->GetParent();
>+    while (f && f->GetType() != nsGkAtoms::numberControlFrame) {
>+      f = f->GetParent();
>+    }
>+    MOZ_ASSERT(f);
>+    return f->GetContent()->AsElement();
>+  }

The "f" null-check in the while loop is technically unnecessary.  It looks like it's there for sanity, but it actually doesn't buy us any sanity, either, because when it makes us bail out of the loop, we'll still crash when we dereference f->GetContent().  So it's not actually useful.

Maybe drop that null-check, and instead have two MOZ_ASSERT(f) lines -- one after each GetParent() invocation -- to ensure debug builds will always fail the assertion before they do a null-deref? (reducing needless null-checks in opt builds)

Feel free to spin this off into a followup, too; not a huge deal, just a code-cleanup thing.

>+NS_IMETHODIMP
>+nsNumberControlFrame::Reflow(nsPresContext* aPresContext,
>+                             nsHTMLReflowMetrics& aDesiredSize,
>+                             const nsHTMLReflowState& aReflowState,
>+                             nsReflowStatus& aStatus)
[...]
>+  NS_ASSERTION(!mFrames.FirstChild() ||
>+               !mFrames.FirstChild()->GetNextSibling(),
>+               "We only expect one direct child frame");

s/only expect one/expect at most one/
(given the updated condition that you're asserting, which now allows for 0 child frames.)

>+  nsIFrame* outerWrapperFrame = mOuterWrapper->GetPrimaryFrame();
>+  if (outerWrapperFrame) { // display:none?

Inside of this condition, we should assert that outerWrapperFrame == mFrames.FirstChild()
(Otherwise, it's unclear whether we're actually reflowing anything in our "mFrames" list.)

>+  if (outerWrapperFrame) {
>+    aDesiredSize.ascent = wrappersDesiredSize.ascent +
>+      aReflowState.mComputedBorderPadding.top;
>+  }

nit: the adjustment isn't quite right there -- we should adding the wrapper's y-offset (which *happens* to currently be mComputedBorderPadding.top but shouldn't necessarily always be that, e.g. if "margin" is set on the wrapper and if we implement that as noted below).

Easily fixed, anyway:
s/aReflowState.mComputedBorderPadding.top/outerWrapperFrame.GetPosition().y/

>+  // The width/height of our content box, which is the available width/height
>+  // for our anonymous content:
>+  nscoord inputFrameContentBoxWidth = aParentReflowState.ComputedWidth();
>+

Drop both instances of "/height" from that comment.

>+  nsHTMLReflowState wrapperReflowState(aPresContext, aParentReflowState,
>+                                       aOuterWrapperFrame,
>+                                       nsSize(inputFrameContentBoxWidth,
>+                                              NS_UNCONSTRAINEDSIZE));
>+
>+  nscoord xoffset = aParentReflowState.mComputedBorderPadding.left;
>+  nscoord yoffset = aParentReflowState.mComputedBorderPadding.top;
>+

We need to also add wrapperReflowState.mComputedMargin.left and .top here, or else "margin" won't have any effect on the wrapper-frame. (The parent frame is responsible for applying child frames' margins.

>+  nsReflowStatus childStatus;
>+  nsHTMLReflowMetrics desiredSize;
>+  nsresult rv = ReflowChild(aOuterWrapperFrame, aPresContext,
>+                            aWrappersDesiredSize, wrapperReflowState,
>+                            xoffset, yoffset, 0, childStatus);

Drop the "desiredSize" local-var; it's unused, now that you have "aWrappersDesiredSize".

>+nsresult
>+nsNumberControlFrame::MakeAnonymousElement(nsIContent** aResult,
>+                                           nsTArray<ContentInfo>& aElements,
>+                                           nsIAtom* aTagName,
>+                                           nsCSSPseudoElements::Type aPseudoType,
>+                                           nsStyleContext* aParentContext)
>+{
[...]
>+  NS_ASSERTION(aPseudoType != nsCSSPseudoElements::ePseudo_NotPseudoElement,
>+               "Add a branch for this to call "
>+               "ResolveStyleFor((*aResult)->AsElement(), aParentContext)");

This is a little cryptic, for an assertion-message.

It sounds like the assertion wants to say something like...
 "Expecting anonymous children to all be pseudo-elements"
...and then the text you currently have in the assertion-message propably wants to be converted into a code-comment (e.g. "If we need to legitimately allow non-pseudo-element anonymous children, then we'll need to add a branch..." etc.)

>+++ b/layout/reftests/forms/input/number/reftest.list
[...]
>+# dynamic type changes:
>+#fuzzy(22,4)
>+== to-number-from-other-type-unthemed-1.html to-number-from-other-type-unthemed-1-ref.html

Drop the #fuzzy line.

>+input[type=number]::-moz-number-spin-up {
>+  /* We should be "display:block" so that we don't get wrapped in an anonymous
>+   * flex item that will prevent the setting of the "flex" property below from
>+   * working.
>+   */
>+  display: block;

Per comment 17, I'll bet you needed the block-level display on this (& spin-down) in previous versions of the patch, but I'll bet you don't need it now, so I think you can remove this.

If things break when you remove this, then I'm fine leaving it in, but please file a bug and CC me, since (per beginning of comment 17) that would indicate a bug in our flexbox impl.

r=me with the above addressed.

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #18)
> >+  nscoord xoffset = aParentReflowState.mComputedBorderPadding.left;
> >+  nscoord yoffset = aParentReflowState.mComputedBorderPadding.top;
> >+
> 
> We need to also add wrapperReflowState.mComputedMargin.left and .top here,
> or else "margin" won't have any effect on the wrapper-frame. (The parent
> frame is responsible for applying child frames' margins.

Alternately: if we want to *explicitly* ignore margins[1], then we should explicitly disable them in forms.css with "margin: 0 !important" (in the same way that we e.g. disable relative positioning with "position: static !important;").  That way, we won't end up partially-honoring a user-specified margin (in computed style) and then dropping it on the floor when building the actual box tree.

[1] e.g. because we know they'll look ugly with the default "width:100%; height:100%", and because you can just as easily use padding on the <input>, and because it makes the reflow code simpler if we ignore them

Comment 20

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> (In reply to Jonathan Watt [:jwatt] from comment #15)
> > Ditto.
> 
> So this actually made a difference?

Yes.

> I don't think you need this anymore in the current patch [...] now
> that you have that style-context-parentage fixed
> 
> Could you double-check to see?

It's still required for some reason.

Comment 21

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #18)
> Feel free to spin this off into a followup, too; not a huge deal, just a
> code-cleanup thing.

I just fixed it.

> Inside of this condition, we should assert that outerWrapperFrame ==
> mFrames.FirstChild()
> (Otherwise, it's unclear whether we're actually reflowing anything in our
> "mFrames" list.)

Good idea.

> nit: the adjustment isn't quite right there -- we should adding the
> wrapper's y-offset (which *happens* to currently be
> mComputedBorderPadding.top but shouldn't necessarily always be that, e.g. if
> "margin" is set on the wrapper and if we implement that as noted below).

Good catch.

> We need to also add wrapperReflowState.mComputedMargin.left and .top here,
> or else "margin" won't have any effect on the wrapper-frame. (The parent
> frame is responsible for applying child frames' margins.

Likewise.

> >+++ b/layout/reftests/forms/input/number/reftest.list
> [...]
> >+# dynamic type changes:
> >+#fuzzy(22,4)
> >+== to-number-from-other-type-unthemed-1.html to-number-from-other-type-unthemed-1-ref.html
> 
> Drop the #fuzzy line.

Actually I need to add a proper fuzzy-if, and I'll file a follow-up to figure out why that's happening. I had it just as a comment while debugging and forgot to change that.

> >+input[type=number]::-moz-number-spin-up {
> >+  /* We should be "display:block" so that we don't get wrapped in an anonymous
> >+   * flex item that will prevent the setting of the "flex" property below from
> >+   * working.
> >+   */
> >+  display: block;
> 
> Per comment 17, I'll bet you needed the block-level display on this (&
> spin-down) in previous versions of the patch, but I'll bet you don't need it
> now, so I think you can remove this.
> 
> If things break when you remove this, then I'm fine leaving it in, but
> please file a bug and CC me, since (per beginning of comment 17) that would
> indicate a bug in our flexbox impl.

I filed bug 926670.

Comment 22

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #19)
> Alternately: if we want to *explicitly* ignore margins[1], then we should
> explicitly disable them in forms.css

I'd rather not do that unless we have a good reason to, so I've not made that restriction.

Comment 23

[Mark Capella [:capella]]

Joing the cc: list ... this looks to fix a part of Bug 928163 I recently filed ...

Comment 24

[Daniel Holbert [:dholbert]]

needinfo=jwatt to make sure this gets landed before too long. :)

Comment 25

[Jonathan Watt [:jwatt]]

I've not forgotten.

Comment 26

[Jonathan Watt [:jwatt]]

Specifically, I need bug 927435 to land this since existing tests send key events to the <input> and check that it's value updates.

Comment 27

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/435d9468467c

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/435d9468467c

Comment 29

[Tim Nguyen :ntim]

Why is this resolved fixed ? The layout isn't even working properly :o

Comment 30

[Daniel Holbert [:dholbert]]

It's just preffed off by default. If you go to about:config and set "dom.forms.number" to true, then
  data:text/html,<input type="number">
will render as a textbox with up/down arrows on its right side.

(The arrows don't seem to work yet, but that's not part of this bug; that's probably one of the other dependencies off of bug 344616.)