179431 – Inserting an image, selecting, underlining, and then deleting leaves the typing style with both "-webkit-text-decorations-in-effect" and "text-decoration"

RESOLVED FIXED 179431

Inserting an image, selecting, underlining, and then deleting leaves the typing style with both "-webkit-text-decorations-in-effect" and "text-decoration"

https://bugs.webkit.org/show_bug.cgi?id=179431

Summary Inserting an image, selecting, underlining, and then deleting leaves the typi...

Wenson Hsieh

Reported 2017-11-08 10:38:24 PST

...this subsequently triggers a debug assert when inserting text. Reduced test case: <body contenteditable></body> <script> document.body.focus(); document.execCommand("InsertHTML", true, "<img></img>"); document.execCommand("SelectAll"); document.execCommand("Underline"); document.execCommand("InsertText", true, "foo"); </script>

Attachments
Patch (5.90 KB, patch) 2017-11-08 12:50 PST, Wenson Hsieh	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews102 for mac-elcapitan (2.27 MB, application/zip) 2017-11-08 13:45 PST, Build Bot	no flags	Details
Patch (12.14 KB, patch) 2017-11-08 13:55 PST, Wenson Hsieh	no flags	Details Formatted Diff Diff
Fix WebCore/ChangeLog (12.14 KB, patch) 2017-11-08 16:33 PST, Wenson Hsieh	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Wenson Hsieh

Comment 1 2017-11-08 10:40:43 PST

The crash log looks something like: 1 0x49ef288fd WTFCrash 2 0x491e3f694 WebCore::reconcileTextDecorationProperties(WebCore::MutableStyleProperties*) 3 0x491e3f004 WebCore::StyleChange::StyleChange(WebCore::EditingStyle*, WebCore::Position const&) 4 0x491e3fce5 WebCore::StyleChange::StyleChange(WebCore::EditingStyle*, WebCore::Position const&) 5 0x491e0bf94 WebCore::ApplyStyleCommand::applyInlineStyleToNodeRange(WebCore::EditingStyle&, WebCore::Node&, WebCore::Node*) 6 0x491e0b76d WebCore::ApplyStyleCommand::fixRangeAndApplyInlineStyle(WebCore::EditingStyle&, WebCore::Position const&, WebCore::Position const&) 7 0x491e0789c WebCore::ApplyStyleCommand::applyInlineStyle(WebCore::EditingStyle&) 8 0x491e05439 WebCore::ApplyStyleCommand::doApply() 9 0x491e12eef WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand>&&) 10 0x491e13423 WebCore::CompositeEditCommand::applyStyle(WebCore::EditingStyle const*, WebCore::EditAction) 11 0x491e86867 WebCore::InsertTextCommand::doApply() 12 0x491e13242 WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::CompositeEditCommand>&&, WebCore::VisibleSelection const&) 13 0x491eb5d68 WebCore::TypingCommand::insertTextRunWithoutNewlines(WTF::String const&, bool) 14 0x491ecfafc WebCore::TypingCommandLineOperation::operator()(unsigned long, unsigned long, bool) const 15 0x491eb5c19 void WebCore::forEachLineInString<WebCore::TypingCommandLineOperation>(WTF::String const&, WebCore::TypingCommandLineOperation const&) 16 0x491eb5b38 WebCore::TypingCommand::insertText(WTF::String const&, bool) 17 0x491eb41d5 WebCore::TypingCommand::insertTextAndNotifyAccessibility(WTF::String const&, bool) 18 0x491eb4e10 WebCore::TypingCommand::doApply() 19 0x491dff08a WebCore::CompositeEditCommand::apply() 20 0x491ea4ff3 WebCore::TextInsertionBaseCommand::applyTextInsertionCommand(WebCore::Frame*, WebCore::TextInsertionBaseCommand&, WebCore::VisibleSelection const&, WebCore::VisibleSelection const&) 21 0x491eb408c WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, WebCore::VisibleSelection const&, unsigned int, WebCore::TypingCommand::TextCompositionType) 22 0x491eb3d6b WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, unsigned int, WebCore::TypingCommand::TextCompositionType)

Wenson Hsieh

Comment 2 2017-11-08 11:57:39 PST

The debug assertion being hit is in reconcileTextDecorationProperties: ASSERT(!textDecorationsInEffect || !textDecoration); so we expect that "-webkit-text-decorations-in-effect" does not appear in the EditingStyle simultaneously as "text-decoration". This was added way back in <https://trac.webkit.org/r46286>. This pathological EditingStyle is computed in DeleteSelectionCommand::saveTypingStyleState, within this line: m_typingStyle = EditingStyle::create(m_selectionToDelete.start(), EditingStyle::EditingPropertiesInEffect); in particular, by this chunk of code, in EditingStyle::init: if (propertiesToInclude == EditingPropertiesInEffect) { if (RefPtr<CSSValue> value = backgroundColorInEffect(node)) m_mutableStyle->setProperty(CSSPropertyBackgroundColor, value->cssText()); if (RefPtr<CSSValue> value = computedStyleAtPosition.propertyValue(CSSPropertyWebkitTextDecorationsInEffect)) m_mutableStyle->setProperty(CSSPropertyTextDecoration, value->cssText()); } which adds the "text-decoration" property to EditingStyle if "-webkit-text-decorations-in-effect" is present. This logic was added in <https://trac.webkit.org/r98794>. So the situation looks like this right now: 1. DeleteSelectionCommand::saveTypingStyleState computes a typing style 2. In doing so, it calls into EditingStyle::init, which observes that "-webkit-text-decorations-in-effect" is present and tacks on a "text-decoration" with an identical CSS value to the EditingStyle's mutable style properties. 3. DeleteSelectionCommand::calculateTypingStyleAfterDelete sets the current selection's typing style to the above typing style 4. Later on, when we try to insert text, we compute the StyleChange using the above typing style, which calls into reconcileTextDecorationProperties. 5. reconcileTextDecorationProperties debug asserts that "-webkit-text-decorations-in-effect" and "text-decoration" don't coexist on the EditingStyle's (i.e. the typing style's) mutable properties. 6. We crash on debug, because (2) made sure that both properties are present! At a quick glance, there are a few ways to address this: (a) Remove the debug assert in reconcileTextDecorationProperties, and have it instead remove one of the text decoration properties if both of them are present. In other words, don't have reconcileTextDecorationProperties crash if both properties exist; instead, make reconcileTextDecorationProperties actually reconcile these two properties. -or- (b) Tweak the debug assert so it only fires if the values of "-webkit-text-decorations-in-effect" and "text-decoration" are different. Is there actually any harm in having both properties on the EditingStyle, if they agree on value? -or- (c) Change EditingStyle::init so that when it doesn't result in having both "-webkit-text-decorations-in-effect" and "text-decoration", by removing "-webkit-text-decorations-in-effect" from the mutable style property after adding "text-decoration". FWIW, I ran the tests in editing/ against iOS simulator after making this change, and nothing failed.

Wenson Hsieh

Comment 3 2017-11-08 11:59:16 PST

(footnote: fixing this would allow me to reland my patch in https://bugs.webkit.org/show_bug.cgi?id=179016, which contained an API test that triggered this existing debug assert)

Wenson Hsieh

Comment 4 2017-11-08 12:50:19 PST

Created attachment 326348 [details] Patch

Build Bot

Comment 5 2017-11-08 13:45:32 PST

Comment on attachment 326348 [details] Patch Attachment 326348 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5151909 New failing tests: editing/execCommand/underline-selection-containing-image.html fast/events/before-input-events-prevent-drag-and-drop.html fast/events/ondrop-text-html.html fast/events/input-events-paste-rich-datatransfer.html editing/pasteboard/data-transfer-get-data-on-drop-rich-text.html

Build Bot

Comment 6 2017-11-08 13:45:33 PST

Created attachment 326361 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Wenson Hsieh

Comment 7 2017-11-08 13:55:33 PST

Created attachment 326366 [details] Patch

Wenson Hsieh

Comment 8 2017-11-08 16:33:39 PST

Created attachment 326402 [details] Fix WebCore/ChangeLog

Ryosuke Niwa

Comment 9 2017-11-09 11:47:37 PST

Comment on attachment 326402 [details] Fix WebCore/ChangeLog Seems okay. r=me.

WebKit Commit Bot

Comment 10 2017-11-09 15:26:59 PST

Comment on attachment 326402 [details] Fix WebCore/ChangeLog Clearing flags on attachment: 326402 Committed r224649: <https://trac.webkit.org/changeset/224649>

WebKit Commit Bot

Comment 11 2017-11-09 15:27:00 PST

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 12 2017-11-15 09:39:08 PST

<rdar://problem/35562150>

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component HTML Editing

Assignee

Wenson Hsieh

Reported

2017-11-08 10:38 PST

Modified

2017-11-15 09:39 PST History

CC List

8 users Show

URL

Keywords InRadar

Depends on

Blocks