[Webkit-unassigned] [Bug 55349] WebKit does not merge text decorations in the typing style and the selected element properly

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=55349





--- Comment #3 from Ryosuke Niwa <rniwa at webkit.org>  2011-03-01 15:54:33 PST ---
(In reply to comment #2)
> (From update of attachment 84201 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84201&action=review
> 
> > Source/WebCore/editing/ApplyStyleCommand.cpp:1284
> > +    if (node->isHTMLElement() && static_cast<HTMLElement*>(node)->inlineStyleDecl()) {
> > +        newInlineStyle = style->copy();
> > +        newInlineStyle->mergeInlineStyleOfElement(static_cast<HTMLElement*>(node));
> >      }
> 
> Why is this code specific to HTMLElement? Can’t this be done on a custom element that is technically not an HTML element? This code would treat <subsection>, for example, differently from <section>. I am not sure I understand why we have so much code specific to HTMLElement.

It's only for historical reasons.  We can fix in a separate patch.  Is there a particular test case you're thinking of?  It'll be good to have a regression test.

> > Source/WebCore/editing/EditingStyle.cpp:714
> > +        bool shouldAddTextDecorations = false;
> > +
> > +        RefPtr<CSSValue> value;
> 
> I think it would be clearer to combine these two local variables. The fact that “value” is specifically a text decorations value is unclear given the current name, and I think that the value being a null pointer would be just as good an indicator as the boolean if it had a good name.

That's an excellent point.  I'd just set value = 0 if it was value && !value->isValueList().

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