Bug 43017 – removeFormat needs to be reimplemented

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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

[Ojan Vafai]

(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.

[Ryosuke Niwa]

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,

[Ryosuke Niwa]

Created attachment 62994 [details]
test elements

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 67970 [details]
test CSS properties

This is my wasted effort to test removal of CSS properties in Firefox.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 70999 [details]
work in progress

[Ryosuke Niwa]

Created attachment 71140 [details]
work in progress 2

[Ryosuke Niwa]

Created attachment 71209 [details]
reimplements RemoveFormat

[Ryosuke Niwa]

(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).

[Tony Chang]

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?

[Ryosuke Niwa]

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

[Tony Chang]

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.

[Ryosuke Niwa]

(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?

[Tony Chang]

(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.

[Ryosuke Niwa]

(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?

[Ryosuke Niwa]

Committed r70283: <http://trac.webkit.org/changeset/70283>

[WebKit Review Bot]

http://trac.webkit.org/changeset/70283 might have broken Qt Linux Release
The following tests are not passing:
editing/execCommand/remove-format-elements.html

[Ryosuke Niwa]

Thanks for the review as always, Tony.