[Webkit-unassigned] [Bug 118185] Gmail reply email - Bold and Italic style get stuck
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jul 30 01:39:55 PDT 2013
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.
More information about the webkit-unassigned
mailing list