Bug 45568 – WebKit nests font element when applying different font styles

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Reproduction steps:
1. Go to http://www.mozilla.org/editor/midasdemo/
2. Type "hello"
3. Change the font size to 4
4. Change the font family to Courier

Expected result:
<font size="4" face="Courier">hello</font>

Actual result:
<font class="Apple-style-span" size="4"><font class="Apple-style-span" face="Courier">hello</font></font>

[Ryosuke Niwa]

Created attachment 67265 [details]
work in progress

I need to rebaseline editing/inserting/insert-3659587-fix.html and editing/style/style-3690704-fix.html as well but I'll convert them to use runDumpAsTextEditingTest() first.

[Ryosuke Niwa]

Created attachment 67331 [details]
fixes the bug

[Ryosuke Niwa]

Comment on attachment 67331 [details]
fixes the bug

I found a way to break this patch up.  Will file a blocker bug with a patch tomorrow morning.

[Ryosuke Niwa]

Created attachment 67706 [details]
new patch

[Tony Chang]

Comment on attachment 67706 [details]
new patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67706&action=review

In general, looks fine, just need some clarification.

> WebCore/editing/ApplyStyleCommand.cpp:1162
> +    if (!needToApplyStyle)
> +        return false;

What happens if we don't return early?  Do we end up with a redundant tag?

> WebCore/editing/ApplyStyleCommand.cpp:1192
> +    if (!element->parentNode() || !element->parentNode()->isContentEditable())
> +        mode = RemoveNone;

This seems strange to me.  Why would we change the passed in mode?  Should the method just return false instead or should this check be done by the caller?

> WebCore/editing/ApplyStyleCommand.cpp:1879
> +        Node* container = fontContainer ? fontContainer : startNode;

Isn't fontContainer always null here?

[Ryosuke Niwa]

Comment on attachment 67706 [details]
new patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67706&action=review

>> WebCore/editing/ApplyStyleCommand.cpp:1162
>> +        return false;
> 
> What happens if we don't return early?  Do we end up with a redundant tag?

No. It's quite opposite.  When needToApplyStyle is false, the current run already has the desired style, and there's nothing to do.  So we skip the run in applyInlineStyleToNodeRange (see http://trac.webkit.org/browser/trunk/WebCore/editing/ApplyStyleCommand.cpp#L1158).  This is to avoid removing and adding the exact same markup.  Of course, it implies that we leave the existing redundant styles in certain cases but I don't think that's a problem.  When needToApplyStyle is true on the other hand, we go ahead and remove all styles that are either in conflict or same as the desired style from the current run so that there is no redundant markups present when we apply new style in applyInlineStyleToNodeRange.

>> WebCore/editing/ApplyStyleCommand.cpp:1192
>> +        mode = RemoveNone;
> 
> This seems strange to me.  Why would we change the passed in mode?  Should the method just return false instead or should this check be done by the caller?

We could do that.  I need to look into the reason I added that check since I don't remember it anymore.

>> WebCore/editing/ApplyStyleCommand.cpp:1879
>> +        Node* container = fontContainer ? fontContainer : startNode;
> 
> Isn't fontContainer always null here?

Right.  Will fix.

[Ryosuke Niwa]

Created attachment 69286 [details]
fixed per Tony's comment

[Tony Chang]

Comment on attachment 69286 [details]
fixed per Tony's comment

View in context: https://bugs.webkit.org/attachment.cgi?id=69286&action=review

r=me without the project.pbxproj change

> WebCore/WebCore.xcodeproj/project.pbxproj:-20860
> -			developmentRegion = English;

I don't think you meant to include this.  It caused the patch to now apply on the ews bots.

[Ryosuke Niwa]

(In reply to comment #8)
> (From update of attachment 69286 [details])
> > WebCore/WebCore.xcodeproj/project.pbxproj:-20860
> > -			developmentRegion = English;
> 
> I don't think you meant to include this.  It caused the patch to now apply on the ews bots.

Yeah, it was too late when I realized it.  Will fix.  Thanks for the timely review!

[Ryosuke Niwa]

Committed r68830: <http://trac.webkit.org/changeset/68830>