Last Comment Bug 776265 - textarea & min-height & border-box buggy since FF16
: textarea & min-height & border-box buggy since FF16
Status: VERIFIED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: 16 Branch
: All All
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: Charly Molter :lahabana
:
: Jet Villegas (:jet)
Mentors:
Depends on: 785754 789007 789824
Blocks: 243412 308801 716875
  Show dependency treegraph
 
Reported: 2012-07-21 10:56 PDT by Jan Skrasek
Modified: 2014-01-11 14:27 PST (History)
13 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
unaffected
+
verified


Attachments
Testcase (489 bytes, text/html)
2012-07-21 12:35 PDT, Loic
no flags Details
Patch with reftests (4.23 KB, patch)
2012-07-24 03:28 PDT, Charly Molter :lahabana
bzbarsky: review-
Details | Diff | Splinter Review
temporary patch for feedback (20.54 KB, patch)
2012-07-26 02:16 PDT, Charly Molter :lahabana
bzbarsky: feedback+
Details | Diff | Splinter Review
patch close to completion (needs more testing) (19.16 KB, patch)
2012-07-27 09:18 PDT, Charly Molter :lahabana
bzbarsky: review-
Details | Diff | Splinter Review
testcase 2 (553 bytes, text/html)
2012-08-07 16:49 PDT, Daniel Holbert [:dholbert]
no flags Details
Patch with reftests (24.12 KB, patch)
2012-08-08 09:52 PDT, Charly Molter :lahabana
bzbarsky: review-
Details | Diff | Splinter Review
final patch (22.22 KB, patch)
2012-08-13 06:22 PDT, Charly Molter :lahabana
no flags Details | Diff | Splinter Review
Fixing patch (23.53 KB, patch)
2012-08-14 07:24 PDT, Charly Molter :lahabana
no flags Details | Diff | Splinter Review
Updated patch (24.71 KB, patch)
2012-08-14 08:58 PDT, Charly Molter :lahabana
roc: review+
mats: review+
Details | Diff | Splinter Review
Patch corrected (24.44 KB, patch)
2012-08-19 07:31 PDT, Charly Molter :lahabana
mats: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
reftests for bug 308801 (4.60 KB, patch)
2012-08-19 11:41 PDT, Charly Molter :lahabana
no flags Details | Diff | Splinter Review

Description Jan Skrasek 2012-07-21 10:56:49 PDT
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 2012-07-21 12:35:36 PDT
Created attachment 644672 [details]
Testcase
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-07-21 12:36:23 PDT
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 Alexandre Dumont 2012-07-21 13:53:30 PDT
(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 [:bz] (still a bit busy) 2012-07-21 16:22:28 PDT
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 2012-07-23 08:47:33 PDT
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 2012-07-24 03:28:07 PDT
Created attachment 645235 [details] [diff] [review]
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 2012-07-24 06:34:18 PDT
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 [:bz] (still a bit busy) 2012-07-24 07:13:01 PDT
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 [:bz] (still a bit busy) 2012-07-24 08:47:10 PDT
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 2012-07-26 02:16:38 PDT
Created attachment 646070 [details] [diff] [review]
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 [:bz] (still a bit busy) 2012-07-26 11:05:40 PDT
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 2012-07-27 02:29:25 PDT
(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 [:bz] (still a bit busy) 2012-07-27 08:29:20 PDT
You shouldn't need the enumerated branch, as far as I can tell.

Ignore bug 369582 for now.  ;)
Comment 14 Charly Molter :lahabana 2012-07-27 09:18:43 PDT
Created attachment 646605 [details] [diff] [review]
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 [:bz] (still a bit busy) 2012-07-27 11:40:15 PDT
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 2012-07-30 10:46:47 PDT
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] 2012-08-07 16:49:42 PDT
Created attachment 649883 [details]
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 [:bz] (still a bit busy) 2012-08-07 16:52:33 PDT
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 2012-08-07 23:27:49 PDT
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 [:bz] (still a bit busy) 2012-08-07 23:36:17 PDT
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 2012-08-08 09:52:13 PDT
Created attachment 650188 [details] [diff] [review]
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 [:bz] (still a bit busy) 2012-08-08 10:17:15 PDT
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 2012-08-08 11:48:36 PDT
(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 [:bz] (still a bit busy) 2012-08-08 11:52:10 PDT
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 2012-08-08 12:05:02 PDT
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) 2012-08-08 15:06:43 PDT
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 [:bz] (still a bit busy) 2012-08-08 16:15:10 PDT
I soft of am until tomorrow.  The problems start if this is not landed by then... ;)
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2012-08-10 06:17:41 PDT
Comment on attachment 650188 [details] [diff] [review]
Patch with reftests

Per comments.
Comment 29 Charly Molter :lahabana 2012-08-10 10:50:11 PDT
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 2012-08-13 06:22:41 PDT
Created attachment 651350 [details] [diff] [review]
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) 2012-08-13 16:43:43 PDT
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 (:mats) 2012-08-13 18:58:39 PDT
>+  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 2012-08-14 07:24:01 PDT
Created attachment 651758 [details] [diff] [review]
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 (:mats) 2012-08-14 08:45:58 PDT
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 2012-08-14 08:58:30 PDT
Created attachment 651789 [details] [diff] [review]
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) 2012-08-14 15:44:40 PDT
(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 2012-08-15 05:45:48 PDT
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 2012-08-18 03:04:55 PDT
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 (:mats) 2012-08-18 10:10:14 PDT
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 (:mats) 2012-08-18 10:25:20 PDT
(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 2012-08-19 07:31:40 PDT
Created attachment 653181 [details] [diff] [review]
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 (:mats) 2012-08-19 08:34:16 PDT
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 2012-08-19 08:37:14 PDT
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 (:mats) 2012-08-19 09:08:28 PDT
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 2012-08-19 11:41:17 PDT
Created attachment 653196 [details] [diff] [review]
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 (:mats) 2012-08-19 18:20:49 PDT
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) 2012-08-19 21:14:08 PDT
https://hg.mozilla.org/mozilla-central/rev/d99b258809ca
Comment 48 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-20 19:27:29 PDT
Will there be an Aurora nom here?
Comment 49 Mats Palmgren (:mats) 2012-08-21 03:50:10 PDT
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 51 Scoobidiver (away) 2012-09-17 00:02:13 PDT
Backout from Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/6d13c5f78d00
Comment 52 Boris Zbarsky [:bz] (still a bit busy) 2012-09-17 04:20:36 PDT
You meant "unaffected": the bug that caused this one was backed out too.
Comment 53 Boris Zbarsky [:bz] (still a bit busy) 2012-09-17 04:21:55 PDT
But maybe it should have stayed "fixed".  Who knows.  In any case, "affected" is wrong.
Comment 54 Manuela Muntean [Away] 2012-10-30 02:12:57 PDT
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

Note You need to log in before you can comment on or make changes to this bug.