WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
118185
Gmail reply email - Bold and Italic style get stuck
https://bugs.webkit.org/show_bug.cgi?id=118185
Summary
Gmail reply email - Bold and Italic style get stuck
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.
Top of Page
Format For Printing
XML
Clone This Bug