Bug 179016

Summary: [Attachment Support] Implement delegate hooks for attachment element insertion and removal
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebKit Misc.Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, mitz, rniwa, ryanhaddad, sam, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 179388    
Bug Blocks:    
Attachments:
Description Flags
First pass
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Patch
thorton: review+
Patch for landing none

Description Wenson Hsieh 2017-10-30 09:34:43 PDT 
Clients need to know when attachment elements are either inserted into or removed from the document.
Comment 1 Wenson Hsieh 2017-10-30 09:35:03 PDT 
<rdar://problem/35250890>
Comment 2 Wenson Hsieh 2017-11-05 01:45:15 PST 
Created attachment 326059 [details]
First pass
Comment 3 Build Bot 2017-11-05 03:19:39 PST 
Comment on attachment 326059 [details]
First pass

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

New failing tests:
editing/pasteboard/copy-paste-attachment.html
fast/attachment/attachment-default-icon.html
fast/attachment/attachment-respects-css-size.html
fast/attachment/attachment-rendering.html
fast/attachment/attachment-subtitle.html
accessibility/attachment-element.html
fast/attachment/attachment-select-on-click-inside-user-select-all.html
editing/pasteboard/drag-and-drop-attachment-contenteditable.html
fast/attachment/attachment-select-on-click.html
fast/attachment/attachment-title.html
fast/attachment/attachment-icon-from-file-extension.html
accessibility/mac/attachment-element-replacement-character.html
fast/attachment/attachment-subtitle-resize.html
fast/attachment/attachment-progress.html
fast/attachment/attachment-label-highlight.html
fast/attachment/attachment-uti.html
editing/pasteboard/drag-files-to-editable-element-as-attachment.html
fast/attachment/attachment-action.html
fast/attachment/attachment-folder-icon.html
fast/attachment/attachment-type-attribute.html
Comment 4 Build Bot 2017-11-05 03:19:44 PST 
Created attachment 326061 [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 5 Build Bot 2017-11-05 03:28:22 PST 
Comment on attachment 326059 [details]
First pass

Attachment 326059 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5111782

New failing tests:
editing/pasteboard/copy-paste-attachment.html
fast/attachment/attachment-default-icon.html
fast/attachment/attachment-respects-css-size.html
fast/attachment/attachment-rendering.html
fast/attachment/attachment-wrapping-action.html
accessibility/attachment-element.html
fast/attachment/attachment-select-on-click-inside-user-select-all.html
fast/attachment/attachment-select-on-click.html
accessibility/ios-simulator/attributed-string-for-range.html
fast/attachment/attachment-icon-from-file-extension.html
fast/attachment/attachment-action.html
fast/attachment/attachment-progress.html
fast/attachment/attachment-borderless.html
fast/attachment/attachment-label-highlight.html
fast/attachment/attachment-title.html
fast/attachment/attachment-subtitle.html
fast/attachment/attachment-uti.html
Comment 6 Build Bot 2017-11-05 03:28:24 PST 
Created attachment 326062 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 7 Build Bot 2017-11-05 03:30:02 PST 
Comment on attachment 326059 [details]
First pass

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

New failing tests:
editing/pasteboard/copy-paste-attachment.html
fast/attachment/attachment-default-icon.html
fast/attachment/attachment-respects-css-size.html
fast/attachment/attachment-rendering.html
fast/attachment/attachment-subtitle.html
accessibility/attachment-element.html
fast/attachment/attachment-select-on-click-inside-user-select-all.html
fast/attachment/attachment-select-on-click.html
fast/attachment/attachment-icon-from-file-extension.html
accessibility/mac/attachment-element-replacement-character.html
fast/attachment/attachment-subtitle-resize.html
fast/attachment/attachment-progress.html
fast/attachment/attachment-uti.html
fast/attachment/attachment-label-highlight.html
fast/attachment/attachment-action.html
fast/attachment/attachment-title.html
fast/attachment/attachment-type-attribute.html
fast/attachment/attachment-folder-icon.html
Comment 8 Build Bot 2017-11-05 03:30:03 PST 
Created attachment 326063 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-11-05 03:35:48 PST 
Comment on attachment 326059 [details]
First pass

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

New failing tests:
editing/pasteboard/copy-paste-attachment.html
fast/attachment/attachment-default-icon.html
fast/attachment/attachment-respects-css-size.html
fast/attachment/attachment-rendering.html
fast/attachment/attachment-subtitle.html
accessibility/attachment-element.html
fast/attachment/attachment-select-on-click-inside-user-select-all.html
fast/attachment/attachment-folder-icon.html
fast/attachment/attachment-select-on-click.html
fast/attachment/attachment-icon-from-file-extension.html
fast/attachment/attachment-subtitle-resize.html
accessibility/mac/attachment-element-replacement-character.html
fast/attachment/attachment-type-attribute.html
editing/pasteboard/drag-and-drop-attachment-contenteditable.html
fast/attachment/attachment-progress.html
fast/attachment/attachment-label-highlight.html
fast/attachment/attachment-action.html
fast/attachment/attachment-title.html
editing/pasteboard/drag-files-to-editable-element-as-attachment.html
fast/attachment/attachment-uti.html
Comment 10 Build Bot 2017-11-05 03:35:50 PST 
Created attachment 326064 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 11 Wenson Hsieh 2017-11-05 11:52:48 PST 
Created attachment 326070 [details]
Patch
Comment 12 Wenson Hsieh 2017-11-06 12:42:02 PST 
Comment on attachment 326070 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326070&action=review

Thanks Tim! (I found a couple of other little things to fix, so I'll make some tweaks before landing).

> Source/WebCore/html/HTMLAttachmentElement.h:70
>      void parseAttribute(const QualifiedName&, const AtomicString&) final;

I also meant to remove the declaration of the `HTMLAttachmentElement(const QualifiedName&, Document&, const String& identifier);` constructor above — I'll do this before landing.

> Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:53
> +#endif

Self nit - this should have an #else UNUSED_PARAM(identifier), otherwise the build would break with an unused variable if we were to somehow have ENABLE_ATTACHMENT_ELEMENT without WK_API_ENABLED.

> Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:60
> +#endif

Ditto.
Comment 13 Wenson Hsieh 2017-11-06 13:14:51 PST 
Created attachment 326151 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2017-11-06 13:46:58 PST 
Comment on attachment 326151 [details]
Patch for landing

Clearing flags on attachment: 326151

Committed r224512: <https://trac.webkit.org/changeset/224512>
Comment 15 Ryan Haddad 2017-11-07 08:51:46 PST 
WKAttachmentTests.AttachmentUpdatesWhenChangingFontStyles is failing on debug bots:

ASSERTION FAILED: !textDecorationsInEffect || !textDecoration
/Volumes/Data/slave/highsierra-debug/build/Source/WebCore/editing/EditingStyle.cpp(1544) : void WebCore::reconcileTextDecorationProperties(WebCore::MutableStyleProperties *)
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)
23  0x491e5e6f4 WebCore::executeInsertText(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)
24  0x491e4b363 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const
25  0x1043d9583 WebKit::WebPage::executeEditingCommand(WTF::String const&, WTF::String const&)
26  0x1043d8d45 WebKit::WebPage::executeEditCommand(WTF::String const&, WTF::String const&)
27  0x1043d8ceb WebKit::WebPage::executeEditCommandWithCallback(WTF::String const&, WTF::String const&, WebKit::CallbackID)
28  0x10446b1c8 void IPC::callMemberFunctionImpl<WebKit::WebPage, void (WebKit::WebPage::*)(WTF::String const&, WTF::String const&, WebKit::CallbackID), std::__1::tuple<WTF::String, WTF::String, WebKit::CallbackID>, 0ul, 1ul, 2ul>(WebKit::WebPage*, void (WebKit::WebPage::*)(WTF::String const&, WTF::String const&, WebKit::CallbackID), std::__1::tuple<WTF::String, WTF::String, WebKit::CallbackID>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul>)
29  0x10446ade0 void IPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(WTF::String const&, WTF::String const&, WebKit::CallbackID), std::__1::tuple<WTF::String, WTF::String, WebKit::CallbackID>, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul> >(std::__1::tuple<WTF::String, WTF::String, WebKit::CallbackID>&&, WebKit::WebPage*, void (WebKit::WebPage::*)(WTF::String const&, WTF::String const&, WebKit::CallbackID))
30  0x10445a577 void IPC::handleMessage<Messages::WebPage::ExecuteEditCommandWithCallback, WebKit::WebPage, void (WebKit::WebPage::*)(WTF::String const&, WTF::String const&, WebKit::CallbackID)>(IPC::Decoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(WTF::String const&, WTF::String const&, WebKit::CallbackID))
31  0x1044539da WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::Decoder&)
2017-11-07 03:38:07.351 TestWebKitAPI[63790:10229547] Expected removed attachments: (
) to match (
    "<_WKAttachment id='45415eb0-d28a-4cb5-8686-ef536d90f4ed'>"
).
FAIL WKAttachmentTests.AttachmentUpdatesWhenChangingFontStyles

/Volumes/Data/slave/highsierra-debug/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:174
Value of: [self _synchronouslyExecuteEditCommand:command argument:argument]
  Actual: false
Expected: true


/Volumes/Data/slave/highsierra-debug/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:101
Value of: removedAttachmentsMatch
  Actual: false
Expected: true

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/builds/712
Comment 16 Ryan Haddad 2017-11-07 12:54:41 PST 
Rolled out in <https://trac.webkit.org/changeset/224544>
Comment 17 Wenson Hsieh 2017-11-08 07:40:48 PST 
Upon further investigation, this API test runs into an existing bug related to text decorations — this is reproducible with the following simple layout test (run from within LayoutTests/editing/execCommand/)

<body contenteditable></body>
<script>
document.body.focus();
document.execCommand("InsertHTML", true, "<img src='../resources/abe.png'></img>");
document.execCommand("SelectAll");
document.execCommand("Underline");
document.execCommand("InsertText", true, "foo");
</script>
Comment 18 Wenson Hsieh 2017-11-08 13:13:39 PST 
Re-landed with a tweaked API test in <https://trac.webkit.org/r224593>. I've investigated the debug assertion, and determined that it's a separate (and existing) issue that has nothing to do with attachments. See <https://bugs.webkit.org/show_bug.cgi?id=179431> for additional analysis, as well as several proposals for a fix.