Bug 43017 - removeFormat needs to be reimplemented
: removeFormat needs to be reimplemented
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: All All
: P2 Major
Assigned To: Ryosuke Niwa
: NeedsReduction
Depends on: 47424 47782 47784 47792
Blocks: 13125 17926 20216 21843 29164
  Show dependency treegraph
 
Reported: 2010-07-26 16:36 PDT by Ryosuke Niwa
Modified: 2010-11-11 10:36 PST (History)
12 users (show)

See Also:


Attachments
test elements (2.72 KB, text/html)
2010-07-29 14:41 PDT, Ryosuke Niwa
no flags Details
test elements (works on Firefox and MSIE) (4.20 KB, text/html)
2010-09-03 15:35 PDT, Ryosuke Niwa
no flags Details
test CSS properties (6.79 KB, text/html)
2010-09-17 16:39 PDT, Ryosuke Niwa
no flags Details
test CSS properties (MSIE / Firefox) (7.03 KB, text/html)
2010-09-24 14:42 PDT, Ryosuke Niwa
no flags Details
work in progress (2.93 KB, patch)
2010-10-17 22:33 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
work in progress 2 (18.39 KB, patch)
2010-10-19 01:01 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
reimplements RemoveFormat (37.83 KB, patch)
2010-10-19 15:22 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed per Tony's comment (61.78 KB, patch)
2010-10-21 15:46 PDT, Ryosuke Niwa
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-07-26 16:36:08 PDT
The current implementation of RemoveFormatCommand deletes the entire selection and put them back using insertText.  But this deletes all elements within the selection.  We must implement a new version of RemoveFormatCommand that matches the behaviors of other browsers such as MSIE and Firefox.

As discussed on webkit-dev, we need to do the following:

1. Determine what each browser do for every HTML / CSS element on removeFormat
2. Rewrite RemoveFormat to match the desired behavior
Comment 1 Ojan Vafai 2010-07-26 17:30:21 PDT
(In reply to comment #0)
> 1. Determine what each browser do for every HTML / CSS element on removeFormat

I think it would be sufficient to test the following:
1. Every HTML element with no CSS.
2. Div with each CSS value/property (one at a time probably).
3. Span with each CSS value/property.
Comment 2 Ryosuke Niwa 2010-07-29 13:22:58 PDT
HTML elements supported by WebKit:

a, abbr, acronym, address, applet, area, article, aside, audio, b, base, basefont, bdo, bgsound, big, blockquote, body, br, button, canvas, caption, center, cite, code, col, colgroup, command, datagrid, datalist, dcell, dcol, dd, del, details, dfn, dir, div, dl, drow, dt, em, embed, fieldset, figcaption, figure, font, footer, form, frame, frameset, h1, h2, h3, h4, h5, h6, head, header, hgroup, hr, html, i, iframe, image, img, input, ins, isindex, kbd, keygen, label, layer, legend, li, link, listing, map, mark, marquee, menu, meta, meter, nav, nobr, noembed, noframes, nolayer, noscript, object, ol, optgroup, option, p, param, plaintext, pre, progress, q, rp, rt, ruby, s, samp, script, section, select, small, source, span, strike, strong, style, sub, summary, sup, table, tbody, td, textarea, tfoot, th, thead, title, tr, track, tt, u, ul, var, video, wbr, xmp

CSS properties supported by WebKit:

color, direction, display, font, font-family, font-size, font-style, font-variant, font-weight, text-rendering, -webkit-text-size-adjust, zoom, line-height, background, background-attachment, background-clip, background-color, background-image, background-origin, background-position, background-position-x, background-position-y, background-repeat, background-repeat-x, background-repeat-y, background-size, border, border-bottom, border-bottom-color, border-bottom-left-radius, border-bottom-right-radius, border-bottom-style, border-bottom-width, border-collapse, border-color, border-left, border-left-color, border-left-style, border-left-width, border-radius, border-right, border-right-color, border-right-style, border-right-width, border-spacing, border-style, border-top, border-top-color, border-top-left-radius, border-top-right-radius, border-top-style, border-top-width, border-width, bottom, caption-side, clear, clip, content, counter-increment, counter-reset, cursor, empty-cells, float, font-stretch, height, left, letter-spacing, list-style, list-style-image, list-style-position, list-style-type, margin, margin-bottom, margin-left, margin-right, margin-top, max-height, max-width, min-height, min-width, opacity, orphans, outline, outline-color, outline-offset, outline-style, outline-width, overflow, overflow-x, overflow-y, padding, padding-bottom, padding-left, padding-right, padding-top, page, page-break-after, page-break-before, page-break-inside, pointer-events, position, quotes, resize, right, size, src, table-layout, text-align, text-decoration, text-indent, text-line-through, text-line-through-color, text-line-through-mode, text-line-through-style, text-line-through-width, text-overflow, text-overline, text-overline-color, text-overline-mode, text-overline-style, text-overline-width, text-shadow, text-transform, text-underline, text-underline-color, text-underline-mode, text-underline-style, text-underline-width, top, unicode-bidi, unicode-range, vertical-align, visibility, white-space, widows, width, word-break, word-spacing, word-wrap, z-index, -webkit-animation, -webkit-animation-delay, -webkit-animation-direction, -webkit-animation-duration, -webkit-animation-fill-mode, -webkit-animation-iteration-count, -webkit-animation-name, -webkit-animation-play-state, -webkit-animation-timing-function, -webkit-appearance, -webkit-backface-visibility, -webkit-background-clip, -webkit-background-composite, -webkit-background-origin, -webkit-background-size, -webkit-binding, -webkit-border-end, -webkit-border-end-color, -webkit-border-end-style, -webkit-border-end-width, -webkit-border-fit, -webkit-border-horizontal-spacing, -webkit-border-image, -webkit-border-radius, -webkit-border-start, -webkit-border-start-color, -webkit-border-start-style, -webkit-border-start-width, -webkit-border-vertical-spacing, -webkit-box-align, -webkit-box-direction, -webkit-box-flex, -webkit-box-flex-group, -webkit-box-lines, -webkit-box-ordinal-group, -webkit-box-orient, -webkit-box-pack, -webkit-box-reflect, -webkit-box-shadow, -webkit-box-sizing, -webkit-color-correction, -webkit-column-break-after, -webkit-column-break-before, -webkit-column-break-inside, -webkit-column-count, -webkit-column-gap, -webkit-column-rule, -webkit-column-rule-color, -webkit-column-rule-style, -webkit-column-rule-width, -webkit-column-span, -webkit-column-width, -webkit-columns, -webkit-font-size-delta, -webkit-font-smoothing, -webkit-highlight, -webkit-hyphenate-character, -webkit-hyphens, -webkit-line-break, -webkit-line-clamp, -webkit-margin-bottom-collapse, -webkit-margin-collapse, -webkit-margin-end, -webkit-margin-start, -webkit-margin-top-collapse, -webkit-marquee, -webkit-marquee-direction, -webkit-marquee-increment, -webkit-marquee-repetition, -webkit-marquee-speed, -webkit-marquee-style, -webkit-mask, -webkit-mask-attachment, -webkit-mask-box-image, -webkit-mask-clip, -webkit-mask-composite, -webkit-mask-image, -webkit-mask-origin, -webkit-mask-position, -webkit-mask-position-x, -webkit-mask-position-y, -webkit-mask-repeat, -webkit-mask-repeat-x, -webkit-mask-repeat-y, -webkit-mask-size, -webkit-match-nearest-mail-blockquote-color, -webkit-nbsp-mode, -webkit-padding-end, -webkit-padding-start, -webkit-perspective, -webkit-perspective-origin, -webkit-perspective-origin-x, -webkit-perspective-origin-y, -webkit-rtl-ordering, -webkit-text-decorations-in-effect, -webkit-text-fill-color, -webkit-text-security, -webkit-text-stroke, -webkit-text-stroke-color, -webkit-text-stroke-width, -webkit-transform, -webkit-transform-origin, -webkit-transform-origin-x, -webkit-transform-origin-y, -webkit-transform-origin-z, -webkit-transform-style, -webkit-transition, -webkit-transition-delay, -webkit-transition-duration, -webkit-transition-property, -webkit-transition-timing-function, -webkit-user-drag, -webkit-user-modify, -webkit-user-select, -webkit-variable-declaration-block,
Comment 3 Ryosuke Niwa 2010-07-29 14:41:59 PDT
Created attachment 62994 [details]
test elements
Comment 4 Ryosuke Niwa 2010-07-29 14:43:04 PDT
Result on Firefox.

Removed
    abbr, acronym, applet, article, aside, b, bdo, big, button, caption, cite, code, colgroup, command, datagrid, datalist, dcell, dcol, del, details, dfn, drow, em, figcaption, figure, font, footer, frameset, header, hgroup, html, i, iframe, ins, kbd, label, layer, legend, map, mark, meter, nav, nolayer, object, optgroup, option, progress, q, rp, rt, ruby, s, samp, section, select, small, span, strike, strong, sub, summary, sup, textarea, track, tt, u, var
Preserved
    noscript, a, address, area, audio, base, basefont, bgsound, blockquote, body, br, canvas, center, col, dd, dir, div, dl, dt, embed, fieldset, form, frame, h1, h2, h3, h4, h5, h6, head, hr, image, img, input, isindex, keygen, li, link, listing, marquee, menu, meta, nobr, noembed, noframes, noscript, ol, p, param, plaintext, pre, script, source, style, table, tbody, td, tfoot, th, thead, title, tr, ul, video, wbr, xmp
Exceptions
Comment 5 Ryosuke Niwa 2010-09-03 15:35:11 PDT
Created attachment 66559 [details]
test elements (works on Firefox and MSIE)

Firefox:
Removed
    abbr, acronym, applet, article, aside, b, bdo, big, button, cite, code, command, datagrid, datalist, dcell, dcol, del, details, dfn, drow, em, figcaption, figure, font, footer, frameset, header, hgroup, html, i, iframe, ins, kbd, label, layer, legend, map, mark, meter, nav, nolayer, object, optgroup, option, progress, q, rp, rt, ruby, s, samp, section, select, small, span, strike, strong, sub, summary, sup, textarea, track, tt, u, var
Preserved
    noscript, a, address, area, audio, base, basefont, bgsound, blockquote, body, br, canvas, center, col, dd, dir, div, dl, dt, embed, fieldset, form, frame, h1, h2, h3, h4, h5, h6, head, hr, image, img, input, isindex, keygen, li, link, listing, marquee, menu, meta, nobr, noembed, noframes, noscript, ol, p, param, plaintext, pre, script, source, style, table, tbody, td, tfoot, th, thead, title, tr, ul, video, wbr, xmp
Exceptions
    caption (<table>hello, world</table> webkit)
    colgroup (<table>hello, world</table> webkit)

MSIE:

Removed 
    acronym, b, bdo, big, cite, code, dfn, em, font, i, ins, kbd, nobr, q, s, samp, small, strike, strong, sub, sup, tt, u, var
Preserved 
    noscript, a, abbr, address, applet, area, article, aside, audio, base, basefont, bgsound, blockquote, body, br, button, canvas, caption, center, colgroup, command, datagrid, datalist, dcell, dcol, dd, del, details, dir, div, dl, drow, dt, embed, fieldset, figcaption, figure, footer, form, frame, h1, h2, h3, h4, h5, h6, head, header, hgroup, hr, html, iframe, image, img, input, isindex, keygen, label, layer, legend, li, link, listing, map, mark, marquee, menu, meta, meter, nav, noembed, noframes, nolayer, noscript, object, ol, optgroup, option, p, param, plaintext, pre, progress, rp, rt, ruby, script, section, select, source, span, style, summary, table, tbody, td, textarea, tfoot, th, thead, title, tr, track, ul, video, wbr, xmp
Exceptions 
    col (<TABLE><COL></TABLE> webkit) failed to select
    frameset (<FRAMESET>hello, world</FRAMESET> webkit) failed to select
Comment 6 Ryosuke Niwa 2010-09-17 16:19:31 PDT
Firefox does something interesting for inline styles.  It removes span regardless of inline styles so I can't really test which CSS property it removes and doesn't remove any CSS property for elements that it doesn't remove.  e.g. all properties on p will be preserved.
Comment 7 Ryosuke Niwa 2010-09-17 16:39:05 PDT
Created attachment 67970 [details]
test CSS properties

This is my wasted effort to test removal of CSS properties in Firefox.
Comment 8 Ryosuke Niwa 2010-09-24 14:42:56 PDT
Created attachment 68752 [details]
test CSS properties (MSIE / Firefox)

It seems like MSIE does the same for CSS properties.  In fact, MSIE does not seem to delete any CSS property at all, which IMHO is wrong because user can never tell the difference between <b>hello</b> and <span style="font-weight: bold;">hello</span> but then MSIE does not support styleWithCSS so we might say that they're being consistent.
Comment 9 Ryosuke Niwa 2010-09-24 14:50:25 PDT
I think the most reasonable thing we can do here is to remove all elements that MSIE deletes (I feel that Firefox delete too much or maybe that my test has a bug).  We should remove corresponding CSS styles even though this will be inconsistent with MSIE because we DO support styleWithCSS.
Comment 10 Ryosuke Niwa 2010-10-17 22:33:08 PDT
Created attachment 70999 [details]
work in progress
Comment 11 Ryosuke Niwa 2010-10-19 01:01:37 PDT
Created attachment 71140 [details]
work in progress 2
Comment 12 Ryosuke Niwa 2010-10-19 15:22:30 PDT
Created attachment 71209 [details]
reimplements RemoveFormat
Comment 13 Ryosuke Niwa 2010-10-19 15:24:12 PDT
(In reply to comment #12)
> Created an attachment (id=71209) [details]
> reimplements RemoveFormat

I didn't add many tests in this patch because this patch already rebaselines a lot of tests and quite large.  I intend to add some scripts tests in a follow up patch for a better test coverage (particularly undo/redo).
Comment 14 Tony Chang 2010-10-21 11:08:43 PDT
Comment on attachment 71209 [details]
reimplements RemoveFormat

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

Code looks fine, just some naming nits.

> WebCore/editing/ApplyStyleCommand.h:131
> +    bool (*m_isInlineElementToRemove)(const Element*);

Can we name this m_isInlineElementToRemoveFunction?  This seems to follow the naming convention in other parts of the code.  Also, can you make a typedef?  In create(), it's particularly hard to read.

> LayoutTests/editing/execCommand/remove-format-elements.html:70
> +        elementName = elementList[elementList.length-1]

Nit: spaces round the minus sign

> LayoutTests/editing/execCommand/remove-format-elements.html:92
> +    document.execCommand('removeFormat', false, null);

This tests each element one at a time.  Would it be possible to add a test that has multiple elements in it?
Comment 15 Ryosuke Niwa 2010-10-21 15:46:42 PDT
Created attachment 71505 [details]
fixed per Tony's comment
Comment 16 Tony Chang 2010-10-21 16:21:10 PDT
Comment on attachment 71505 [details]
fixed per Tony's comment

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

> WebCore/editing/ApplyStyleCommand.cpp:1536
> +    Vector<RefPtr<Element> > elementsToPushDown;

Nit: Maybe add a comment explaining why you need this vector.  I don't think it's obvious from the code.
Comment 17 Ryosuke Niwa 2010-10-21 17:29:26 PDT
(In reply to comment #16)
> (From update of attachment 71505 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71505&action=review
> 
> > WebCore/editing/ApplyStyleCommand.cpp:1536
> > +    Vector<RefPtr<Element> > elementsToPushDown;
> 
> Nit: Maybe add a comment explaining why you need this vector.  I don't think it's obvious from the code.

You mean into the code?  or to the change log?
Comment 18 Tony Chang 2010-10-21 17:36:38 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 71505 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=71505&action=review
> > 
> > > WebCore/editing/ApplyStyleCommand.cpp:1536
> > > +    Vector<RefPtr<Element> > elementsToPushDown;
> > 
> > Nit: Maybe add a comment explaining why you need this vector.  I don't think it's obvious from the code.
> 
> You mean into the code?  or to the change log?

The code.  The description in the ChangeLog seems sufficient.
Comment 19 Ryosuke Niwa 2010-10-21 17:47:38 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > > Nit: Maybe add a comment explaining why you need this vector.  I don't think it's obvious from the code.
> > 
> > You mean into the code?  or to the change log?
> 
> The code.  The description in the ChangeLog seems sufficient.

    // The outer loop is traversing the tree vertically from highestAncestor to targetNode
    Node* current = highestAncestor;
    // Along the way, styled elements that contain targetNode are removed and accumulated into elementsToPushDown.
    // Each child of the removed element, exclusing ancestors of targetNode, is then wrapped by clones of elements in elementsToPushDown.
    Vector<RefPtr<Element> > elementsToPushDown;
    while (current != targetNode) {

How does this sound?
Comment 20 Ryosuke Niwa 2010-10-21 18:30:19 PDT
Committed r70283: <http://trac.webkit.org/changeset/70283>
Comment 21 WebKit Review Bot 2010-10-21 19:04:01 PDT
http://trac.webkit.org/changeset/70283 might have broken Qt Linux Release
The following tests are not passing:
editing/execCommand/remove-format-elements.html
Comment 22 Ryosuke Niwa 2010-10-21 19:38:42 PDT
Thanks for the review as always, Tony.