Bug 179431 - Inserting an image, selecting, underlining, and then deleting leaves the typing style with both "-webkit-text-decorations-in-effect" and "text-decoration"
Summary: Inserting an image, selecting, underlining, and then deleting leaves the typi...
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
Keywords: InRadar
Depends on:
Reported: 2017-11-08 10:38 PST by Wenson Hsieh
Modified: 2017-11-15 09:39 PST (History)
8 users (show)

See Also:

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

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2017-11-08 10:38:24 PST
...this subsequently triggers a debug assert when inserting text.

Reduced test case:

<body contenteditable></body>
document.execCommand("InsertHTML", true, "<img></img>");
document.execCommand("InsertText", true, "foo");
Comment 1 Wenson Hsieh 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)
Comment 2 Wenson Hsieh 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.


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


    (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.
Comment 3 Wenson Hsieh 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)
Comment 4 Wenson Hsieh 2017-11-08 12:50:19 PST
Created attachment 326348 [details]
Comment 5 Build Bot 2017-11-08 13:45:32 PST
Comment on attachment 326348 [details]

Attachment 326348 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5151909

New failing tests:
Comment 6 Build Bot 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
Comment 7 Wenson Hsieh 2017-11-08 13:55:33 PST
Created attachment 326366 [details]
Comment 8 Wenson Hsieh 2017-11-08 16:33:39 PST
Created attachment 326402 [details]
Fix WebCore/ChangeLog
Comment 9 Ryosuke Niwa 2017-11-09 11:47:37 PST
Comment on attachment 326402 [details]
Fix WebCore/ChangeLog

Seems okay. r=me.
Comment 10 WebKit Commit Bot 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>
Comment 11 WebKit Commit Bot 2017-11-09 15:27:00 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2017-11-15 09:39:08 PST