Bug 43017 - removeFormat needs to be reimplemented
: removeFormat needs to be reimplemented
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: All All
: P2 Major
Assigned To:
:
: NeedsReduction
: 47424 47782 47784 47792
: 13125 17926 20216 21843 29164
  Show dependency treegraph
 
Reported: 2010-07-26 16:36 PST by
Modified: 2010-11-11 10:36 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-07-26 16:36:08 PST
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 From 2010-07-26 17:30:21 PST -------
(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 From 2010-07-29 13:22:58 PST -------
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 From 2010-07-29 14:41:59 PST -------
Created an attachment (id=62994) [details]
test script to be ran on major browsers
------- Comment #4 From 2010-07-29 14:43:04 PST -------
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 From 2010-09-03 15:35:11 PST -------
Created an attachment (id=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 From 2010-09-17 16:19:31 PST -------
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 From 2010-09-17 16:39:05 PST -------
Created an attachment (id=67970) [details]
test CSS properties

This is my wasted effort to test removal of CSS properties in Firefox.
------- Comment #8 From 2010-09-24 14:42:56 PST -------
Created an attachment (id=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 From 2010-09-24 14:50:25 PST -------
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 From 2010-10-17 22:33:08 PST -------
Created an attachment (id=70999) [details]
work in progress
------- Comment #11 From 2010-10-19 01:01:37 PST -------
Created an attachment (id=71140) [details]
work in progress 2
------- Comment #12 From 2010-10-19 15:22:30 PST -------
Created an attachment (id=71209) [details]
reimplements RemoveFormat
------- Comment #13 From 2010-10-19 15:24:12 PST -------
(In reply to comment #12)
> Created an attachment (id=71209) [details] [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 From 2010-10-21 11:08:43 PST -------
(From update of attachment 71209 [details])
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 From 2010-10-21 15:46:42 PST -------
Created an attachment (id=71505) [details]
fixed per Tony's comment
------- Comment #16 From 2010-10-21 16:21:10 PST -------
(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.
------- Comment #17 From 2010-10-21 17:29:26 PST -------
(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?
------- Comment #18 From 2010-10-21 17:36:38 PST -------
(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 71505 [details] [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 From 2010-10-21 17:47:38 PST -------
(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 From 2010-10-21 18:30:19 PST -------
Committed r70283: <http://trac.webkit.org/changeset/70283>
------- Comment #21 From 2010-10-21 19:04:01 PST -------
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 From 2010-10-21 19:38:42 PST -------
Thanks for the review as always, Tony.