[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





--- Comment #31 from Ryosuke Niwa <rniwa at webkit.org>  2013-07-30 01:39:41 PST ---
(From update of attachment 207698)
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.

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