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
(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.
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,
Created attachment 62994 [details] test elements
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
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
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.
Created attachment 67970 [details] test CSS properties This is my wasted effort to test removal of CSS properties in Firefox.
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.
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.
Created attachment 70999 [details] work in progress
Created attachment 71140 [details] work in progress 2
Created attachment 71209 [details] reimplements RemoveFormat
(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 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?
Created attachment 71505 [details] fixed per Tony's comment
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.
(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?
(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.
(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?
Committed r70283: <http://trac.webkit.org/changeset/70283>
http://trac.webkit.org/changeset/70283 might have broken Qt Linux Release The following tests are not passing: editing/execCommand/remove-format-elements.html
Thanks for the review as always, Tony.