951310 - min-height and max-height don't work with number inputs

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P4)

Description

[Tim Nguyen :ntim]

Brian Grinstead noticed this in bug 948324 (there is a screenshot there). I noticed that applying it to :-moz-number-text works (using an user style).

Also, I saw something interesting, applying display:flex; to the parent of the input makes min/max-height work.

Test cases :
- min-height : data:text/html, <input type="number" style="min-height:300px"/>
- max-height : data:text/html, <input type="number" style="max-height:10px"/>
- Flex container + min-height : data:text/html, <div style="display:flex;"><input type="number" style="min-height:300px"/></div>
- Flex container + max-height : data:text/html, <div style="display:flex;"><input type="number" style="max-height:10px"/></div>

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Daniel Holbert [:dholbert]]

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

[sorry for the bit of delay on this]

>+      // Make sure we obey min/max-height in the case when we're doing intrinsic
>+      // sizing (we get it for free when we have a non-intrinsic
>+      // aReflowState.ComputedHeight()).  Note that we do this before
>+      // adjusting for borderpadding, since mComputedMaxHeight and
>+      // mComputedMinHeight are content heights.
>+      contentBoxHeight =
>+        NS_CSS_MINMAX(contentBoxHeight,
>+                      aReflowState.ComputedMinHeight(),
>+                      aReflowState.ComputedMaxHeight());

This is a subtle point. (particularly because ComputedMinHeight() != computed 'min-height', due to the box-sizing adjustments we do in the reflow state)

Could you include a test to make sure we're doing this correctly (and that we don't accidentally break this down the road)?

e.g. a test with like "min-height" on an intrinsically-sized <number> frame, with a largeish border, where shrinkwrap height is less than the min-height, but including the border pushes us over.
(with & without 'box-sizing: border-box')

>+    nscoord yoffset = aReflowState.ComputedPhysicalBorderPadding().top +
>+                        wrapperReflowState.ComputedPhysicalMargin().top;
>+
[...]
>+    // Center child vertically
>+    nscoord extraSpace = contentBoxHeight - wrappersDesiredSize.Height();
>+    yoffset = std::max(0, extraSpace / 2);

So this seems to be centering the wrapper's content-box inside the <input>'s content-box.

Shouldn't we instead be centering the wrapper's *margin-box*?

Related point: it looks like we're stomping on the original yoffset value here.  Aren't we losing information (e.g. about its margin) by doing that?

e.g. if we have an intrinsically-sized <input type="number"> with a margin on its wrapper, it looks to me like we'll end up ignoring the margin, both for sizing and positioning purposes. (We use it for the initial ReflowChild call, but then by the time we've called FinishReflowChild, I think we've lost track of it.)

(If I'm not mistaken about this -- could you include a test for this sort of thing as well?)

Comment 3

[Daniel Holbert [:dholbert]]

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

>+  nscoord contentBoxHeight = aReflowState.ComputedHeight();
[SNIP]
>+    nscoord contentBoxWidth = aReflowState.ComputedWidth();
[SNIP]
>+  aDesiredSize.Width() = aReflowState.ComputedWidth() +
>+                         aReflowState.ComputedPhysicalBorderPadding().LeftRight();
>+  aDesiredSize.Height() = contentBoxHeight +
>+                          aReflowState.ComputedPhysicalBorderPadding().TopBottom();

Nit: It'd probably be a bit nicer to declare contentBoxWidth up one level of scope, next to contentBoxHeight. (Perhaps as "const" to indicate that it (unlike the height) won't be adjusted.)

Then, you can use it when setting aDesiredSize.Width(), making that a bit more consistent with the .Height()-setting. (and making it more clear that you're consistently using the same content-box-width value throughout the function).

>+  aDesiredSize.SetOverflowAreasToDesiredBounds();
>+
>+  if (outerWrapperFrame) {
>     ConsiderChildOverflow(aDesiredSize.mOverflowAreas, outerWrapperFrame);
>   }

It looks like in current mozilla-central, we call these two functions ^^ in the wrong order (first ConsiderChildOverflow, and then SetOverflowAreasToDesiredBounds, which I think unceremoniously stomps on anything that ConsiderChildOverflow did).

Your patch fixes that, which is great.

That seems like a bug that should have observable effects.  Is it possible to test that as well? (e.g. with a MozReftestInvalidate-triggered adjustment of the position of an <input type="number">, with an overflowing child inside of the <input>.)

(Maybe file a followup on that test, if you don't get to it here.)

Comment 4

[Daniel Holbert [:dholbert]]

[to clarify: when I said "This is a subtle point" in comment 2, I was referring to the code-comment about "Note that we do this before adjusting for borderpadding etc"]

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> This is a subtle point. (particularly because ComputedMinHeight() !=
> computed 'min-height', due to the box-sizing adjustments we do in the reflow
> state)
> 
> Could you include a test to make sure we're doing this correctly (and that
> we don't accidentally break this down the road)?

I added tests for this. Unfortunately it was a pain to get the x-offset of the spinner in test and ref to match so I had to disable it.

> So this seems to be centering the wrapper's content-box inside the <input>'s
> content-box.
> 
> Shouldn't we instead be centering the wrapper's *margin-box*?
> 
> (If I'm not mistaken about this -- could you include a test for this sort of
> thing as well?)

I've added a comment to bug 926412 to do this as part of that bug since we need to access the pseudo-elements to test this.

(In reply to Daniel Holbert [:dholbert] from comment #3)
> >+  aDesiredSize.SetOverflowAreasToDesiredBounds();
> >+
> >+  if (outerWrapperFrame) {
> >     ConsiderChildOverflow(aDesiredSize.mOverflowAreas, outerWrapperFrame);
> >   }
> 
> It looks like in current mozilla-central, we call these two functions ^^ in
> the wrong order (first ConsiderChildOverflow, and then
> SetOverflowAreasToDesiredBounds, which I think unceremoniously stomps on
> anything that ConsiderChildOverflow did).
> 
> Your patch fixes that, which is great.
> 
> That seems like a bug that should have observable effects.  Is it possible
> to test that as well? (e.g. with a MozReftestInvalidate-triggered adjustment
> of the position of an <input type="number">, with an overflowing child
> inside of the <input>.)
> 
> (Maybe file a followup on that test, if you don't get to it here.)

I'll file a follow-up.

Comment 7

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #6)
> I'll file a follow-up.

Actually that too needs access to the pseudo-elements, so I've added a comment to bug 926412 for that too.

Comment 8

[Daniel Holbert [:dholbert]]

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

>+++ b/layout/forms/nsNumberControlFrame.cpp
>+    if (contentBoxHeight == NS_INTRINSICSIZE) {
>+      // We are intrinsically sized -- we should shrinkwrap the outer wrapper's
>+      // height:
>+      contentBoxHeight = wrappersDesiredSize.Height() +
>+                           wrapperReflowState.ComputedPhysicalMargin().TopBottom();
[...]
>+    }
>+
>+    // Center child vertically
>+    nscoord extraSpace = contentBoxHeight -
>+                           wrappersDesiredSize.Height() -
>+                           wrapperReflowState.ComputedPhysicalMargin().TopBottom();

Let's simplify the arithmetic (and make it clearer what's going) on by declaring this, before the NS_INTRINSICSIZE check:

 nscoord wrappersMarginBoxHeight = wrappersDesiredSize.Height() +
   wrapperReflowState.ComputedPhysicalMargin().TopBottom();

and then use that in both of the expressions above.
>+++ b/layout/reftests/forms/input/number/number-min-height-1-ref.html
>@@ -0,0 +1,18 @@
>+<!DOCTYPE html>
>+<html>
>+  <head>
>+    <style>
>+
>+div {
>+  border: 3px solid black;
>+  padding: none;

"none" isn't a valid value for "padding".

(No need to set padding anyway, since it defaults to 0 on div. Just drop that line)

>+++ b/layout/reftests/forms/input/number/number-min-height-1.html
>+input {
>+  border: 3px solid black;
>+  padding: 3px;
>+  width: 300px;
>+  min-height: 100px;
>+  box-sizing: content-box;

Please use a different value (e.g. "padding:4px") for the padding vs. border, just for clarity's sake.

(I was initially confused at where the "306px" in the reference case came from -- I initially thought it was from the border, and that seemed like a bug, given the 'content-box' box-sizing. But now I realize it's just from the padding [which doesn't exist in the reference case]. Anyway, that would be clearer if there were different amounts of border vs. padding, e.g. "padding:4px" and "width:308px" in the reference case.)

>+++ b/layout/reftests/forms/input/number/number-min-height-2.html
>+input {
>+  border: 3px solid black;
>+  padding: 3px;

Same here.

Also: please add some similar reftests that exercise "max-height" instead of "min-height".

r=me with the above.

Comment 9

[Jonathan Watt [:jwatt]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Jonathan Watt [:jwatt]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug in new feature (bug 344616)
User impact if declined: number control styling failure if styled using min/max-width/height
Testing completed (on m-c, etc.): merged to m-c
Risk to taking this patch (and alternatives if risky): low risk, early in aurora
String or IDL/UUID changes made by this patch: none

Comment 12

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0d08b822b2a2