776265 - textarea & min-height & border-box buggy since FF16

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Jan Skrasek]

I need to set min-height and height to textarea with -moz-box-sizing: border-box;
It looks similar to #308801, but It's quite weird because in ff14 (stable now) it already works correctly.
See testcase http://jsfiddle.net/hrach/Jh6eK/:
FF14: OK
FF15: OK
FF16+: FAIL

in FF16+:
1) textarea is rendered higher than should be, because min-height is calc without padding & border.
2) resizing handler is rendered on the old place, so there is another gap.

Comment 1

[Loic]

Attachment: Testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  This is a regression from bug 716875 (which rewrote textarea layout, so there's no great surprise that there's a behavior change).

Charly, Alexandre, do you have time to look at this?  Basically, the min/max-height stuff needs to consider box-sizing.

Comment 3

[u443890]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Ah, yes.  This is a regression from bug 716875 (which rewrote textarea
> layout, so there's no great surprise that there's a behavior change).
> 
> Charly, Alexandre, do you have time to look at this?  Basically, the
> min/max-height stuff needs to consider box-sizing.

Hi Boris. I'll try to take a look at this ASAP, but I'm really busy these days so it might take some time before I send a new patch. First I need to locate where min/max-height stuff is handled within the frame. 

By the way, since all the reftests were ok when we posted our previous patch, it might be a good idea to create a non-regression test for this case.

Comment 4

[Boris Zbarsky [:bzbarsky]]

The min/max handling is actually just in the code that was added in bug 716875.

Do you have a general estimate on the time?  If you're too busy, I can find someone else to do this, or do it myself.

And yes, we need to add more tests.  ;)

Comment 5

[Charly Molter :lahabana]

Hey 
I'm going to have a look at that tomorrow. Hope to make a patch in the day as this seems to be a small correction

Comment 6

[Charly Molter :lahabana]

Attachment: Patch with reftests

This patch seems to solve the problem.
I also added 2 new reftests one with min-height and one with max-height
I sent it to try a minute ago:
https://tbpl.mozilla.org/?tree=Try&rev=4d1dc2adf2d9
I will tell here how it goes

Comment 7

[Charly Molter :lahabana]

So my patch fixes the thing when it's with -moz-box-sizing: border-box;
But it breaks bug 481024 which is using {min, max}-{height,width} without border-box
what I need to do is to add an if statement I think does that seems correct to you boris?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yes, you need to actually check the box-sizing value.  I thought we had a helper for that, but I don't see one around now....

Comment 9

[Boris Zbarsky [:bzbarsky]]

Per irc discussion, the real problem is that reflow state doesn't adjust min/max height for box sizing.  It should do so.  That will also fix this testcase:

div {
    padding: 40px;
    min-height: 50px;
    min-width: 50px;
    max-height: 50px;
    max-width: 50px;
    box-sizing: border-box;
    -moz-box-sizing: border-box;
    background: green;
}

(which should produce a square box, but doesn't).

Comment 10

[Charly Molter :lahabana]

Attachment: temporary patch for feedback

Hey this is the start of what I've done to fix the bug. It's finished but I'll be away at least all day so I'd like to have a little feedback to be sure I'm going the right way. 
This patch seems to work with every test cases from: bug 308801 and this one even the div thing :bz posted.
A few reftests needs to be added and I think that some of the code I did might be wrong.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 646070 [details] [diff] [review]
temporary patch for feedback

You don't need all that complexity, because the various NS_STYLE_WIDTH_* enums involved aren't valid values for "height" so far (as their names imply).

The ComputeWidthValue calls in nsFrame in the height case are just wrong.

Why can the minmax bit go away?  Was it never really needed because the reflow state already handles that for the computed height?

Other than that, seems plausible.

Comment 12

[Charly Molter :lahabana]

(In reply to Boris Zbarsky (:bz) from comment #11)
> Comment on attachment 646070 [details] [diff] [review]
> temporary patch for feedback
> 
> You don't need all that complexity, because the various NS_STYLE_WIDTH_*
> enums involved aren't valid values for "height" so far (as their names
> imply).
Yes that's what I thought I should still keep the aCoord.IsCoordPercentageCalcUnit() and the (eStyleUnit_Enumerated == aCoord.GetUnit()) branches isn't it?

> The ComputeWidthValue calls in nsFrame in the height case are just wrong.

Indeed don't know what I did there.. (too much copy and paste and not enough attention)
> 
> Why can the minmax bit go away?  Was it never really needed because the
> reflow state already handles that for the computed height?

For me reflowState handles it in nsLayoutUtils::Compute{Height,Width}DependentValue
If not the way I was using it is probably wrong because bug 369582 the mComputed{Min,Max}{Width,Height} should not have to be used
> 
> Other than that, seems plausible.

Comment 13

[Boris Zbarsky [:bzbarsky]]

You shouldn't need the enumerated branch, as far as I can tell.

Ignore bug 369582 for now.  ;)

Comment 14

[Charly Molter :lahabana]

Attachment: patch close to completion (needs more testing)

This patch takes into account your remarks and seems to work on the few example I found. 
Though I haven't checked all the spec and many more reftests needs to be added.
I sent it to try to see how it is going for the moment:
https://tbpl.mozilla.org/?tree=Try&rev=5684d545cc4d

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 646605 [details] [diff] [review]
patch close to completion (needs more testing)

I don't see how this can possibly work, since you use neither aContentEdgeToBoxSizing nor aBoxSizingToMarginEdge.

You also don't use aRenderingContext or aFrame, which is fine: you just shouldn't be passing them in.

The function should have three arguments, all used: aContainingBlockHeight, aContentEdgeToBoxSizing, aCoord.

Comment 16

[Charly Molter :lahabana]

Hey I haven't given up. I'm just moving so i won't be able to provide a patch before at least tomorrow.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Just ran into this myself. Here's another testcase (perhaps clearer than the existing one), with no explicit moz-box-sizing required.

This testcase has equal "min-height" and "height" properties on all the widgets -- so one would expect that the min-height shouldn't have any effect.  (but it does, because if you remove the "min-height" property, it works as expected.)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Charly, are you still working on this?  We really want to get a fix in on aurora no later than Aug 27 if we can, and that'll include lag time for approvals..

Comment 19

[Charly Molter :lahabana]

Ok I'll get that done in worst this weekend is that ok? I'm sorry I'm quite busy atm but I'll find time to finish this

Comment 20

[Boris Zbarsky [:bzbarsky]]

That should be ok, but you might want to ask roc or dbaron for review, because I'll be gone for all of next week.

Comment 21

[Charly Molter :lahabana]

Attachment: Patch with reftests

This patch should fix the problem. {min,max}-{height,width}.
I've sent it try https://tbpl.mozilla.org/?tree=Try&rev=91a14d001195
I added some reftests cause apparently it wasn't tested before...

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 650188 [details] [diff] [review]
Patch with reftests

aBoxSizingToMarginEdge is still unused; just get rid of it.

I'm not sure about the warnings for aContainingBlockHeight == NS_UNCONSTRAINEDSIZE.  Generally it can be (or more precisely NS_AUTOHEIGHT), but ComputeHeightDependentValue has existing asserts that are similar.  If you're already checking this at all callers, this should be an assertion, not a warning.  But I think you're not in fact checking this for mComputedHeight in nsHTMLReflowState::InitConstraints, unless I'm missing something....

nsCSSOffsetState::ComputeHeightValue has some indentation issues in the return statement.

The div bit in 776265-1a/b/c/d.html seems completely unrelated to what the test is actually testing, right?  Why is it there?

Same thing with 2a/b/c/d.

Does this patch end up fixing bug 308801 too?  If so, add tests for that?

Comment 23

[Charly Molter :lahabana]

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #22)
> Comment on attachment 650188 [details] [diff] [review]
> Patch with reftests
> 
> aBoxSizingToMarginEdge is still unused; just get rid of it.
OK
> I'm not sure about the warnings for aContainingBlockHeight ==
> NS_UNCONSTRAINEDSIZE.  Generally it can be (or more precisely
> NS_AUTOHEIGHT), but ComputeHeightDependentValue has existing asserts that
> are similar.  If you're already checking this at all callers, this should be
> an assertion, not a warning.  But I think you're not in fact checking this
> for mComputedHeight in nsHTMLReflowState::InitConstraints, unless I'm
> missing something....
Hmm this warning seems to be everywhere in the server tests so something is wrong about it... I'm gonna dig 
> nsCSSOffsetState::ComputeHeightValue has some indentation issues in the
> return statement.
OK
> The div bit in 776265-1a/b/c/d.html seems completely unrelated to what the
> test is actually testing, right?  Why is it there?
It's true it's useless it was helpful to debug by sight before making it a reftest I'll remove these
> Same thing with 2a/b/c/d.
> 
> Does this patch end up fixing bug 308801 too?  If so, add tests for that?
Yes it fixes 308801 too I've checked with the testcases that were given and it works. 
The tests I've made also check that no? especially the 1{a,b,c,d)?  the 2 were added cause it seemed not logical to add tests for height without adding tests for width

Comment 24

[Boris Zbarsky [:bzbarsky]]

The tests you added don't test bug 308801 as far as I can tell.  That bug is about auto-height blocks, not text controls.

Comment 25

[Charly Molter :lahabana]

Ho ok so I'll add more reftests with auto-height blocks. I didn't get the difference sorry...
Btw should a bug be reported to refactor nsHTMLReflowState::InitConstraintscause that method is more than 200 lines long and therefore really hard to understand.

Comment 26

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

Comment on attachment 650188 [details] [diff] [review]
Patch with reftests

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

Looks like Boris is already on this :-)

Comment 27

[Boris Zbarsky [:bzbarsky]]

I soft of am until tomorrow.  The problems start if this is not landed by then... ;)

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 650188 [details] [diff] [review]
Patch with reftests

Per comments.

Comment 29

[Charly Molter :lahabana]

So I really don't know if that warning makes sens... Isn't it possible at that point to have an unconstrained height? It looks like it cause this warning is raised often by the reftests.
For me there should be cases where we use aContainingBlockHeight as unconstrained no?

Comment 30

[Charly Molter :lahabana]

Attachment: final patch

Here is the patch the warning has been removed as apparently it was sometimes true and sometimes false. I don't think it made that much sense in the end. What do you think?
I've added reftests this patch also fixes Bug 308801 and I've written reftests for that later though they are not included here I'll make a patch when this patch is checked-into central. 
There's reftests in the enclosed patch.
I've sent it to try:
https://tbpl.mozilla.org/?tree=Try&rev=20166f0dc804
There's only one problem left with an assertion that is raised that I don't really understand:
The assertion is: ###!!! ASSERTION: non-root frame's desired size changed during an incremental reflow: '(target == rootFrame && size.height == NS_UNCONSTRAINEDSIZE) || (desiredSize.width == size.width && desiredSize.height == size.height)', file e:/builds/moz2_slave/try-w32-dbg/build/layout/base/nsPresShell.cpp, line 7472
The crashtest is: file:///c:/talos-slave/test/build/reftest/tests/layout/xul/base/src/crashtests/464407-1.xhtml 
Don't really know what that problem is...

Comment 31

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

Comment on attachment 651350 [details] [diff] [review]
final patch

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

I don't see reftests for auto-height blocks in this patch. In fact I don't see how this handles box-sizing:border-box on auto-height blocks with min-height/max-height, since the changes in nsFrame::ComputeSize only affect non-auto-height frames. I think that could be done as a separate patch though, probably in bug 308801 instead of this one.

::: layout/base/nsLayoutUtils.cpp
@@ +2788,5 @@
> +    result = 0;
> +  }
> +  if (result < 0)
> +    result = 0;
> +  return result;

return NS_MAX(0, result);

::: layout/base/nsLayoutUtils.h
@@ +910,5 @@
>     * Likewise, but for 'height', 'min-height', or 'max-height'.
>     */
> +  static nscoord ComputeHeightValue(
> +                   nscoord             aContainingBlockHeight,
> +                   nscoord             aContentEdgeToBoxSizing,

This name isn't very helpful. Can you call it aContentEdgeToBoxSizingBoxEdge?

Comment 32

[Mats Palmgren (inactive)]

>+  nscoord boxSizingToMarginEdgeHeight =

unused variable

>+  if (aCoord.IsCoordPercentCalcUnit()) {
> [...]
>+  } else {
>+    NS_NOTREACHED("unexpected height value");
>+    result = 0;
>+  }

Please replace the above with:
  MOZ_ASSERT(aCoord.IsCoordPercentCalcUnit());
  [...]

(nsLayoutUtils::ComputeWidthValue can be improved similarly)

Regarding the aContainingBlockHeight warning - I think you should
add it back like so:
  MOZ_ASSERT(aContainingBlockHeight != NS_AUTOHEIGHT || !aCoord.HasPercent(),
             "caller must deal with %% of unconstrained height");

Your added NS_ASSERTIONs about negative height seems unnecessarily
verbose - I think you should just document that the method guarantees
a non-negative return value in the header file. (roc's NS_MAX suggestion
above makes it clear)

>+nsCSSOffsetState::ComputeHeightValue(nscoord aContainingBlockHeight,
>+                                     PRUint8 aBoxSizing,
>+                                     const nsStyleCoord& aCoord)
>+{
>+  nscoord inside = 0, outside = mComputedBorderPadding.TopBottom() +
>+                                mComputedMargin.TopBottom();
>+  switch (aBoxSizing) {
>+    case NS_STYLE_BOX_SIZING_BORDER:
>+      inside = mComputedBorderPadding.TopBottom();
>+      break;
>+    case NS_STYLE_BOX_SIZING_PADDING:
>+      inside = mComputedPadding.TopBottom();
>+      break;
>+  }
>+  outside -= inside;
>+
>+  return ComputeHeightValue(aContainingBlockHeight, inside, aCoord);
>+}

Why calculate 'outside'?  It's not used for anything AFAICT.

Comment 33

[Charly Molter :lahabana]

Attachment: Fixing patch

This is the patch with the correction you asked for plus a few other minor modifications (NS_MAX instead of "if(result < 0)..."). I've sent it to try:
https://tbpl.mozilla.org/?tree=Try&rev=e4a9b4640b98
Concerning roc's comment:
>I don't see reftests for auto-height blocks in this patch. 
Sorry I wasn't clear in my previous submission. The reftests for auto-height are not included in that patch because they will be added in a patch for bug 308801.

>In fact I don't see how this handles box-sizing:border-box on auto-height blocks
>with min-height/max-height, since the changes in nsFrame::ComputeSize only
>affect non-auto-height frames.
Maybe I'm wrong but that's not the case, nsLayoutUtils::ComputeHeightValue has been totally rewritten to consider box-sizing:border-box that's the whole point of the parameter "aContentEdgeToBoxSizingBoxEdge". no? Consequently both auto-height and non auto-height are influenced by that change.
Not everything is 100% clear to me in size computation so I might be wrong. Though it seems to be fixed for both...

Comment 34

[Mats Palmgren (inactive)]

What I meant was that we should replace code like this:

  if (aCoord.IsCoordPercentCalcUnit()) {
    XXX;
  } else {
    NS_NOTREACHED("unexpected height value");
    ...
  }

with this:

  MOZ_ASSERT(aCoord.IsCoordPercentCalcUnit());
  XXX;

It makes the code more readable and eliminating the
"if (aCoord.IsCoordPercentCalcUnit())" test in optimized builds makes it
faster (MOZ_ASSERT is only evaluated in debug builds).  If we're
asserting a condition is true there's no need to add code to handle it
being false.  (I know we have some old code that does this)

For nsLayoutUtils::ComputeWidthValue it would look like this:

  if (aCoord.IsCoordPercentCalcUnit()) {
    ...
  } else {
    MOZ_ASSERT(eStyleUnit_Enumerated == aCoord.GetUnit());
    ...
  }

(that is, the "if (eStyleUnit_Enumerated == aCoord.GetUnit())" test is
redundant since it's always true in the first else-branch)

Comment 35

[Charly Molter :lahabana]

Attachment: Updated patch

Just corrected what :mats said about MOZ_ASSERT else it's the same patch.

Comment 36

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

(In reply to Charly Molter :lahabana from comment #33)
> Maybe I'm wrong but that's not the case, nsLayoutUtils::ComputeHeightValue
> has been totally rewritten to consider box-sizing:border-box that's the
> whole point of the parameter "aContentEdgeToBoxSizingBoxEdge". no?
> Consequently both auto-height and non auto-height are influenced by that
> change.

Sorry, you're right.

Comment 37

[Charly Molter :lahabana]

Hey just sent yesterday's patch to try: 
https://tbpl.mozilla.org/?tree=Try&rev=ca3db8c3d48c
There's no results ftm but I'm not convinced the assert that was raised on windows won't raise again...

Comment 38

[Charly Molter :lahabana]

Hey,
There's still a problem with the assert on windows. Though I don't really have any idea...
Also bz wanted to include it quite quickly so it would be nice to get reviews and solve the last glitches in order to push in to mozilla-central

Comment 39

[Mats Palmgren (inactive)]

Comment on attachment 651789 [details] [diff] [review]
Updated patch

A couple of nits:

nsLayoutUtils::ComputeHeightValue is more readable if you put
both assertions at the top, remove the "result = 0", and put
the subtraction into NS_MAX, like so:

{
  MOZ_ASSERT(aContainingBlockHeight != NS_AUTOHEIGHT || !aCoord.HasPercent(),
             "caller must deal with %% of unconstrained height");
  MOZ_ASSERT(aCoord.IsCoordPercentCalcUnit());

  nscoord result = nsRuleNode::ComputeCoordPercentCalc(aCoord,
                                                       aContainingBlockHeight);
  // Clamp calc(), and the subtraction for box-sizing.
  return NS_MAX(0, result - aContentEdgeToBoxSizingBoxEdge);
}

It's short enough to be put in nsLayoutUtils.h for inlining.

The first variant of nsCSSOffsetState::ComputeHeightValue is only used once,
by the 2nd variant, so you can just call nsLayoutUtils::ComputeHeightValue
directly there and remove the first variant.

=====

The Try run have an unexpected assertion on Windows for
layout/xul/base/src/crashtests/464407-1.xhtml

###!!! ASSERTION: non-root frame's desired size changed during an incremental reflow: '(target == rootFrame && size.height == NS_UNCONSTRAINEDSIZE) || (desiredSize.width == size.width && desiredSize.height == size.height)', file e:/builds/moz2_slave/try-w32-dbg/build/layout/base/nsPresShell.cpp, line 7472

I debugged it briefly and the reflow target for the assertion is a
ScrollbarFrame.  The available height when reflowing it is
NS_AUTOHEIGHT, but that may be due to the "height:72057594037927940pt"
in the testcase (nscoord_MAX == NS_AUTOHEIGHT).  I converted the test
to HTML and that asserts also on Linux and furthermore it also asserts
in an *unpatched* build (I've attached the test in 450974 as "testcase3").
Bug 366791 and bug 450974 already handles this assertion and have tests
that are similar to 464407-1.xhtml.

So I'm OK with annotating 464407-1.xhtml with "asserts-if(winWidget,1)"
in the manifest (referencing bug 450974) and landing the patch with the
above nits fixed.

r=mats

Comment 40

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #22)
> Does this patch end up fixing bug 308801 too?  If so, add tests for that?

Yes.  I've attached a patch with reftests there.

Comment 41

[Charly Molter :lahabana]

Attachment: Patch corrected

This is the corrected patch with the few things you said.
When you were saying "It's short enough to be put in nsLayoutUtils.h for inlining." you really wanted to make it inline yes? I did so anyway

It was sent to try:
https://tbpl.mozilla.org/?tree=Try&rev=b3318f79f9c2

Comment 42

[Mats Palmgren (inactive)]

Comment on attachment 653181 [details] [diff] [review]
Patch corrected

Yes, but you don't need the keyword |inline| explicitly since the method
is inline within the class definition.

Do you have commit access?  (I can land it for you otherwise)

Comment 43

[Charly Molter :lahabana]

No I don't. Please land it for me.
I had a few other reftests for bug 308801 should I make a patch or the patch you created is enough tests?
Thx for the reviews

Comment 44

[Mats Palmgren (inactive)]

Thank *you* for fixing this bug, it will help a lot towards unprefixing
-moz-box-sizing.

Please attach your tests for bug 308801 in that bug.  I'll land them
together with mine unless they look the same.

Removed the |inline| and re-indented some lines:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d99b258809ca

Comment 45

[Charly Molter :lahabana]

Attachment: reftests for bug 308801

These are reftests I made for bug 308801 :mats is going to look at them and judge if they are useful or not.

Comment 46

[Mats Palmgren (inactive)]

Comment on attachment 653196 [details] [diff] [review]
reftests for bug 308801

I think these cases are already covered by the reftests I just landed
for bug 308801.  Thanks anyway.

Comment 47

[Phil Ringnalda (:philor)]

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

Comment 48

[Lukas Blakk [:lsblakk] use ?needinfo]

Will there be an Aurora nom here?

Comment 49

[Mats Palmgren (inactive)]

Comment on attachment 653181 [details] [diff] [review]
Patch corrected

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 716875
User impact if declined: text input controls will have wrong size in some rare cases
Testing completed (on m-c, etc.): on m-c since 2012-08-19
Risk to taking this patch (and alternatives if risky): the risk for regressions is fairly low since this part of the code have pretty good test coverage
String or UUID changes made by this patch: none

Comment 50

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/163a48714779

Comment 51

[Scoobidiver (away)]

Backout from Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/6d13c5f78d00

Comment 52

[Boris Zbarsky [:bzbarsky]]

You meant "unaffected": the bug that caused this one was backed out too.

Comment 53

[Boris Zbarsky [:bzbarsky]]

But maybe it should have stayed "fixed".  Who knows.  In any case, "affected" is wrong.

Comment 54

[Manuela Muntean [Away]]

Fixed on the latest beta, checked for both MAC and Win 7 64bit:

-> Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/17.0 Firefox/17.0, Build ID: 20121023124120

-> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0, Build ID: 20121023124120