[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 #32 from vanivhegde at gmail.com  2013-07-30 02:05:36 PST ---
(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?

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