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.
<rdar://problem/7437752>
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/
Created attachment 50203 [details] patch v1; preserve original height when the renderer is layout-root
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 @@ > +2010-03-08 MORITA Hajime <morrita@google.com> > + > + 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 @@ > +2010-03-08 MORITA Hajime <morrita@google.com> > + > + 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().
Created attachment 50277 [details] v2; follow the feedback.
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.
Created attachment 50690 [details] take the feedback
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?
Created attachment 50763 [details] switch to change objectIsRelayoutBoundary()
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.
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.
Created attachment 50764 [details] fix style violation
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?
> 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.
Created attachment 50892 [details] replaced tests with script-tests
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.
Created attachment 51394 [details] revert last patch, with small fix on text in tests
> 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.
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 ....'
Created attachment 56667 [details] gathered tests into one script-test
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 .
Comment on attachment 56667 [details] gathered tests into one script-test The patch contains no test expectation files.
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
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"?
Created attachment 56671 [details] took feedbacks
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.
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?
(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");
Created attachment 56841 [details] tool feedbacks
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.
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().
Comment on attachment 56841 [details] tool feedbacks Clearing flags on attachment: 56841 Committed r60060: <http://trac.webkit.org/changeset/60060>
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
Created attachment 56853 [details] fix pixel test failure
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.
Created attachment 56855 [details] attempt to fix style violation
> > - 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.
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.
It looks EWS went something wrong at this time. check-webkit-style passes the patch locally.
Comment on attachment 56855 [details] attempt to fix style violation Clearing flags on attachment: 56855 Committed r60068: <http://trac.webkit.org/changeset/60068>
All reviewed patches have been landed. Closing bug.
Reopening. Rolled out in https://bugs.webkit.org/show_bug.cgi?id=39600 due to test failures.
(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.
(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.