Bug 28091 – ApplyStyleCommand removes presentational tags even when not necessary

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

[Ryosuke Niwa]

implicitlyStyledElementShouldBeRemovedWhenApplyingStyle returns true regardless of whether the existing tag is useful or not.  This causes tags to disappear and causes DOM tree to change unexpectedly.

Bolding "hello<b id="test">world</b>"  results in <b>hello</b><span id="test"><b>world</b></span>
and bolding "hello<b><i>world</i></b>" results in "<b>hello</b><i><b>world</b></i>" 
(notice the ordering of b and i changed.)

[Ryosuke Niwa]

Created attachment 34355 [details]
fixes the bug

[Adam Barth]

This patch appears reasonable on its face but has been sitting in the review queue for a long time.  Who should review this code (clearly not me)?

[Ryosuke Niwa]

(In reply to comment #2)
> This patch appears reasonable on its face but has been sitting in the review
> queue for a long time.  Who should review this code (clearly not me)?

This patch would conflict.  We need to submit new patch for this.

[Ryosuke Niwa]

Created attachment 38971 [details]
new patch

[Ryosuke Niwa]

Unfortunately, this patch got larger because I had to modify one more function and rebaseline one more test to make it work.

[Eric Seidel (no email)]

Comment on attachment 38971 [details]
new patch

In general this looks good.

CSSDecorationAddedByTag should be "cssDecorationAddedByTag"

And here:
 65     bool shouldRemoveTextDecorationTag(CSSStyleDeclaration* styleToApply, int CSSDecorationAddedByTag) const;

You could have used locals to store "nextElement" instead of casting nextSibling twice.  But it's also OK as is.

LGTM.

[Ryosuke Niwa]

Landed as http://trac.webkit.org/changeset/49414.