Bug 32077 - textarea grows when you type
Summary: textarea grows when you type
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar
Depends on: 39600
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-02 11:11 PST by Ojan Vafai
Modified: 2011-09-02 13:02 PDT (History)
9 users (show)

See Also:


Attachments
test case (85 bytes, text/html)
2009-12-02 11:11 PST, Ojan Vafai
no flags Details
patch v0 (15.02 KB, patch)
2010-03-07 22:51 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v1; preserve original height when the renderer is layout-root (11.85 KB, patch)
2010-03-08 04:10 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
v2; follow the feedback. (14.68 KB, patch)
2010-03-08 22:25 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
take the feedback (14.90 KB, patch)
2010-03-15 01:12 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
switch to change objectIsRelayoutBoundary() (12.29 KB, patch)
2010-03-15 23:00 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
fix style violation (12.28 KB, patch)
2010-03-15 23:16 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
replaced tests with script-tests (16.31 KB, patch)
2010-03-17 03:39 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
revert last patch, with small fix on text in tests (12.40 KB, patch)
2010-03-22 22:46 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
gathered tests into one script-test (5.50 KB, patch)
2010-05-20 21:31 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
took feedbacks (6.35 KB, patch)
2010-05-20 22:37 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
tool feedbacks (6.50 KB, patch)
2010-05-23 18:43 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
fix pixel test failure (6.47 KB, patch)
2010-05-24 00:41 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
attempt to fix style violation (6.48 KB, patch)
2010-05-24 00:55 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2009-12-02 11:11:12 PST
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.
Comment 1 Mark Rowe (bdash) 2009-12-02 12:11:52 PST
<rdar://problem/7437752>
Comment 2 Hajime Morrita 2010-03-07 22:51:55 PST
Created attachment 50190 [details]
patch v0
Comment 3 mitz 2010-03-08 00:04:15 PST
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.
Comment 4 mitz 2010-03-08 00:05:03 PST
> I don’t see why the texture cannot be the layout root, as
s/texture/textarea/
Comment 5 Hajime Morrita 2010-03-08 04:10:20 PST
Created attachment 50203 [details]
patch v1; preserve original height when the renderer is layout-root
Comment 6 Hajime Morrita 2010-03-08 04:16:26 PST
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 7 mitz 2010-03-08 10:54:14 PST
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().
Comment 8 Hajime Morrita 2010-03-08 22:25:27 PST
Created attachment 50277 [details]
v2; follow the feedback.
Comment 9 Hajime Morrita 2010-03-08 22:33:15 PST
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 10 mitz 2010-03-13 10:53:26 PST
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.
Comment 11 Hajime Morrita 2010-03-15 01:12:13 PDT
Created attachment 50690 [details]
take the feedback
Comment 12 Hajime Morrita 2010-03-15 01:19:59 PDT
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.
Comment 13 mitz 2010-03-15 08:10:26 PDT
Perhaps a simpler fix would be to add the percent height check to the text control branch of the condition in isRelayoutBoubdary() then?
Comment 14 Hajime Morrita 2010-03-15 23:00:07 PDT
Created attachment 50763 [details]
switch to change objectIsRelayoutBoundary()
Comment 15 Hajime Morrita 2010-03-15 23:07:05 PDT
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.
Comment 16 WebKit Review Bot 2010-03-15 23:08:13 PDT
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.
Comment 17 Hajime Morrita 2010-03-15 23:16:48 PDT
Created attachment 50764 [details]
fix style violation
Comment 18 Shinichiro Hamaji 2010-03-17 01:46:31 PDT
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?
Comment 19 Shinichiro Hamaji 2010-03-17 02:36:45 PDT
> 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.
Comment 20 Hajime Morrita 2010-03-17 03:39:18 PDT
Created attachment 50892 [details]
replaced tests with script-tests
Comment 21 Shinichiro Hamaji 2010-03-18 11:34:36 PDT
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.
Comment 22 Hajime Morrita 2010-03-22 22:46:10 PDT
Created attachment 51394 [details]
revert last patch, with small fix on text in tests
Comment 23 Hajime Morrita 2010-03-22 22:48:24 PDT
> 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 24 Kent Tamura 2010-05-20 02:54:41 PDT
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 ....'
Comment 25 Hajime Morrita 2010-05-20 21:31:12 PDT
Created attachment 56667 [details]
gathered tests into one script-test
Comment 26 Hajime Morrita 2010-05-20 21:34:12 PDT
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 27 Kent Tamura 2010-05-20 21:39:04 PDT
Comment on attachment 56667 [details]
gathered tests into one script-test

The patch contains no test expectation files.
Comment 28 Kent Tamura 2010-05-20 21:39:31 PDT
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 29 Shinichiro Hamaji 2010-05-20 21:48:53 PDT
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"?
Comment 30 Hajime Morrita 2010-05-20 22:37:18 PDT
Created attachment 56671 [details]
took feedbacks
Comment 31 Hajime Morrita 2010-05-20 22:40:04 PDT
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 32 Kent Tamura 2010-05-20 22:46:41 PDT
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?
Comment 33 Darin Adler 2010-05-21 11:22:30 PDT
(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");
Comment 34 Hajime Morrita 2010-05-23 18:43:22 PDT
Created attachment 56841 [details]
tool feedbacks
Comment 35 Hajime Morrita 2010-05-23 18:45:47 PDT
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 36 Kent Tamura 2010-05-23 18:50:23 PDT
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 37 Hajime Morrita 2010-05-23 21:39:37 PDT
Comment on attachment 56841 [details]
tool feedbacks

Clearing flags on attachment: 56841

Committed r60060: <http://trac.webkit.org/changeset/60060>
Comment 38 Hajime Morrita 2010-05-23 22:26:00 PDT
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
Comment 39 Hajime Morrita 2010-05-24 00:41:46 PDT
Created attachment 56853 [details]
fix pixel test failure
Comment 40 WebKit Review Bot 2010-05-24 00:43:50 PDT
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.
Comment 41 Hajime Morrita 2010-05-24 00:55:05 PDT
Created attachment 56855 [details]
attempt to fix style violation
Comment 42 Hajime Morrita 2010-05-24 00:56:18 PDT
> 
>  - 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.
Comment 43 WebKit Review Bot 2010-05-24 00:57:33 PDT
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.
Comment 44 Hajime Morrita 2010-05-24 01:10:02 PDT
It looks EWS went something wrong at this time.
check-webkit-style passes the patch locally.
Comment 45 Hajime Morrita 2010-05-24 03:25:46 PDT
Comment on attachment 56855 [details]
attempt to fix style violation

Clearing flags on attachment: 56855

Committed r60068: <http://trac.webkit.org/changeset/60068>
Comment 46 Hajime Morrita 2010-05-24 03:25:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 47 Julie Parent 2010-05-24 10:02:33 PDT
Reopening.  Rolled out in https://bugs.webkit.org/show_bug.cgi?id=39600 due to test failures.
Comment 48 Ojan Vafai 2010-05-27 15:30:12 PDT
(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.
Comment 49 Hajime Morrita 2010-05-27 21:20:07 PDT
(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.