| 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: | P2 | Keywords: | GoogleBug | ||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||
| URL: | http://gmail.google.com | ||||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||||
| Bug Blocks: | 9638 | ||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||
|
Description
Marta Pawlowska
2013-06-28 05:06:13 PDT
Does this work correctly in Safari 6.0.5? 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>
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.
Working on it. I will upload the patch soon. Created attachment 207306 [details]
Patch
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. (In reply to comment #1) > Does this work correctly in Safari 6.0.5? I think this only reproduces on non-Mac platforms. Created attachment 207311 [details]
Patch
Thanks for the review. I have modified layout tests to use runDumpAsTextEditingTest. Please have a look at the new patch 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. Would you want me to knock off indentation completely? Created attachment 207316 [details]
Patch
Created attachment 207430 [details]
Patch
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. Created attachment 207435 [details]
Patch
Submitted new patch with all review comments incorporated 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. Created attachment 207437 [details]
Patch
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.
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. (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! Created attachment 207445 [details]
Patch
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. Created attachment 207506 [details]
Patch
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. Created attachment 207688 [details]
Patch
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. 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 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
Created attachment 207698 [details]
Patch
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. (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? (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. Created attachment 207721 [details]
Patch
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.
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. Created attachment 207800 [details]
Patch
Comment on attachment 207800 [details] Patch Clearing flags on attachment: 207800 Committed r153509: <http://trac.webkit.org/changeset/153509> All reviewed patches have been landed. Closing bug. *** Bug 118285 has been marked as a duplicate of this bug. *** |