[webkit-reviews] review denied: [Bug 118185] Gmail reply email - Bold and Italic style get stuck : [Attachment 207430] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jul 24 22:52:04 PDT 2013


Ryosuke Niwa <rniwa at webkit.org> has denied vanivhegde at gmail.com's request for
review:
Bug 118185: Gmail reply email - Bold and Italic style  get stuck
https://bugs.webkit.org/show_bug.cgi?id=118185

Attachment 207430: Patch
https://bugs.webkit.org/attachment.cgi?id=207430&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=207430&action=review


The test looks much better but we can do better.

> LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:10
> +<style>
> +    .editing {
> +    border: 2px solid red;
> +    padding: 12px;
> +    font-size: 24px;
> +    }
> +</style>

We don't need this style. It's for legacy pixel-based editing tests.

> LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:15
> +    var testDiv = document.getElementById("root");
> +    testDiv.focus();

This is done by editing.js. We're missing some other id otherwise.

> LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:26
> +<title>Test Bold and Italic for content with mixed editability</title>

We don't need title. We should put all script elements in body instead and get
rid of head element entirely.

> LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:33
> +<p>
> +    This is the test case for defect <a
href="https://bugs.webkit.org/show_bug.cgi?id=118185">118185</a> :
> +    <b> Gmail reply email - Bold and Italic style get stuck</b><br>This
tests bold/italic style toggling for a content with mixed editability
> +</p>

It's more helpful to state exactly what this test is trying to test.

> LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:38
> +    To test manually, select content in the above div and apply bold/italic
repeatedly. The behavior should be

I don't think we need this instruction since selecting, bolding, & italicizing
all works in the browser.

> LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:39
> +    <ol>

Also, p can't contain ol.

> LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:46
> +    runDumpAsTextEditingTest(true);

This test already passes on Mac so you need to force win or unix editing
behavior.
Also, there is no need to indent the script.

> LayoutTests/editing/style/toggle-style-bold-italic.html:10
> +<style>
> +    .editing {
> +    border: 2px solid red;
> +    padding: 12px;
> +    font-size: 24px;
> +    }
> +</style>

Ditto.

> LayoutTests/editing/style/toggle-style-bold-italic.html:15
> +    var testDiv = document.getElementById("root");
> +    testDiv.focus();

Ditto.

> LayoutTests/editing/style/toggle-style-bold-italic.html:27
> +<title>Test Bold and Italic on a content that has text node without
renderer</title>
> +</head>

Ditto.

> LayoutTests/editing/style/toggle-style-bold-italic.html:30
> +<p>

I would have added id="description" like other editing tests.

> LayoutTests/editing/style/toggle-style-bold-italic.html:32
> +    This is the test case for defect <a
href="https://bugs.webkit.org/show_bug.cgi?id=118185">118185</a> :
> +    <b> Gmail reply email - Bold and Italic style get stuck</b><br>

Again, this description isn't helpful.	Adding non-essential text like this
clutters the test and makes it harder to understand the test code.

> LayoutTests/editing/style/toggle-style-bold-italic.html:39
> +<p>To test manually, select content in the above div and apply bold/italic
repeatedly. Toggling between styles should be successfull.</p>

I don't think we need this instruction since the test runs in the browser.

> LayoutTests/editing/style/toggle-style-bold-italic.html:41
> +    runDumpAsTextEditingTest(true);

Ditto about the indentation and forcing win or unix editing behavior.


More information about the webkit-reviews mailing list