716875 - nsTextControlFrame shouldn't inherit from nsStackFrame

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Mounir Lamouri (:mounir)]

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)]

nsContainerFrame.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note also the discussion in bug 527459 (which this bug would almost certainly fix).

Comment 3

[Charly Molter :lahabana]

Attachment: temporary patch

This is a temporary patch it is not finished but a look would be nice

Comment 4

[Charly Molter :lahabana]

(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)]

You can use the feedback field to ask someone to give some feedback on a patch halfway through.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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)]

Comment on attachment 630146 [details] [diff] [review]
temporary patch

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

what bz said

Comment 8

[Charly Molter :lahabana]

Attachment: 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)]

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)]

Sent to try:
https://tbpl.mozilla.org/?tree=Try&rev=fecf96de48c4

Comment 11

[Mounir Lamouri (:mounir)]

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)]

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]

Attachment: 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]

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]

Attachment: 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

[u443890]

(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)]

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]

Attachment: 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)]

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)]

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)]

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]

Ho yes indeed that was stupid of me. I'm fixing that right now

Comment 23

[u443890]

(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]

Attachment: 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)]

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]

Attachment: 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)]

(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

[u443890]

(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 [:bzbarsky]]

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]

Attachment: 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)]

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]

Attachment: Patch ready for push

Here it is

Comment 33

[Boris Zbarsky [:bzbarsky]]

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)]

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]]

https://hg.mozilla.org/mozilla-central/rev/47bb129f9fbe

Comment 36

[Boris Zbarsky [:bzbarsky]]

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]

(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 [:bzbarsky]]

> 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)]

(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 [:bzbarsky]]

No problem.  Let's just get that comment added?

Comment 41

[Charly Molter :lahabana]

(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]

Attachment: 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 [:bzbarsky]]

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)]

Attachment: 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 (inactive)]

Comment on attachment 660999 [details] [diff] [review]
Backout for beta

Yes, I verified I got the same result.

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What bug(s) is the backout fixing?

Comment 47

[Mats Palmgren (inactive)]

Bug 787274 is the one we don't know how to fix.

Comment 48

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

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)]

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]]

Approving now. Please update the status flags once it is backed out.

Comment 51

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

Backed out on beta.

https://hg.mozilla.org/releases/mozilla-beta/rev/6f7d29816c57
https://hg.mozilla.org/releases/mozilla-beta/rev/2db84458c7c3
https://hg.mozilla.org/releases/mozilla-beta/rev/93b1aaf8376b
https://hg.mozilla.org/releases/mozilla-beta/rev/6d13c5f78d00
https://hg.mozilla.org/releases/mozilla-beta/rev/73df8ef996a7