Bug 118185

Summary:

Gmail reply email - Bold and Italic style get stuck

Product:

WebKit

Reporter:

Marta Pawlowska <m.pawlowska>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, ktf.kim, rniwa, vanivhegde

Priority:

Keywords:

GoogleBug

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

URL:

http://gmail.google.com

Bug Depends on:

Bug Blocks:

9638

Attachments:

Description	Flags
Editing - text get stacked with bold style	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2	none
Patch	none
Patch	none
Patch	none

Marta Pawlowska

Reported 2013-06-28 05:06:13 PDT

[Steps] 1. Goto gmail.google.com long in 2. Create simple emial - just text and send it to your accout email example: "asdasdasd adasda asd sdf sdf sdf sdf" 3. Find this email in inbox 4. Press reply, expand previous email (...). Do not add any text on this email (just this reply) 5. Select whole text (ctrl+a) 6. Select "B" (bold) from formatting menu 7. Select whole text (ctrl+a) 8. Select "B" (bold) from formatting menu [Expected result] Text changes bold/not bold. [Actual result] Text is bold all the time (after second pressing "B") [Research] Webkit.org: FAIL Firefox 21: OK IE 10: FAIL Chrome canary ver30.0.1550.0: FAIL

Attachments
Editing - text get stacked with bold style (920 bytes, text/html) 2013-07-01 05:39 PDT, Marta Pawlowska	no flags	Details
Patch (20.34 KB, patch) 2013-07-22 23:43 PDT, vanivhegde	no flags	Details Formatted Diff Diff
Patch (16.21 KB, patch) 2013-07-23 01:55 PDT, vanivhegde	no flags	Details Formatted Diff Diff
Patch (15.55 KB, patch) 2013-07-23 02:27 PDT, vanivhegde	no flags	Details Formatted Diff Diff
Patch (17.29 KB, patch) 2013-07-24 21:03 PDT, vanivhegde	no flags	Details Formatted Diff Diff
Patch (18.87 KB, patch) 2013-07-25 00:21 PDT, vanivhegde	no flags	Details Formatted Diff Diff
Patch (24.78 KB, patch) 2013-07-25 01:13 PDT, vanivhegde	no flags	Details Formatted Diff Diff
Patch (25.76 KB, patch) 2013-07-25 03:31 PDT, vanivhegde	no flags	Details Formatted Diff Diff
Patch (25.93 KB, patch) 2013-07-25 21:28 PDT, vanivhegde	no flags	Details Formatted Diff Diff
Patch (25.89 KB, patch) 2013-07-29 22:29 PDT, vanivhegde	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (489.32 KB, application/zip) 2013-07-30 00:18 PDT, Build Bot	no flags	Details
Patch (27.10 KB, patch) 2013-07-30 01:28 PDT, vanivhegde	no flags	Details Formatted Diff Diff
Patch (27.51 KB, patch) 2013-07-30 04:39 PDT, vanivhegde	no flags	Details Formatted Diff Diff
Patch (27.84 KB, patch) 2013-07-30 20:25 PDT, vanivhegde	no flags	Details Formatted Diff Diff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2013-06-28 09:58:17 PDT

Does this work correctly in Safari 6.0.5?

Marta Pawlowska

Comment 2 2013-07-01 04:45:36 PDT

Minimal test case: <html> <head> <script> function boldit() { var test = document.getElementById('zmiana'); document.getSelection().selectAllChildren(test); document.execCommand("Bold"); } </script> </head> <body contenteditable=true> <div style="border: solid 2px; width: 100px;"> Font style:<br /> <button onclick="boldit()">B</button> </div> <br /> <br /> <div contenteditable="true" id="zmiana"> <div> a </div> <img> b </div> </body> </html>

Marta Pawlowska

Comment 3 2013-07-01 05:39:40 PDT

Created attachment 205801 [details] Editing - text get stacked with bold style I've change id name to english and added pass conditions in test case.

vanivhegde

Comment 4 2013-07-22 05:24:25 PDT

Working on it. I will upload the patch soon.

vanivhegde

Comment 5 2013-07-22 23:43:54 PDT

Created attachment 207306 [details] Patch

Ryosuke Niwa

Comment 6 2013-07-22 23:51:56 PDT

Comment on attachment 207306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207306&action=review > LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:44 > + runEditingTest(); We should be using runDumpAsTextEditingTest or dump-as-markup.js instead.. > LayoutTests/editing/style/toggle-style-bold-italic.html:43 > + runEditingTest(); Ditto.

Ryosuke Niwa

Comment 7 2013-07-22 23:52:15 PDT

(In reply to comment #1) > Does this work correctly in Safari 6.0.5? I think this only reproduces on non-Mac platforms.

vanivhegde

Comment 8 2013-07-23 01:55:31 PDT

Created attachment 207311 [details] Patch

vanivhegde

Comment 9 2013-07-23 01:57:23 PDT

Thanks for the review. I have modified layout tests to use runDumpAsTextEditingTest. Please have a look at the new patch

Ryosuke Niwa

Comment 10 2013-07-23 01:59:32 PDT

Comment on attachment 207311 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207311&action=review > LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:1 > +<html> Missing DOCTYPE and we don't need to indent the test content like this. It's much less readable IMO. > LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:21 > + boldCommand(); > + italicCommand(); > + boldCommand(); > + italicCommand(); > + boldCommand(); > + italicCommand(); You should call execBoldCommand and execItalicCommand so that the result of each execCommand will be logged. > LayoutTests/editing/style/toggle-style-bold-italic.html:21 > + selectAllCommand(); > + boldCommand(); > + italicCommand(); > + boldCommand(); > + italicCommand(); > + boldCommand(); > + italicCommand(); Ditto.

vanivhegde

Comment 11 2013-07-23 02:06:26 PDT

Would you want me to knock off indentation completely?

vanivhegde

Comment 12 2013-07-23 02:27:22 PDT

Created attachment 207316 [details] Patch

vanivhegde

Comment 13 2013-07-24 21:03:10 PDT

Created attachment 207430 [details] Patch

Ryosuke Niwa

Comment 14 2013-07-24 22:51:55 PDT

Comment on attachment 207430 [details] Patch 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.

vanivhegde

Comment 15 2013-07-25 00:21:58 PDT

Created attachment 207435 [details] Patch

vanivhegde

Comment 16 2013-07-25 00:24:11 PDT

Submitted new patch with all review comments incorporated

Ryosuke Niwa

Comment 17 2013-07-25 00:28:01 PDT

Comment on attachment 207435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207435&action=review The tests read much better but there are still some issues. > LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:13 > +test('win'); > +test('unix'); Don't we need to reset the content before each test? Also, if we're testing both win & unix editing behaviors, we might as well as test mac editing behavior as well. > LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:17 > +if (window.internals) > +internals.settings.setEditingBehavior(platform); > +runDumpAsTextEditingTest(true); Let's indent the function definition & the statement inside if. > LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:24 > +execSelectAllCommand(); > +execBoldCommand(); > +execItalicCommand(); > +execBoldCommand(); > +execItalicCommand(); Indentation. > LayoutTests/editing/style/toggle-style-bold-italic.html:26 > +test('win'); > +test('unix'); > +function test(platform) { > +if (window.internals) > +internals.settings.setEditingBehavior(platform); > +runDumpAsTextEditingTest(true); > +} > +function editingTest() { > +execSelectAllCommand(); > +execBoldCommand(); > +execItalicCommand(); > +execBoldCommand(); > +execItalicCommand(); > +} Ditto.

vanivhegde

Comment 18 2013-07-25 01:13:36 PDT

Created attachment 207437 [details] Patch

vanivhegde

Comment 19 2013-07-25 01:17:22 PDT

Thanks a lot for the review > > LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:13 > > +test('win'); > > +test('unix'); > > Don't we need to reset the content before each test? I do not think we have to reset the test content, as I am executing each command(bold/italic) twice in each test. Hence by the end of one test the content would be reset automatically. I have addressed other review comments.

Ryosuke Niwa

Comment 20 2013-07-25 01:32:27 PDT

Comment on attachment 207437 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207437&action=review > LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:14 > +test('mac'); > +test('win'); > +test('unix'); Can we call debug() to output the editing behavior we're going to use? Take a look at other tests that call setEditingBehavior. Some them do this.

vanivhegde

Comment 21 2013-07-25 03:27:29 PDT

(In reply to comment #20) > (From update of attachment 207437 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207437&action=review > > > LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:14 > > +test('mac'); > > +test('win'); > > +test('unix'); > > Can we call debug() to output the editing behavior we're going to use? > Take a look at other tests that call setEditingBehavior. Some them do this. We can do that, but we will have to write a custom log function to add text to already existing <ol> that is used by editing.js to log data. I will upload a patch with this change, it won't look that bad though. It's just that we are adding an additional function in the test files. Hope that's okay!

vanivhegde

Comment 22 2013-07-25 03:31:17 PDT

Created attachment 207445 [details] Patch

Ryosuke Niwa

Comment 23 2013-07-25 17:13:37 PDT

Comment on attachment 207445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207445&action=review > LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:19 > + log("Above are the results with "+platform+" platform editing behavior"); It would have been better to print out the platform before the test starts. Adding a blank line between each platform will also help. > LayoutTests/editing/style/toggle-style-bold-italic-mixed-editability.html:35 > +function log(text) { > + var newItem = document.createElement('ul'); > + newItem.appendChild(document.createTextNode(text)); > + var markupResultList = document.getElementsByTagName("ol")[0]; > + markupResultList.appendChild(newItem); > +} Why don't we add this to editing.js and call it something like separateMarkupResultList()? It's odd to modify the variable editing.js defines like this. > LayoutTests/editing/style/toggle-style-bold-italic.html:36 > +function log(text) { > + var newItem = document.createElement('ul'); > + newItem.appendChild(document.createTextNode(text)); > + var markupResultList = document.getElementsByTagName("ol")[0]; > + markupResultList.appendChild(newItem); > +} Ditto.

vanivhegde

Comment 24 2013-07-25 21:28:29 PDT

Created attachment 207506 [details] Patch

Ryosuke Niwa

Comment 25 2013-07-29 20:18:56 PDT

Comment on attachment 207506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207506&action=review > LayoutTests/ChangeLog:12 > + (createNewLine): > + Added new utility functions to be able to log data and to be able to > + create line breaks between log statements Why are we line breaking after the colon? > LayoutTests/ChangeLog:19 > + Layout tests added to test bold/italic style toggling on a content > + with mixed editability You should probably mention this at the beginning of the entry after Reviewed by line. > LayoutTests/editing/editing.js:936 > + var newItem = document.createTextNode(text); > + markupResultList.appendChild(newItem); This isn't right. You need to close the existing list and create a new list. Inserting a dangling text node without li results in "invalid" markup. > LayoutTests/editing/editing.js:942 > +function createNewLine() { > + var newItem = document.createElement('br'); > + markupResultList.appendChild(newItem); > +} This function name seems too generic. It also has the same problem as separateMarkupResultList.

vanivhegde

Comment 26 2013-07-29 22:29:18 PDT

Created attachment 207688 [details] Patch

Ryosuke Niwa

Comment 27 2013-07-30 00:06:14 PDT

Comment on attachment 207688 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207688&action=review > LayoutTests/editing/editing.js:942 > + if (markupResultList.hasChildNodes()) { > + document.body.appendChild(markupResultList); > + var newLine = document.createElement('br'); > + document.body.appendChild(newLine); > + markupResultList = document.createElement('ol'); > + } > + var newItem = document.createTextNode(text); > + document.body.appendChild(newItem); We shouldn't modify the body until editingTest had finished running. This could interfere with testing. We should instead hold onto an array of elements to insert at the end.

Build Bot

Comment 28 2013-07-30 00:18:26 PDT

Comment on attachment 207688 [details] Patch Attachment 207688 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1298248 New failing tests: editing/selection/leak-document-with-selection-inside.html

Build Bot

Comment 29 2013-07-30 00:18:28 PDT

Created attachment 207695 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

vanivhegde

Comment 30 2013-07-30 01:28:09 PDT

Created attachment 207698 [details] Patch

Ryosuke Niwa

Comment 31 2013-07-30 01:39:41 PDT

Comment on attachment 207698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207698&action=review > Source/WebCore/editing/EditingStyle.cpp:691 > TriState nodeState = triStateOfStyle(&computedStyle, node->isTextNode() ? EditingStyle::DoNotIgnoreTextOnlyProperties : EditingStyle::IgnoreTextOnlyProperties); > if (node == selection.start().deprecatedNode()) > state = nodeState; > - else if (state != nodeState && node->isTextNode()) { > + else if (state != nodeState && node->isTextNode() && node->renderer() && node->rendererIsEditable()) { Come to think of it, this code change is probably incorrect. We shouldn't be updating state when node doesn't have a renderer or it's not editable although I can't think of a test case in which start().deprecatedNode() will be either invisible or not editable. We should nonetheless make this code more robust by skipping any non-editable node or any node without renderer. Note that we need to create a local boolean to replace "node == selection.start().deprecatedNode()" in that case since we need to skip until we see the first editable node with renderer. > LayoutTests/editing/editing.js:908 > +var markupResultList = []; > +var markupOrderedList = document.createElement('ol'); Instead of having two global variables like this, we can always append a new child to the last list element in markupResultList. Also, I think markupResultList is a descriptive variable name since it's a list of elements to be inserted at the end. Maybe elementsForDumpingMarkupList? > LayoutTests/editing/editing.js:927 > + document.body.appendChild(markupOrderedList); Once we've made the improved I suggested above, we wouldn't need the this line. > LayoutTests/editing/editing.js:934 > - markupResultList.appendChild(newItem); > + markupOrderedList.appendChild(newItem); To clarify, we should be doing elementsForDumpingMarkupList[elementsForDumpingMarkupList.length - 1].appendChild(newItem) here. > LayoutTests/editing/editing.js:938 > +function separateMarkupOrderedList(text) { I would have called this startNewMarkupGroup since the text is for the next group. Also, we should use a better variable name than "text". label maybe? > LayoutTests/editing/editing.js:943 > + var newLine = document.createElement('br'); > + markupResultList.push(newLine); We don't need this local variable.

vanivhegde

Comment 32 2013-07-30 02:05:36 PDT

(In reply to comment #31) > (From update of attachment 207698 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207698&action=review > > > Source/WebCore/editing/EditingStyle.cpp:691 > > TriState nodeState = triStateOfStyle(&computedStyle, node->isTextNode() ? EditingStyle::DoNotIgnoreTextOnlyProperties : EditingStyle::IgnoreTextOnlyProperties); > > if (node == selection.start().deprecatedNode()) > > state = nodeState; > > - else if (state != nodeState && node->isTextNode()) { > > + else if (state != nodeState && node->isTextNode() && node->renderer() && node->rendererIsEditable()) { > > Come to think of it, this code change is probably incorrect. We shouldn't be updating state when node doesn't have a renderer or it's not editable > although I can't think of a test case in which start().deprecatedNode() will be either invisible or not editable. > > We should nonetheless make this code more robust by skipping any non-editable node or any node without renderer. > Note that we need to create a local boolean to replace "node == selection.start().deprecatedNode()" in that case > since we need to skip until we see the first editable node with renderer. Thanks for the review. I am bit confused here. To make sure that we update the state only when start().deprecatedNode() has renderer and editable wouldn't the below check be sufficient if (node == selection.start().deprecatedNode() && node->renderer() && node->rendererIsEditable()) Why do we need the local boolean?

Ryosuke Niwa

Comment 33 2013-07-30 02:07:30 PDT

(In reply to comment #32) > (In reply to comment #31) > > (From update of attachment 207698 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=207698&action=review > > > > > Source/WebCore/editing/EditingStyle.cpp:691 > > > TriState nodeState = triStateOfStyle(&computedStyle, node->isTextNode() ? EditingStyle::DoNotIgnoreTextOnlyProperties : EditingStyle::IgnoreTextOnlyProperties); > > > if (node == selection.start().deprecatedNode()) > > > state = nodeState; > > > - else if (state != nodeState && node->isTextNode()) { > > > + else if (state != nodeState && node->isTextNode() && node->renderer() && node->rendererIsEditable()) { > > > > Come to think of it, this code change is probably incorrect. We shouldn't be updating state when node doesn't have a renderer or it's not editable > > although I can't think of a test case in which start().deprecatedNode() will be either invisible or not editable. > > > > We should nonetheless make this code more robust by skipping any non-editable node or any node without renderer. > > Note that we need to create a local boolean to replace "node == selection.start().deprecatedNode()" in that case > > since we need to skip until we see the first editable node with renderer. > > Thanks for the review. I am bit confused here. To make sure that we update the state only when start().deprecatedNode() has renderer and editable wouldn't the below check be sufficient > > if (node == selection.start().deprecatedNode() && node->renderer() && node->rendererIsEditable()) > > Why do we need the local boolean? No, because state must be updated when we computed the style for the first editable node with renderer. If start().deprecatedNode() didn't have renderer, then "node == selection.start().deprecatedNode()" will never be true for subsequent nodes and never update state to true state.

vanivhegde

Comment 34 2013-07-30 04:39:20 PDT

Created attachment 207721 [details] Patch

vanivhegde

Comment 35 2013-07-30 04:52:21 PDT

Hi rniwa, I have made changes as per your review comments. Please have a look. Few explanations: 1) var elementsForDumpingMarkupList = [document.createElement('ol')]; Have initialized the list with <ol> to avoid the failure of existing test cases 2) function startNewMarkupGroup(label) { if (!elementsForDumpingMarkupList[elementsForDumpingMarkupList.length - 1].hasChildNodes()) elementsForDumpingMarkupList.pop(); This is to avoid logging empty <ol>s. This happens if we invoke startNewMarkupGroup(if we want to log some data before actual test results) before invoking execCommand* functions.

Ryosuke Niwa

Comment 36 2013-07-30 12:07:45 PDT

Comment on attachment 207721 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207721&action=review > Source/WebCore/editing/EditingStyle.cpp:692 > + if (node->renderer() && node->rendererIsEditable()) { Instead of checking renderer here, why don't we wrap this whole loop with node->renderer() && node->rendererIsEditable() except the break? That way, we avoid calling computedStyle on non-editable nodes and nodes without renderer altogether.

vanivhegde

Comment 37 2013-07-30 20:25:52 PDT

Created attachment 207800 [details] Patch

WebKit Commit Bot

Comment 38 2013-07-30 21:53:47 PDT

Comment on attachment 207800 [details] Patch Clearing flags on attachment: 207800 Committed r153509: <http://trac.webkit.org/changeset/153509>

WebKit Commit Bot

Comment 39 2013-07-30 21:53:51 PDT

All reviewed patches have been landed. Closing bug.

Ryosuke Niwa

Comment 40 2013-08-01 20:02:47 PDT

*** Bug 118285 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.