[Webkit-unassigned] [Bug 122097] ASSERTION FAILED: !style->propertyIsImportant(propertyID) in WebCore::setTextDecorationProperty

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Santosh Mahto <santosh.ma at samsung.com>  2013-10-16 11:23:59 PST ---

Editing bugs are so tricky!!
(In reply to comment #4)
> (From update of attachment 214148 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214148&action=review
> 
> > Source/WebCore/ChangeLog:25
> > +        (WebCore::StyleChange::extractTextStyles): Remove property even if 
> > +        it is declared !important.
> 
> Why?

Yes becasue we do it for other styleproperty in same function. In code ApplyStyleCommand::addInlineStyleIfNeeded :  we try to find StyleChange that we need to apply(pushing down style in conflicting style node childs).
 StyleChange class constructor doesnot consider !important in its calculation for any other property.it just try to find 6 text changes to applied.
    : m_applyBold(false)
    , m_applyItalic(false)
    , m_applyUnderline(false)
    , m_applyLineThrough(false)
    , m_applySubscript(false)
    , m_applySuperscript(false)

What I mean is these text properties are applied in different way (by surrounding with node <b>, <i> etc) not by CSS rules to child node. So we need to clear out css style (style->removeProperty()) whether it is declared !important or not.

!important property is used in resolving the style among multiple CSS contenders. since for textdecoration:underline !important  we will enforce style by adding node(utag) so !important consideration is useless.

> > Source/WebCore/editing/EditingStyle.cpp:1446
> > -        setTextDecorationProperty(style, newTextDecoration.get(), CSSPropertyTextDecoration);
> > +        if (newTextDecoration->length())
> > +            setTextDecorationProperty(style, newTextDecoration.get(), CSSPropertyTextDecoration);
> > +        else
> > +            style->removeProperty(CSSPropertyTextDecoration);
> > +
> 
> This doesn't make any sense. The whole point of setTextDecorationProperty is to take care of that.
> We should figure out why setTextDecorationProperty has the assertion instead of working around it here.

As above explanation: since textdecoration:underline style will be applied by surrounding node and so !important is not relevant. So We should remove property[ style->removeProperty(propertyID);](if empty) even if it is declared as important in code.

So in this path of flow ASSERT is wrong.
I think better way is to remove ASSERT from setTextDecorationProperty.


> > LayoutTests/editing/execCommand/remove-format-textdecoration-in-iframe.html:6
> > +    <iframe onload=" {     document.designMode=&apos;on&apos;;
> > +                   document.execCommand(&apos;selectall&apos;);
> > +                   document.execCommand(&apos;RemoveFormat&apos;);      } ">
> 
> Please fix the indentation.
> Also, we should use dump-as-text or dump-as-markup.
> There is no reason for this test to be a render tree dump.
> r- because of this.
I will update updated testcase in next patch, thanks

There is more to explain: 
Actually crash is Quite general(see next test case).

code modification in iframe:
As you can see in patch I added an iframe check. The reason for this was that
since we apply style on inline block by surrounding with node(utag, <b>), To do this we disconnect iframe node and again reattach as child of styling node.But whenever iframe node is added on tree, subframe loading is triggered and again onload event handler will be called (1st test case) and again everything will repeat and finally stackoverflow.

But I think node->renderer()->isReplaced() will be a better check there.

please comment if i confused you anywhere...

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