nsTextControlFrame shouldn't inherit from nsStackFrame

RESOLVED FIXED in mozilla17

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mounir, Assigned: lahabana)

Tracking

Trunk
mozilla17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox16+ wontfix)

Details

(Whiteboard: [mentor=mounir][lang=c++])

Attachments

(3 attachments, 8 obsolete attachments)

(Reporter)

Description

6 years ago
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?
nsContainerFrame.
Note also the discussion in bug 527459 (which this bug would almost certainly fix).
Blocks: 527459
(Reporter)

Updated

5 years ago
Whiteboard: [mentor=volkmar][lang=c++] → [mentor=mounir][lang=c++]
(Assignee)

Comment 3

5 years ago
Created attachment 630146 [details] [diff] [review]
temporary patch

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

Comment 4

5 years ago
(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()
(Reporter)

Updated

5 years ago
Attachment #630146 - Attachment is patch: true
(Reporter)

Comment 5

5 years ago
You can use the feedback field to ask someone to give some feedback on a patch halfway through.
(Assignee)

Updated

5 years ago
Attachment #630146 - Flags: feedback+
(Assignee)

Updated

5 years ago
Attachment #630146 - Flags: feedback?(bzbarsky)
Attachment #630146 - Flags: feedback+
(Assignee)

Updated

5 years ago
Attachment #630146 - Flags: feedback?(bzbarsky) → feedback?(roc)
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!
Attachment #630146 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 630146 [details] [diff] [review]
temporary patch

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

what bz said
Attachment #630146 - Flags: feedback?(roc) → feedback+
(Assignee)

Comment 8

5 years ago
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.
Attachment #630146 - Attachment is obsolete: true
Attachment #630955 - Flags: review+
(Reporter)

Comment 9

5 years ago
Comment on attachment 630955 [details] [diff] [review]
patch that need testing

Don't r+ your patch until someone did actually r+ it ;)
Attachment #630955 - Flags: review+
(Reporter)

Comment 10

5 years ago
Sent to try:
https://tbpl.mozilla.org/?tree=Try&rev=fecf96de48c4
(Reporter)

Comment 11

5 years ago
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.
(Reporter)

Comment 12

5 years ago
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.
(Assignee)

Comment 13

5 years ago
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.
Attachment #630955 - Attachment is obsolete: true
Attachment #632252 - Flags: review?(roc)
Attachment #632252 - Flags: review?(bzbarsky)
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Comment 15

5 years ago
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
Attachment #632252 - Attachment is obsolete: true
Attachment #632252 - Flags: review?(roc)
Attachment #632252 - Flags: review?(bzbarsky)
Attachment #633071 - Flags: review?(roc)
Attachment #633071 - Flags: review?(bzbarsky)

Comment 16

5 years ago
(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 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
(Assignee)

Comment 18

5 years ago
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
Attachment #633071 - Attachment is obsolete: true
Attachment #633071 - Flags: review?(roc)
Attachment #633071 - Flags: review?(bzbarsky)
Attachment #634366 - Flags: review?(roc)
Attachment #634366 - Flags: review?(mounir)
Attachment #634366 - Flags: review?(bzbarsky)
(Reporter)

Comment 19

5 years ago
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.
Attachment #634366 - Flags: review?(mounir)
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.
Attachment #634366 - Flags: review?(roc) → review+
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
Attachment #634366 - Flags: review+ → review-
(Assignee)

Comment 22

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

Comment 23

5 years ago
(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.
(Assignee)

Updated

5 years ago
Assignee: nobody → charly.molter
(Assignee)

Comment 24

5 years ago
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
Attachment #634366 - Attachment is obsolete: true
Attachment #634366 - Flags: review?(bzbarsky)
Attachment #635068 - Flags: review?(roc)
Attachment #635068 - Flags: review?(bzbarsky)
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
Attachment #635068 - Flags: review?(roc) → review+
(Assignee)

Comment 26

5 years ago
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
Attachment #635068 - Attachment is obsolete: true
Attachment #635068 - Flags: review?(bzbarsky)
Attachment #635105 - Flags: review?(bzbarsky)
(Reporter)

Comment 27

5 years ago
(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

5 years ago
(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 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!
Attachment #635105 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 30

5 years ago
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
Attachment #635105 - Attachment is obsolete: true
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 :-)
(Assignee)

Comment 32

5 years ago
Created attachment 635718 [details] [diff] [review]
Patch ready for push

Here it is
Attachment #635697 - Attachment is obsolete: true
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....
(Reporter)

Comment 34

5 years ago
Pushed in mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47bb129f9fbe

It should be synced with mozilla-central soon.
Status: NEW → ASSIGNED
Flags: in-testsuite-
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/47bb129f9fbe
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
(Assignee)

Comment 37

5 years ago
(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
> 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?
(Reporter)

Comment 39

5 years ago
(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 :(
No problem.  Let's just get that comment added?
(Assignee)

Comment 41

5 years ago
(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
(Assignee)

Comment 42

5 years ago
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...)?
Attachment #636251 - Flags: review?(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.
Attachment #636251 - Flags: review?(bzbarsky) → review-
Depends on: 776265
Depends on: 782660
Blocks: 787274
No longer blocks: 787274
Depends on: 787274
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?
Attachment #660999 - Flags: review?(matspal)
Comment on attachment 660999 [details] [diff] [review]
Backout for beta

Yes, I verified I got the same result.
Attachment #660999 - Flags: review?(matspal) → review+
What bug(s) is the backout fixing?
Bug 787274 is the one we don't know how to fix.
I think we should fix bug 787274 on trunk and Aurora. I hope we only need to back this out on beta.
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.
Attachment #660999 - Flags: approval-mozilla-beta?
Approving now. Please update the status flags once it is backed out.
tracking-firefox16: --- → +

Updated

5 years ago
Attachment #660999 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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

Updated

5 years ago
status-firefox16: --- → affected
status-firefox16: affected → fixed
status-firefox16: fixed → wontfix
Target Milestone: mozilla16 → mozilla17
You need to log in before you can comment on or make changes to this bug.