WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED CONFIGURATION CHANGED
32077
textarea grows when you type
https://bugs.webkit.org/show_bug.cgi?id=32077
Summary
textarea grows when you type
Ojan Vafai
Reported
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.
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
Show Obsolete
(12)
View All
Add attachment
proposed patch, testcase, etc.
Mark Rowe (bdash)
Comment 1
2009-12-02 12:11:52 PST
<
rdar://problem/7437752
>
Hajime Morrita
Comment 2
2010-03-07 22:51:55 PST
Created
attachment 50190
[details]
patch v0
mitz
Comment 3
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.
mitz
Comment 4
2010-03-08 00:05:03 PST
> I don’t see why the texture cannot be the layout root, as
s/texture/textarea/
Hajime Morrita
Comment 5
2010-03-08 04:10:20 PST
Created
attachment 50203
[details]
patch v1; preserve original height when the renderer is layout-root
Hajime Morrita
Comment 6
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().
mitz
Comment 7
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().
Hajime Morrita
Comment 8
2010-03-08 22:25:27 PST
Created
attachment 50277
[details]
v2; follow the feedback.
Hajime Morrita
Comment 9
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?
mitz
Comment 10
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.
Hajime Morrita
Comment 11
2010-03-15 01:12:13 PDT
Created
attachment 50690
[details]
take the feedback
Hajime Morrita
Comment 12
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.
mitz
Comment 13
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?
Hajime Morrita
Comment 14
2010-03-15 23:00:07 PDT
Created
attachment 50763
[details]
switch to change objectIsRelayoutBoundary()
Hajime Morrita
Comment 15
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.
WebKit Review Bot
Comment 16
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.
Hajime Morrita
Comment 17
2010-03-15 23:16:48 PDT
Created
attachment 50764
[details]
fix style violation
Shinichiro Hamaji
Comment 18
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?
Shinichiro Hamaji
Comment 19
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.
Hajime Morrita
Comment 20
2010-03-17 03:39:18 PDT
Created
attachment 50892
[details]
replaced tests with script-tests
Shinichiro Hamaji
Comment 21
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.
Hajime Morrita
Comment 22
2010-03-22 22:46:10 PDT
Created
attachment 51394
[details]
revert last patch, with small fix on text in tests
Hajime Morrita
Comment 23
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.
Kent Tamura
Comment 24
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 ....'
Hajime Morrita
Comment 25
2010-05-20 21:31:12 PDT
Created
attachment 56667
[details]
gathered tests into one script-test
Hajime Morrita
Comment 26
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 .
Kent Tamura
Comment 27
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.
Kent Tamura
Comment 28
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
Shinichiro Hamaji
Comment 29
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"?
Hajime Morrita
Comment 30
2010-05-20 22:37:18 PDT
Created
attachment 56671
[details]
took feedbacks
Hajime Morrita
Comment 31
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.
Kent Tamura
Comment 32
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?
Darin Adler
Comment 33
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");
Hajime Morrita
Comment 34
2010-05-23 18:43:22 PDT
Created
attachment 56841
[details]
tool feedbacks
Hajime Morrita
Comment 35
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.
Kent Tamura
Comment 36
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().
Hajime Morrita
Comment 37
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
>
Hajime Morrita
Comment 38
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
Hajime Morrita
Comment 39
2010-05-24 00:41:46 PDT
Created
attachment 56853
[details]
fix pixel test failure
WebKit Review Bot
Comment 40
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.
Hajime Morrita
Comment 41
2010-05-24 00:55:05 PDT
Created
attachment 56855
[details]
attempt to fix style violation
Hajime Morrita
Comment 42
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.
WebKit Review Bot
Comment 43
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.
Hajime Morrita
Comment 44
2010-05-24 01:10:02 PDT
It looks EWS went something wrong at this time. check-webkit-style passes the patch locally.
Hajime Morrita
Comment 45
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
>
Hajime Morrita
Comment 46
2010-05-24 03:25:59 PDT
All reviewed patches have been landed. Closing bug.
Julie Parent
Comment 47
2010-05-24 10:02:33 PDT
Reopening. Rolled out in
https://bugs.webkit.org/show_bug.cgi?id=39600
due to test failures.
Ojan Vafai
Comment 48
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.
Hajime Morrita
Comment 49
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.
Ahmad Saleem
Comment 50
2023-01-24 03:38:53 PST
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
Comment 51
2024-01-22 14:41:41 PST
Closing based on Ahmad's testing.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug