Bug 32077 – textarea grows when you type

• Component: Forms
• Status: RESOLVED CONFIGURATION CHANGED

[Ojan Vafai]

Created attachment 44164 [details]
test case

Load the test case. Type a letter in the textarea and see that it grows. Happens in Safari 4 and in today's nightly.

[Mark Rowe (bdash)]

<rdar://problem/7437752>

[Hajime Morrita]

Created attachment 50190 [details]
patch v0

Comment on attachment 50190 [details]
patch v0

It is incorrect to fix up the layout root during FrameView::layout() like this. The repeated calls to setNeedsLayout(), which dirties ancestor, from within walkToLayoutSafeAscent() make no sense to me. It seems like you may end up marking all the way to the RenderView but still return one of its descendants as the new layout root, and consequently end up with a dirty tree after layout.

If the textarea cannot be the layout root, the right place to fix this is in isRelayoutBoundary(), not after the fact.

However, I don’t see why the texture cannot be the layout root, as anything inside the textarea cannot affect the layout of anything outside it. The problem here is that the height calculation is performed for the textarea when its containers are already laid out and have their final widths, leading to different results (only because of the quirk that allows percentage heights to be computed based off the height of the auto-height body element, and it’s questionable if even that should work the way it does). To fix this, you should do something similar to what RenderBox::calcWidth() does (for similar reasons). I don’t think the exact same thing in RenderBox::calcHeight() will work, but you can ensure that RenderBlock::layout() restores the height to its previous value (and skips the call to calcHeight()) if this is the layout root.

> I don’t see why the texture cannot be the layout root, as
s/texture/textarea/

[Hajime Morrita]

Created attachment 50203 [details]
patch v1; preserve original height when the renderer is layout-root

[Hajime Morrita]

Thank you for reviewing quickly!

> I don’t think the exact same thing in RenderBox::calcHeight() will work, but
> you can ensure that RenderBlock::layout() restores the height to its previous
> value (and skips the call to calcHeight()) if this is the layout root.
This looks fine. Doing so would be robust for edge cases.
So I updated the patch to go this way. Would you review the updated one?

If this is OK, I'll refactor RenderBox::calcWidth() in anotehr patch,
to move root-layout check out to layoutBlock(). 
Doing so  will allow to share the predicate with calcHeight().

Comment on attachment 50203 [details]
patch v1; preserve original height when the renderer is layout-root

> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,32 @@
> [phone]  MORITA Hajime  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Textarea grows when you type.
> +        https://bugs.webkit.org/show_bug.cgi?id=32077
> +
> +        Fix to prevent box height change on re-layout with the renderer of
> +        the box as a root. RenderBlock with pecent-specified 'height'
> +        propety (and family) did cause different box height values between
> +        full-layout and partial layout with the renderer as a layout-root.
> +        This fix restores original height in such case.

The LayoutTests change log need not contain a description of the fix.

> +<script src="./resources/textarea-percentage-dimensions.js"></script>

You can omit the “./”.

> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,29 @@
> [phone]  MORITA Hajime  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Textarea grows when you type.
> +        https://bugs.webkit.org/show_bug.cgi?id=32077
> +
> +        Fix to prevent box height change on re-layout with the renderer of
> +        the box as a root. RenderBlock with pecent-specified 'height'

Typo: “pecent”.

> +        propety (and family) did cause different box height values between

Typo: “propety”. I don’t understand “and family”.

> +        full-layout and partial layout with the renderer as a layout-root.
> +        This fix restores original height in such case.

I think the description could be improved. The main points, as I see them, are the specific case where percentage height calculation depends on the state of ancestors during their layout, and the fact that by definition, the height of the layout root must not change during layout, or else it wouldn’t be a valid layout root.

> +    bool isLayoutRoot = node() && view()->frameView() && view()->frameView()->layoutRoot(true) == this;

Using a local variable for this seems redundant. Why the check for node()?

>      int oldHeight = height();
> -    calcHeight();
> +
> +    if (isLayoutRoot)
> +        setHeight(previousHeight);
> +    else
> +        calcHeight();
> +
>      if (oldHeight != height()) {

This is suboptimal. If you skip the height calculation, it seems like you could skip the (oldHeight != height()) test and the previousHeight != height() test that follows. Those could be in the else block with calcHeight().

[Hajime Morrita]

Created attachment 50277 [details]
v2; follow the feedback.

[Hajime Morrita]

Thank you for the feedback! I've updated the patch.

> The LayoutTests change log need not contain a description of the fix.
Fix to put test description instead.

> 
> > +<script src="./resources/textarea-percentage-dimensions.js"></script>
> 
> You can omit the “./”.
Fixed.

> Typo: “pecent”.
(...)
> Typo: “propety”. I don’t understand “and family”.
Fixed. I'm sorry for that...

> I think the description could be improved. The main points, as I see them, are
> the specific case where percentage height calculation depends on the state of
> ancestors during their layout, and the fact that by definition, the height of
> the layout root must not change during layout, or else it wouldn’t be a valid
> layout root.
I rewrote the description on ChangeLog. Thank you for your suggestion!

> 
> > +    bool isLayoutRoot = node() && view()->frameView() && view()->frameView()->layoutRoot(true) == this;
> 
> Using a local variable for this seems redundant. Why the check for node()?
Oops. The conditional was just copied from RenderBox::calcWidth(). I removed redundant check against node().
And I also moved layout-root check from RenderBox::calcWidth() to RenderBlock::layoutBlock() to share the criteria.
between the check for calcHeight().

> 
> >      int oldHeight = height();
> > -    calcHeight();
> > +
> > +    if (isLayoutRoot)
> > +        setHeight(previousHeight);
> > +    else
> > +        calcHeight();
> > +
> >      if (oldHeight != height()) {
> 
> This is suboptimal. If you skip the height calculation, it seems like you could
> skip the (oldHeight != height()) test and the previousHeight != height() test
> that follows. Those could be in the else block with calcHeight().
Fixed. 

Would you review the patch again?

Comment on attachment 50277 [details]
v2; follow the feedback.

Thanks for updating the patch. I wish you didn’t change calcWidth(). It’s not part of the fix for this bug, and I don’t see why it needs to be made specific to RenderBlock instead of all RenderBoxes.

[Hajime Morrita]

Created attachment 50690 [details]
take the feedback

[Hajime Morrita]

Thank you for reviewing!  I've updated the patch.

> Thanks for updating the patch. I wish you didn’t change calcWidth(). It’s not
> part of the fix for this bug, 
Agreed. 
To sharing the conditional, I just extract the predicate to
RenderBox::shouldComputeOwnSize() and use it from layoutBlock()

> and I don’t see why it needs to be made specific
> to RenderBlock instead of all RenderBoxes.
I think, this is because objectIsRelayoutBoundary() takes RenderTextControl as 
a special case, and RenderTextControl is a subclass of RenderBlock.
For other boxes,  height properties in percentage prevent the RO
from becoming a layout root.

Perhaps a simpler fix would be to add the percent height check to the text control branch of the condition in isRelayoutBoubdary() then?

[Hajime Morrita]

Created attachment 50763 [details]
switch to change objectIsRelayoutBoundary()

[Hajime Morrita]

Thank you for reviewing again! I've updated the patch.

> Perhaps a simpler fix would be to add the percent height check to the text
> control branch of the condition in isRelayoutBoubdary() then?
I've been concerned that changing relayout boundary criteria might increase
size of relayout nodes, which get possible performance penalty.
But yes, it will be a rare case, and localizing such a decision inside 
objectIsRelayoutBoubdary() simplifies the code, as you said.

[WebKit Review Bot]

Attachment 50763 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderObject.h:965:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hajime Morrita]

Created attachment 50764 [details]
fix style violation

[Shinichiro Hamaji]

Comment on attachment 50763 [details]
switch to change objectIsRelayoutBoundary()

Some style nitpicks for your tests.

> diff --git a/LayoutTests/fast/forms/resources/textarea-percentage-dimensions.js b/LayoutTests/fast/forms/resources/textarea-percentage-dimensions.js
> new file mode 100644

Cannot we use script-tests ?

> index 0000000..e64c9ea
> --- /dev/null
> +++ b/LayoutTests/fast/forms/resources/textarea-percentage-dimensions.js
> @@ -0,0 +1,26 @@
> +
> +if (window.layoutTestController)
> +    layoutTestController.dumpAsText();

The first line seems to be unnecessary.

> diff --git a/LayoutTests/fast/forms/textarea-percentage-dimension-height-expected.txt b/LayoutTests/fast/forms/textarea-percentage-dimension-height-expected.txt
> new file mode 100644
> index 0000000..16fa0ef
> --- /dev/null
> +++ b/LayoutTests/fast/forms/textarea-percentage-dimension-height-expected.txt
> @@ -0,0 +1,4 @@
> +
> +a
> +
> +PASS

I don't understand this issue, but if it's possible to use another message instead of "a", it would be nicer to have more meaningful messages.

> diff --git a/WebCore/rendering/RenderObject.h b/WebCore/rendering/RenderObject.h
> index f7b460a..85a2f4b 100644
> --- a/WebCore/rendering/RenderObject.h
> +++ b/WebCore/rendering/RenderObject.h
> @@ -954,13 +954,19 @@ inline bool objectIsRelayoutBoundary(const RenderObject *obj)
>  {
>      // FIXME: In future it may be possible to broaden this condition in order to improve performance.
>      // Table cells are excluded because even when their CSS height is fixed, their height()
> -    // may depend on their contents.
> -    return obj->isTextControl()
> -        || (obj->hasOverflowClip() && !obj->style()->width().isIntrinsicOrAuto() && !obj->style()->height().isIntrinsicOrAuto() && !obj->style()->height().isPercent() && !obj->isTableCell())
> +    // may also depend on their contents.
> +    bool hasContentsDependHeight = ((obj->style()->height().isIntrinsicOrAuto() || obj->style()->height().isPercent()) 
> +                                 || (obj->style()->minHeight().isIntrinsicOrAuto() || obj->style()->minHeight().isPercent())
> +                                 || (obj->style()->maxHeight().isIntrinsicOrAuto() || obj->style()->maxHeight().isPercent()));
> +
> +        ;

It seems like we can remove this line and it's not a false positive of style checker?

[Shinichiro Hamaji]

> It seems like we can remove this line and it's not a false positive of style
> checker?

Oops. I've looked at wrong version of your patch! Please just ignore this comment.

[Hajime Morrita]

Created attachment 50892 [details]
replaced tests with script-tests

[Shinichiro Hamaji]

Comment on attachment 50892 [details]
replaced tests with script-tests

> +
> +description('Check if the height of the textarea does not change even after text is inserted.');

Unnecessary blank line?

> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> +<html>
> +<head>
> +<link rel="stylesheet" href="../../fast/js/resources/js-test-style.css">
> +<script src="../../fast/js/resources/js-test-pre.js"></script>
> +</head>
> +<body>
> +
> +    <div>
> +      <textarea id="target" style="height: 100%;"></textarea> 
> +    </div>
> +    <p>(placeholder for the layout. you can ignore this.)</p><span id="log"></span>
> +
> +<p id="description"></p>
> +<div id="console"></div>
> +<script src="script-tests/textarea-percentage-dimensions.js"></script>

When we use script-tests, the HTML should be generated by TEMPLATE.html. I didn't notice the textarea-percentage-dimensions.js is referred by multiple HTMLs... I'm sorry, I think we shouldn't use the script-tests framework in this change.

[Hajime Morrita]

Created attachment 51394 [details]
revert last patch, with small fix on text in tests

[Hajime Morrita]

> didn't notice the textarea-percentage-dimensions.js is referred by multiple
> HTMLs... I'm sorry, I think we shouldn't use the script-tests framework in this
> change.
Fixed. I'm sorry that I didn't understand script-tests mechanism.

[Kent Tamura]

Comment on attachment 51394 [details]
revert last patch, with small fix on text in tests

> +        Add test for the case with various type of properties for vertical box size.
> +        including height, min-height, max-height, padding-top/bottom, margin-top/bottom.
> +        
> +        * fast/forms/resources/textarea-percentage-dimensions.js: Added.
> +        * fast/forms/textarea-percentage-dimension-height-expected.txt: Added.
> +        * fast/forms/textarea-percentage-dimension-height.html: Added.
> +        * fast/forms/textarea-percentage-dimension-margin-bottom-expected.txt: Added.
> +        * fast/forms/textarea-percentage-dimension-margin-bottom.html: Added.
> +        * fast/forms/textarea-percentage-dimension-margin-top-expected.txt: Added.
> +        * fast/forms/textarea-percentage-dimension-margin-top.html: Added.
> +        * fast/forms/textarea-percentage-dimension-max-height-expected.txt: Added.
> +        * fast/forms/textarea-percentage-dimension-max-height.html: Added.
> +        * fast/forms/textarea-percentage-dimension-min-height-expected.txt: Added.
> +        * fast/forms/textarea-percentage-dimension-min-height.html: Added.
> +        * fast/forms/textarea-percentage-dimension-padding-bottom-expected.txt: Added.
> +        * fast/forms/textarea-percentage-dimension-padding-bottom.html: Added.
> +        * fast/forms/textarea-percentage-dimension-padding-top-expected.txt: Added.
> +        * fast/forms/textarea-percentage-dimension-padding-top.html: Added.

Can you merge these tests into one, and convert it to script-tests?
You can generate each of test target HTMLs like the following.

var parent = document.createElement('div');
document.body.appendChild(parent);
parent.innerHTML = '<div><textarea ....'

[Hajime Morrita]

Created attachment 56667 [details]
gathered tests into one script-test

[Hajime Morrita]

Hi Kent-san, thank you for reviewing this long-lived patch!

(In reply to comment #24)
> Can you merge these tests into one, and convert it to script-tests?
> You can generate each of test target HTMLs like the following.
> (snip)
Using innerHTML sounds good. Fixed to do such .

[Kent Tamura]

Comment on attachment 56667 [details]
gathered tests into one script-test

The patch contains no test expectation files.

[Kent Tamura]

Comment on attachment 56667 [details]
gathered tests into one script-test

LayoutTests/fast/forms/script-tests/textarea-percentage-dimensions.js:29
 +  \ No newline at end of file
Please fix

[Shinichiro Hamaji]

Comment on attachment 56667 [details]
gathered tests into one script-test

LayoutTests/fast/forms/script-tests/textarea-percentage-dimensions.js:1
 +  
An unnecessary blank line?

WebCore/ChangeLog:12
 +          its layout, but the this assumption get violated when the
"the this" => "this" ?

WebCore/ChangeLog:11
 +          is because calcHeight() assumes that their ancestors are during
Not sure if it's OK as English. Maybe "calcHeight() assumes the existence of its ancestors during its layout"? Or, is the "their" different from "its"?

[Hajime Morrita]

Created attachment 56671 [details]
took feedbacks

[Hajime Morrita]

Kent-san, Hamaji-san, thank you for reviewing quickly.

(In reply to comment #27)
> (From update of attachment 56667 [details])
> The patch contains no test expectation files.
Oops. Added it. I'm sorry for that.

(In reply to comment #28)
> (From update of attachment 56667 [details])
> LayoutTests/fast/forms/script-tests/textarea-percentage-dimensions.js:29
>  +  \ No newline at end of file
Fixed.


(In reply to comment #29)
> (From update of attachment 56667 [details])
> LayoutTests/fast/forms/script-tests/textarea-percentage-dimensions.js:1
>  +  
> An unnecessary blank line?
Fixed.

> 
> WebCore/ChangeLog:12
>  +          its layout, but the this assumption get violated when the
> "the this" => "this" ?
Fixed.

> 
> WebCore/ChangeLog:11
>  +          is because calcHeight() assumes that their ancestors are during
> Not sure if it's OK as English. Maybe "calcHeight() assumes the existence of its ancestors during its layout"? Or, is the "their" different from "its"?
I agree that the description was confusing. I tried to clarify that part.

[Kent Tamura]

Comment on attachment 56671 [details]
took feedbacks

LayoutTests/ChangeLog:13
 +          * fast/forms/textarea-percentage-dimensions.html: Added.
You need to update ChangeLog for textarea-percentage-dimensions-expected.txt.


LayoutTests/fast/forms/script-tests/textarea-percentage-dimensions.js:13
 +      shouldBe("window.heightBeforeInsert", "window.heightAfterInsert");
If one of the test cases failed, it's not easy to identify what test case failed.
Would you improve test result readabily?  e.g.  adding debug("A test for " + style); before shouldBe().



LayoutTests/ChangeLog:5
 +          Textarea grows when you type.
The 1-line description is unclear.   Textarea should grow when you type? Textarea should not grow when you type?

[Darin Adler]

(In reply to comment #32)
> LayoutTests/fast/forms/script-tests/textarea-percentage-dimensions.js:13
>  +      shouldBe("window.heightBeforeInsert", "window.heightAfterInsert");
> If one of the test cases failed, it's not easy to identify what test case failed.
> Would you improve test result readabily?  e.g.  adding debug("A test for " + style); before shouldBe().

An even better way to improve the readability is to write a function that performs the test and then just use shouldBe, maybe a function that returns the change in height like this:

    shouldBe("heightChange('height:70%;')", "0");

[Hajime Morrita]

Created attachment 56841 [details]
tool feedbacks

[Hajime Morrita]

Kent-san, Darin, thank you for your advice!
I've updated the patch.

> (From update of attachment 56671 [details])
> LayoutTests/ChangeLog:13
>  +          * fast/forms/textarea-percentage-dimensions.html: Added.
> You need to update ChangeLog for textarea-percentage-dimensions-expected.txt.
Added.

> LayoutTests/fast/forms/script-tests/textarea-percentage-dimensions.js:13
>  +      shouldBe("window.heightBeforeInsert", "window.heightAfterInsert");
> If one of the test cases failed, it's not easy to identify what test case failed.
> Would you improve test result readabily?  e.g.  adding debug("A test for " + style); before shouldBe().
Fixed following Darin's advice.

> LayoutTests/ChangeLog:5
>  +          Textarea grows when you type.
> The 1-line description is unclear.   Textarea should grow when you type? Textarea should not grow when you type?
Fixed.

[Kent Tamura]

Comment on attachment 56841 [details]
tool feedbacks

LayoutTests/fast/forms/script-tests/textarea-percentage-dimensions.js:20
 +  shouldBe('heightChanged("height:70%;")', "0");
nit: I like heightChanged() returns a boolean and using shouldBeFalse().

[Hajime Morrita]

Comment on attachment 56841 [details]
tool feedbacks

Clearing flags on attachment: 56841

Committed r60060: <http://trac.webkit.org/changeset/60060>

[Hajime Morrita]

Rolled out at http://trac.webkit.org/changeset/60062 because it breaks follwoing pixel tests:

 - fast/repaint/moving-shadow-on-container.html
 - fast/repaint/moving-shadow-on-path.html
 - fast/repaint/search-field-cancel.html

[Hajime Morrita]

Created attachment 56853 [details]
fix pixel test failure

[WebKit Review Bot]

Attachment 56853 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/RenderObject.h:988:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hajime Morrita]

Created attachment 56855 [details]
attempt to fix style violation

[Hajime Morrita]

> 
>  - fast/repaint/moving-shadow-on-container.html
>  - fast/repaint/moving-shadow-on-path.html
>  - fast/repaint/search-field-cancel.html
The landed patch broke some pixel tests
because it excluded SVG root objects from the relayout boundary.
I updated the patch to include SVG root as a relayout root.
Now pixel tests are going to pass.

[WebKit Review Bot]

Attachment 56855 [details] did not pass style-queue:

Failed to run "[u'/mnt/git/webkit-style-queue/WebKitTools/Scripts/check-webkit-style', '--use-patch-as-diff', '/tmp/tmpc5K_Yp.patch']" exit_code: 2
Last 3072 characters of output:
use

Paths:
  Certain style-checking behavior depends on the paths relative to
  the WebKit source root of the files being checked.  For example,
  certain types of errors may be handled differently for files in
  WebKit/gtk/webkit/ (e.g. by suppressing "readability/naming" errors
  for files in this directory).

  Consequently, if the path relative to the source root cannot be
  determined for a file being checked, then style checking may not
  work correctly for that file.  This can occur, for example, if no
  WebKit checkout can be found, or if the source root can be detected,
  but one of the files being checked lies outside the source tree.

  If a WebKit checkout can be detected and all files being checked
  are in the source tree, then all paths will automatically be
  converted to paths relative to the source root prior to checking.
  This is also useful for display purposes.

  Currently, this command can detect the source root only if the
  command is run from within a WebKit checkout (i.e. if the current
  working directory is below the root of a checkout).  In particular,
  it is not recommended to run this script from a directory outside
  a checkout.

  Running this script from a top-level WebKit source directory and
  checking only files in the source tree will ensure that all style
  checking behaves correctly -- whether or not a checkout can be
  detected.  This is because all file paths will already be relative
  to the source root and so will not need to be converted.

Options:
  -h, --help            show this help message and exit
  -f RULES, --filter-rules=RULES
                        set a filter to control what categories of style errors to report.  Specify a
                        filter using a comma-delimited list of boolean filter rules, for example "--
                        filter -whitespace,+whitespace/braces".  To display all categories and which
                        are enabled by default, pass no value (e.g. '-f ""' or '--filter=').
  -g COMMIT, --git-diff=COMMIT, --git-commit=COMMIT
                        check all changes in the given git commit. Use 'commit_id..' to check all
                        changes after commmit_id
  -m INT, --min-confidence=INT
                        set the minimum confidence of style errors to report.  Can be an integer 1-5,
                        with 1 displaying all errors.  Defaults to 1.
  -o FORMAT, --output-format=FORMAT
                        set the output format, which can be "emacs" or "vs7" (for Visual Studio).
                        Defaults to "emacs".
  -s, --squash          All diffs from the remote branch are checked.If excluded, prompts whether to
                        squash when there are multiple commits.
  --no-squash           Only working copy diffs are checked.If excluded, prompts whether to squash
                        when there are multiple commits.
  -v, --verbose         enable verbose logging.

This script can miss errors and does not substitute for code review.

ERROR: no such option: --use-patch-as-diff


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hajime Morrita]

It looks EWS went something wrong at this time.
check-webkit-style passes the patch locally.

[Hajime Morrita]

Comment on attachment 56855 [details]
attempt to fix style violation

Clearing flags on attachment: 56855

Committed r60068: <http://trac.webkit.org/changeset/60068>

[Hajime Morrita]

All reviewed patches have been landed.  Closing bug.

[Julie Parent]

Reopening.  Rolled out in https://bugs.webkit.org/show_bug.cgi?id=39600 due to test failures.

[Ojan Vafai]

(In reply to comment #47)
> Reopening.  Rolled out in https://bugs.webkit.org/show_bug.cgi?id=39600 due to test failures.

To be clear, the failing test was fast/repaint/search-field-cancel on the Win and Linux Chromium bots. It looked like a real regression.

[Hajime Morrita]

(In reply to comment #48)
> To be clear, the failing test was fast/repaint/search-field-cancel on the Win and Linux Chromium bots. It looked like a real regression.
Thank you for clarify.
Because I couldn't figure out workaround yet, we need to keep this open.
I'm sorry for my lack of followup.

[Ahmad Saleem]

I took test case from the attached patch and changed it to this JSFiddle:

Link - https://jsfiddle.net/etqr57k1/show

All browser (Safari 16.3, Chrome Canary 111 and Firefox Nightly 111) passes all test. Do we need anything else?. Thanks!

[Brent Fulgham]

Closing based on Ahmad's testing.