Bug 9507 – Empty style spans created in applyInlineStyle

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Graham Dennis]

applyInlineStyle moves the start position of the selection forward if the offset in the start node is greater than the caretMaxOffset() for that node. This can cause the start position to be beyond the end position for an empty element. And in this case, because applyInlineStyle doesn't modify the start position in this case, the inline style is applied to the element following the empty element in addInlineStyleIfNeeded(style, node, node);

Steps to reproduce, examine the innerHTML of an editable div after pasting the entire contents of the webpage http://nowataballpark.org/contact.html into the div. In ToT, the first <td> element has the following code (after applyInlineStyle):
<td width="88" height="12"><font class="Apple-style-span" color="#FF0000"></font><p align="CENTER"><font class="Apple-style-span" color="#FF0000"><b>Name</b></font></p></td> 
With this patch, the empty <font></font> tag is removed.

This patch modifies the layout of two LayoutTests. The modification is caused by removing unnecessary tags in the HTML. The two layout tests with problems are editing/pasteboard/paste-4039777-fix and editing/unsupported-content/table-delete-001.
WebKit r14810 gives the following innerHTML for the root editable element in paste-4039777:
<ul style="text-align: right;"><font class="Apple-style-span" size="6"></font><span class="Apple-style-span" style="font-size: 24px;"></span><li style=""><font class="Apple-style-span" size="6"><span class="Apple-style-span" style="font-size: 24px;">A</span></font></li></ul><div style=""><font class="Apple-style-span" size="6"></font><span class="Apple-style-span" style="font-size: 24px;"></span><ul style=""><font class="Apple-style-span" size="6"></font><span class="Apple-style-span" style="font-size: 24px;"></span><li style=""><a href=""><font class="Apple-style-span" size="6"><span class="Apple-style-span" style="font-size: 24px;">B</span></font></a><font class="Apple-style-span" size="6"><span class="Apple-style-span" style="font-size: 24px;"> </span></font><font class="Apple-style-span" size="6"><span class="Apple-style-span" style="font-size: 24px;"><br style=""></span></font><font class="Apple-style-span" size="6"><span class="Apple-style-span" style="font-size: 24px;">C</span></font></li></ul></div><div id="test" class="editing" style="text-align: right;"><br><div> </div> </div>
With this patch, this becomes:
<div id="test" class="editing"><ul style="text-align: right;"><li style="">A</li></ul><div style=""><ul style=""><li style=""><a href="">B</a> <br style="">C</li></ul></div><br><div> </div> </div>

WebKit r14810 gives the following innerHTML for the root editable element in table-delete-001:
<div id="test" class="editing"> before<font class="Apple-style-span" size="4"><span class="Apple-style-span" style="border-spacing: 2px 2px; font-size: 16px;"><br></span></font>after </div>
With this patch, this becomes:
<div id="test" class="editing"> before<br>after </div>

[Graham Dennis]

Created attachment 8921 [details]
patch

[Maciej Stachowiak]

Comment on attachment 8921 [details]
patch

Kiling the empty style spans is great! Not sure of the reasoning for all the changes. Does this pass all the layout tests besides the two mentioned? Is it worth it having a test case jst checking for empty style spans like i the original reported case?

[Graham Dennis]

Created attachment 8932 [details]
testcase

Testcase.
ToT generates an empty <font> tag between 'foo' and 'bar'. This patch removes the reason that tag was created.

This patch passes all testcases except the two mentioned in my previous message.
One thing I didn't explain about the patch is the lines:
if (node->isAncestor(endNode)
    endNode = endNode->traverseNextSibling();
The reason for this is that if you have HTML like
<block1>
<block2></block2></block1>
the 'start' position can be (block1, 0) after removeInlineStyle() (just before this patch), and the end position can be (block1, 1), now in this case the if (start.offset() >= start.node()->caretMaxOffset()) line can move the 'start' node forward to block2, and so when a traversal is done (via traverseNextNode), the current node will never be reach the end node, and so the exit condition will never be reached.

[Maciej Stachowiak]

Comment on attachment 8921 [details]
patch

Looks ok to me. This should get a ChangeLog entry and should have the test case included in the patch, with expected results. Include that and I'll r+ it.

[Graham Dennis]

Created attachment 9582 [details]
patch 2

Updated patch. ChangeLog entries, new testcase and modified testcase results included.

[Justin Garcia]

Comment on attachment 9582 [details]
patch 2

there'll be a problem applying this change after adeles recent change.  graham: please post an updated patch.

[Graham Dennis]

Created attachment 9744 [details]
patch 3

Patch 3 updated to work with current ToT

[Justin Garcia]

r15714