Last Comment Bug 716875 - nsTextControlFrame shouldn't inherit from nsStackFrame
: nsTextControlFrame shouldn't inherit from nsStackFrame
Status: RESOLVED FIXED
[mentor=mounir][lang=c++]
:
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: Charly Molter :lahabana
:
: Jet Villegas (:jet)
Mentors:
Depends on: 776265 782660 787274
Blocks: 157846 527459
  Show dependency treegraph
 
Reported: 2012-01-10 06:49 PST by Mounir Lamouri (:mounir)
Modified: 2012-09-20 13:16 PDT (History)
14 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix


Attachments
temporary patch (16.98 KB, patch)
2012-06-05 06:30 PDT, Charly Molter :lahabana
bzbarsky: feedback+
roc: feedback+
Details | Diff | Splinter Review
patch that need testing (17.63 KB, patch)
2012-06-07 06:54 PDT, Charly Molter :lahabana
no flags Details | Diff | Splinter Review
patch that should be ok after testing (18.89 KB, text/plain)
2012-06-12 07:44 PDT, Charly Molter :lahabana
no flags Details
Patch that pass all tests (20.98 KB, patch)
2012-06-14 02:05 PDT, Charly Molter :lahabana
no flags Details | Diff | Splinter Review
New version with :roc's comments taken into account (20.22 KB, patch)
2012-06-19 05:52 PDT, Charly Molter :lahabana
roc: review-
Details | Diff | Splinter Review
patch with overflow refactored (18.42 KB, patch)
2012-06-20 14:26 PDT, Charly Molter :lahabana
roc: review+
Details | Diff | Splinter Review
patch without the extra line (18.42 KB, patch)
2012-06-20 15:50 PDT, Charly Molter :lahabana
bzbarsky: review+
Details | Diff | Splinter Review
Patch ready for push (18.57 KB, patch)
2012-06-22 05:01 PDT, Charly Molter :lahabana
no flags Details | Diff | Splinter Review
Patch ready for push (18.57 KB, patch)
2012-06-22 06:07 PDT, Charly Molter :lahabana
no flags Details | Diff | Splinter Review
Patch with comment to getMinSize added (980 bytes, text/plain)
2012-06-25 02:03 PDT, Charly Molter :lahabana
bzbarsky: review-
Details
Backout for beta (56.40 KB, patch)
2012-09-13 14:46 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
bajaj.bhavana: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-01-10 06:49:20 PST
See bug 157846 comment 15 from roc: "Making nsTextControlFrame not be an nsStackFrame would make the code nicer IMHO.".

Doing this might be long but it mostly depend on the methodology I believe. It will also require re-thinking the placeholder implementation.
I remember I did try that some day but I've no idea where the patch could be.

BTW, what would be the best candidate here? nsBlockFrame? nsContainerFrame? Something else?
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-10 12:34:32 PST
nsContainerFrame.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-01-10 19:14:42 PST
Note also the discussion in bug 527459 (which this bug would almost certainly fix).
Comment 3 Charly Molter :lahabana 2012-06-05 06:30:12 PDT
Created attachment 630146 [details] [diff] [review]
temporary patch

This is a temporary patch it is not finished but a look would be nice
Comment 4 Charly Molter :lahabana 2012-06-05 06:35:10 PDT
(In reply to charly.molter from comment #3)
> Created attachment 630146 [details] [diff] [review]
> temporary patch
> 
> This is a temporary patch it is not finished but a look would be nice

Hello,
I've been working on that with alexdmt for a while now and we are starting to get a results. Though it is still quite buggy.
It's our first steps in the layout and as there is quite a lot of work we would like to have a review halfway through.
By a review we mean to be told what we did wrong and what is still lacking or shouldn't be here. 
Here are the things we are the most doubting of:
- the computation of the ascent
- the getPrefWidth which currently returns the containerFrame::GetMinWidth()
Comment 5 Mounir Lamouri (:mounir) 2012-06-05 06:40:11 PDT
You can use the feedback field to ask someone to give some feedback on a patch halfway through.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-06-05 14:20:01 PDT
Comment on attachment 630146 [details] [diff] [review]
temporary patch

> - the getPrefWidth which currently returns the containerFrame::GetMinWidth()

GetMinWidth should be returning nsTextControlFrame::GetPrefWidth.  The old code used to do this right, and even had a comment saying it needed to work that way (which the attached patch removes; please reinstate it).

> - the computation of the ascent

Seems like it should just be nsLayoutUtils::GetCenteredFontBaseline(fontMet, lineHeight) plus the top borderpadding of the input, I would think.

Other than those....

The IsLeaf() impl you removed should stay.  

The handling of min/max-height is wrong-ish.  It should be applied to aReflowState.ComputedHeight(), not computed height plus borderpadding.  That said, there are box-sizing problems there, and inputs use weird box-sizing values...

Apart from those, this looks like a good start!
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-05 15:30:49 PDT
Comment on attachment 630146 [details] [diff] [review]
temporary patch

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

what bz said
Comment 8 Charly Molter :lahabana 2012-06-07 06:54:45 PDT
Created attachment 630955 [details] [diff] [review]
patch that need testing

This is an updated patch with the corrections :bz and :roc asked for.
First testing has been conclusive though we haven't run it through servers :mounir is going to do until I get level 1.
Comment 9 Mounir Lamouri (:mounir) 2012-06-07 06:57:09 PDT
Comment on attachment 630955 [details] [diff] [review]
patch that need testing

Don't r+ your patch until someone did actually r+ it ;)
Comment 10 Mounir Lamouri (:mounir) 2012-06-07 06:58:54 PDT
Sent to try:
https://tbpl.mozilla.org/?tree=Try&rev=fecf96de48c4
Comment 11 Mounir Lamouri (:mounir) 2012-06-07 09:27:46 PDT
Seems like you guys have this assert firing a lot:
###!!! ASSERTION: visual overflow is not in bounds: 'overflow.height <= aDesiredSize.height && overflow.width <= aDesiredSize.width', file ../../../layout/forms/nsTextControlFrame.cpp, line 564

C, J and R are failing on debug builds because of that.

For what I've seen mochitest others failures are not failures. I mean they are marked as failures but no errors are inside the logs. I've requested a re-run to double-check.
Comment 12 Mounir Lamouri (:mounir) 2012-06-11 06:29:21 PDT
Charly sent this to try again on Friday:
https://tbpl.mozilla.org/?tree=Try&rev=3c76b7a5e7a5

Seems like some assertions disappeared which is causing some failures. IMO, updating the expected assertion count would be all right.
Comment 13 Charly Molter :lahabana 2012-06-12 07:44:14 PDT
Created attachment 632252 [details]
patch that should be ok after testing

This patch should be ok the try has just started. It should be all green. 
Some crashtests assert number has been changed I'm not sure if everything is ok though.
Comment 14 Charly Molter :lahabana 2012-06-12 07:46:01 PDT
Here is the try link: https://tbpl.mozilla.org/?tree=Try&rev=cd22dc97bd1f

(In reply to Charly Molter from comment #13)
> Created attachment 632252 [details]
> patch that should be ok after testing
> 
> This patch should be ok the try has just started. It should be all green. 
> Some crashtests assert number has been changed I'm not sure if everything is
> ok though.
Comment 15 Charly Molter :lahabana 2012-06-14 02:05:51 PDT
Created attachment 633071 [details] [diff] [review]
Patch that pass all tests

So after commenting to the wrong bug here is the patch that pass all tests!
We think our code is ready therefore ask for a review.
Here is the tests it passed:
https://tbpl.mozilla.org/?tree=Try&rev=4c52175978b2
Comment 16 Alexandre Dumont 2012-06-14 02:38:06 PDT
(In reply to Charly Molter from comment #15)
> Created attachment 633071 [details] [diff] [review]
> Patch that pass all tests
> 
> So after commenting to the wrong bug here is the patch that pass all tests!
> We think our code is ready therefore ask for a review.
> Here is the tests it passed:
> https://tbpl.mozilla.org/?tree=Try&rev=4c52175978b2

In addition our patch seems to resolve bug 527459 - Padding specified in percent ignored on text inputs.
See https://bugzilla.mozilla.org/show_bug.cgi?id=527459 .
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-18 20:36:56 PDT
Comment on attachment 633071 [details] [diff] [review]
Patch that pass all tests

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

This looks really good.

::: layout/forms/nsTextControlFrame.cpp
@@ +491,3 @@
>                                      aCBSize, aAvailableWidth,
>                                      aMargin, aBorder,
>                                      aPadding, aShrinkWrap);

Fix indent

@@ +516,5 @@
>  
> +  // perform reflow on all kids
> +  nsIFrame* kid = mFrames.FirstChild();
> +  while (kid) {
> +    ReflowTextControlContents(kid, aPresContext, aReflowState, aStatus);

Let's call this ReflowTextControlChild since it only reflows one child, not the whole contents.

@@ +549,5 @@
> +  // child overflow handling
> +  aDesiredSize.SetOverflowAreasToDesiredBounds();
> +  while (kid) {
> +    ConsiderChildOverflow(aDesiredSize.mOverflowAreas, kid);
> +    kid = kid->GetNextSibling();

You can move this up and do it in the same loop as the ReflowTextControlContents call. Actually instead of calling ConsiderChildOverflow, you can combine the overflow returned in 'desiredSize' in ReflowTextControlContents.

@@ +559,5 @@
> +   */
> +  nsRect overflow = aDesiredSize.mOverflowAreas.VisualOverflow();
> +  NS_ASSERTION(overflow.height == aDesiredSize.height && 
> +                overflow.width == aDesiredSize.width, 
> +                "visual overflow is not in bounds");

We probably shouldn't assert this. Just remove it.

@@ +603,3 @@
>  
> +  // reflow the child
> +  nsHTMLReflowMetrics aDesiredSize;  

call this desiredSize since 'a' is for parameters.

::: layout/forms/nsTextControlFrame.h
@@ +165,5 @@
> +   */
> +  void ReflowTextControlContents(nsIFrame*                aFrame,
> +                              nsPresContext*           aPresContext,
> +                              const nsHTMLReflowState& aReflowState,
> +                              nsReflowStatus&          aStatus);

Fix indent

@@ +209,5 @@
>      nsTextControlFrame* mFrame;
>      nsCOMPtr<nsIEditor> mEditor;
>      bool mOuterTransaction;
>      bool mCanceled;
> +

remove blank line
Comment 18 Charly Molter :lahabana 2012-06-19 05:52:42 PDT
Created attachment 634366 [details] [diff] [review]
New version with :roc's comments taken into account

This is the corrected patch. I wasn't sure about what :roc meant about the overflow so I tried to do it but I'm not convinced.
I also put on try servers to be sure that change didn't break anything:
https://tbpl.mozilla.org/?tree=Try&rev=59b00b10a5b2
Comment 19 Mounir Lamouri (:mounir) 2012-06-19 06:01:39 PDT
Comment on attachment 634366 [details] [diff] [review]
New version with :roc's comments taken into account

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

The patch looks great to me but I'm not a layout peer so r+ from me wouldn't have any value ;)

::: layout/forms/nsTextControlFrame.h
@@ -1,5 @@
>  /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> -

nit: don't remove this line.

::: layout/reftests/css-invalid/input/input-maxlength-invalid.html
@@ +4,5 @@
>               it should be affected by :invalid pseudo-class. -->
>    <!-- TODO: this is valid until bug bug 613016 and bug 613019 are fixed. -->
>    <link rel='stylesheet' type='text/css' href='style.css'>
>    <body onload="document.getElementById('i').value='foo'; document.documentElement.className='';">
> +    <input class="notinvalid" maxlength="2" id='i'>

I don't think this change is doing anything.
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-19 17:33:15 PDT
Comment on attachment 634366 [details] [diff] [review]
New version with :roc's comments taken into account

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

::: layout/forms/nsTextControlFrame.cpp
@@ +522,5 @@
> +    ReflowTextControlChild(kid, aPresContext, aReflowState, aStatus, aDesiredSize);
> +    kid = kid->GetNextSibling();
> +  }
> +  // take into account css properties that affect overflow handling
> +  FinishAndStoreOverflow(&aDesiredSize);

You need to call this after we've set up the rest of aDesiredSize.

@@ +559,5 @@
> +nsTextControlFrame::ReflowTextControlChild(nsIFrame*                aKid,
> +                                           nsPresContext*           aPresContext,
> +                                           const nsHTMLReflowState& aReflowState,
> +                                           nsReflowStatus&          aStatus,
> +                                           nsHTMLReflowMetrics aParentDesiredSize)

This has to be a reference!!! Otherwise, this is the right idea.
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-19 17:33:56 PDT
Comment on attachment 634366 [details] [diff] [review]
New version with :roc's comments taken into account

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

oops, didn't mean to r+ this just yet
Comment 22 Charly Molter :lahabana 2012-06-19 17:41:53 PDT
Ho yes indeed that was stupid of me. I'm fixing that right now
Comment 23 Alexandre Dumont 2012-06-19 22:42:15 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> Comment on attachment 634366 [details] [diff] [review]
> New version with :roc's comments taken into account
> 
> Review of attachment 634366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> oops, didn't mean to r+ this just yet

Thank you for your comments Robert. The patch is being fixed as soon as possible.
Comment 24 Charly Molter :lahabana 2012-06-20 14:26:46 PDT
Created attachment 635068 [details] [diff] [review]
patch with overflow refactored

Here is a corrected version of the patch.
The tests seem to pass:
https://tbpl.mozilla.org/?tree=Try&rev=edcc442465f2
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-20 15:00:57 PDT
Comment on attachment 635068 [details] [diff] [review]
patch with overflow refactored

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

::: layout/forms/nsTextControlFrame.cpp
@@ +537,5 @@
> +  aDesiredSize.ascent = 
> +        nsLayoutUtils::GetCenteredFontBaseline(fontMet, lineHeight) 
> +        + aReflowState.mComputedBorderPadding.top;
> +
> +

remove unnecessary blank line
Comment 26 Charly Molter :lahabana 2012-06-20 15:50:43 PDT
Created attachment 635105 [details] [diff] [review]
patch without the extra line

I don't ask roc for review anymore as he "+" the last one I just removed the extra line he asked for
Comment 27 Mounir Lamouri (:mounir) 2012-06-21 09:59:42 PDT
(In reply to Charly Molter :lahabana from comment #26)
> Created attachment 635105 [details] [diff] [review]
> patch without the extra line
> 
> I don't ask roc for review anymore as he "+" the last one I just removed the
> extra line he asked for

With roc's r+, you actually don't need any other review (one peer is enough).
Is this patch passing try? Let me know so I can push it to mozilla-inbound.
Comment 28 Alexandre Dumont 2012-06-21 10:13:42 PDT
(In reply to Mounir Lamouri (:mounir) from comment #27)
> (In reply to Charly Molter :lahabana from comment #26)
> > Created attachment 635105 [details] [diff] [review]
> > patch without the extra line
> > 
> > I don't ask roc for review anymore as he "+" the last one I just removed the
> > extra line he asked for
> 
> With roc's r+, you actually don't need any other review (one peer is enough).
> Is this patch passing try? Let me know so I can push it to mozilla-inbound.

Here are the results on try servers. All tests seem to pass succesfully.
https://tbpl.mozilla.org/?tree=Try&rev=edcc442465f2
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2012-06-21 14:18:51 PDT
Comment on attachment 635105 [details] [diff] [review]
patch without the extra line

> NS_IMETHODIMP
> nsTextControlFrame::Reflow(nsPresContext*   aPresContext,
>+  NS_ENSURE_SUCCESS(rv, 0);

That's wrong.  The method it came from returned a length, so 0 made sense.  Please replace the 0 with "rv".

>+nsTextControlFrame::ReflowTextControlChild(nsIFrame*                aKid,

>+  nsSize availSize(aReflowState.ComputedWidth(), 
>+                  aReflowState.ComputedHeight());

Fix the indent, please.

>+  nsHTMLReflowState reflowState(aPresContext, aReflowState, aKid,

Please name this kidReflowState?

>+  nscoord xOffset = aReflowState.mComputedBorderPadding.left;

This needs to include kidReflowState.mComputedMargin.left.  yoffset needs to include kidReflowState.mComputedMargin.top.

Please add a reftest for the percentage padding thing this patch fixes.  Doing that as a separate patch in that other bug is fine.

r=me with the above nits fixed.  This looks great!
Comment 30 Charly Molter :lahabana 2012-06-22 05:01:06 PDT
Created attachment 635697 [details] [diff] [review]
Patch ready for push

Here is the patch corrected I sent it to try just to check:
https://tbpl.mozilla.org/?tree=Try&rev=8576f8c9b758
Everything seems ok
I'm writing a reftest for the padding I'll submit it on the other bug
Comment 31 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-22 05:49:01 PDT
Comment on attachment 635697 [details] [diff] [review]
Patch ready for push

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

Just fix that, then we can land this :-)

::: layout/forms/nsTextControlFrame.cpp
@@ +563,4 @@
>  {
> +  // compute available size and frame offsets for child
> +  nsSize availSize(aReflowState.ComputedWidth(), 
> +                    aReflowState.ComputedHeight());

The indent seems to be one space too far to the right now :-)
Comment 32 Charly Molter :lahabana 2012-06-22 06:07:29 PDT
Created attachment 635718 [details] [diff] [review]
Patch ready for push

Here it is
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2012-06-22 12:59:44 PDT
For future reference, you probably want a User line and a checkin comment in your patches.  Makes it harder for people to screw up pushing them.  ;)

Did you resolve the issues with the urlbar textfield you were seeing?  I don't see anything obvious in this patch that would handle that....
Comment 34 Mounir Lamouri (:mounir) 2012-06-23 04:12:58 PDT
Pushed in mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47bb129f9fbe

It should be synced with mozilla-central soon.
Comment 35 Ryan VanderMeulen [:RyanVM] 2012-06-23 19:57:16 PDT
https://hg.mozilla.org/mozilla-central/rev/47bb129f9fbe
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2012-06-23 20:28:20 PDT
Um.  The reason I didn't push this on Friday is because I was waiting for an answer to my question from comment 33....  I would still appreciate an answer to that.
Comment 37 Charly Molter :lahabana 2012-06-24 01:52:50 PDT
(In reply to Boris Zbarsky (:bz) from comment #33)
> For future reference, you probably want a User line and a checkin comment in
> your patches.  Makes it harder for people to screw up pushing them.  ;)
> 
> Did you resolve the issues with the urlbar textfield you were seeing?  I
> don't see anything obvious in this patch that would handle that....
We resolved it by readding getMinSize() cause it was defined differently in nsBoxFrame
Comment 38 Boris Zbarsky [:bz] (still a bit busy) 2012-06-24 07:44:30 PDT
> We resolved it by readding getMinSize() cause it was defined differently in nsBoxFrame

OK.  Then you should have added a comment saying that the implementation of GetMinSize() was the way it was on purpose and what that purpose was...  Please post a patch with that comment?
Comment 39 Mounir Lamouri (:mounir) 2012-06-24 08:25:31 PDT
(In reply to Boris Zbarsky (:bz) from comment #36)
> Um.  The reason I didn't push this on Friday is because I was waiting for an
> answer to my question from comment 33....  I would still appreciate an
> answer to that.

They told me that they solved that issue on IRC some time ago. I should have make that clear before pushing the patch. Sorry :(
Comment 40 Boris Zbarsky [:bz] (still a bit busy) 2012-06-24 12:18:04 PDT
No problem.  Let's just get that comment added?
Comment 41 Charly Molter :lahabana 2012-06-24 12:31:48 PDT
(In reply to Boris Zbarsky (:bz) from comment #40)
> No problem.  Let's just get that comment added?

Yes I don't have time right now I will do it tomorrow
Comment 42 Charly Molter :lahabana 2012-06-25 02:03:09 PDT
Created attachment 636251 [details]
Patch with comment to getMinSize added

Here is a patch with the comment that bz asked for.
Is all the format right this time? (user that modified it, commit message...)?
Comment 43 Boris Zbarsky [:bz] (still a bit busy) 2012-06-25 11:19:07 PDT
Comment on attachment 636251 [details]
Patch with comment to getMinSize added

> correcting the comment on nsTextControlFrame::GetMinSize

Please cite the bug number, like so:

  Bug 716875.  Document why nsTextControlFrame::GetMinSize exists.

> +  // This is to enable xul inputs to resize to smaller than defaults input's
> min size

Apart from the grammar nit (s/default/defaults/) it would be good to explain _why_ that's needed.  E.g. that particular XBL bindings or particular XUL apps depend on that behavior.  That way someone who wants to change it can just test whether they're breaking the things you're trying to not break here.
Comment 44 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-09-13 14:46:33 PDT
Created attachment 660999 [details] [diff] [review]
Backout for beta

This backs out bugs 716875, 776265, 789007, 785754, and 782660 on beta.

Does this look alright to you, Mats?
Comment 45 Mats Palmgren (:mats) 2012-09-13 15:47:08 PDT
Comment on attachment 660999 [details] [diff] [review]
Backout for beta

Yes, I verified I got the same result.
Comment 46 David Baron :dbaron: ⌚️UTC-10 2012-09-13 15:54:39 PDT
What bug(s) is the backout fixing?
Comment 47 Mats Palmgren (:mats) 2012-09-13 15:59:30 PDT
Bug 787274 is the one we don't know how to fix.
Comment 48 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-09-13 16:44:40 PDT
I think we should fix bug 787274 on trunk and Aurora. I hope we only need to back this out on beta.
Comment 49 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-09-13 16:46:11 PDT
Comment on attachment 660999 [details] [diff] [review]
Backout for beta

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

Back this stuff out on Beta to fix regression bug 787274.

There's a few bugs being backed out here, but the backout seems lower risk than trying to fix 787274 (and other unknown regressions?) on beta.
Comment 50 bhavana bajaj [:bajaj] 2012-09-14 15:19:35 PDT
Approving now. Please update the status flags once it is backed out.

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