[Webkit-unassigned] [Bug 118185] Gmail reply email - Bold and Italic style get stuck

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=118185


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #207430|review?                     |review-
               Flag|                            |




--- Comment #14 from Ryosuke Niwa <rniwa at webkit.org>  2013-07-24 22:51:55 PST ---
(From update of attachment 207430)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.