Bug 28091 - ApplyStyleCommand removes presentational tags even when not necessary
Summary: ApplyStyleCommand removes presentational tags even when not necessary
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P3 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-07 18:43 PDT by Ryosuke Niwa
Modified: 2009-10-09 20:39 PDT (History)
3 users (show)

See Also:


Attachments
fixes the bug (19.88 KB, patch)
2009-08-07 19:02 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
new patch (28.72 KB, patch)
2009-09-03 00:58 PDT, Ryosuke Niwa
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2009-08-07 18:43:54 PDT
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.)
Comment 1 Ryosuke Niwa 2009-08-07 19:02:44 PDT
Created attachment 34355 [details]
fixes the bug
Comment 2 Adam Barth 2009-09-01 17:57:26 PDT
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)?
Comment 3 Ryosuke Niwa 2009-09-01 21:43:47 PDT
(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.
Comment 4 Ryosuke Niwa 2009-09-03 00:58:59 PDT
Created attachment 38971 [details]
new patch
Comment 5 Ryosuke Niwa 2009-09-03 01:04:28 PDT
Unfortunately, this patch got larger because I had to modify one more function and rebaseline one more test to make it work.
Comment 6 Eric Seidel (no email) 2009-10-06 09:42:14 PDT
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.
Comment 7 Ryosuke Niwa 2009-10-09 20:39:09 PDT
Landed as http://trac.webkit.org/changeset/49414.