Bug 192640 - Make HTMLConverter work across shadow boundaries
Summary: Make HTMLConverter work across shadow boundaries
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on: 192613 192653
Blocks:
  Show dependency treegraph
 
Reported: 2018-12-12 14:32 PST by Ryosuke Niwa
Modified: 2018-12-13 17:45 PST (History)
8 users (show)

See Also:


Attachments
Fixes the bug (38.77 KB, patch)
2018-12-12 22:21 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.55 MB, application/zip)
2018-12-12 23:28 PST, Build Bot
no flags Details
Patch for landing (47.43 KB, patch)
2018-12-13 14:26 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.77 MB, application/zip)
2018-12-13 15:20 PST, Build Bot
no flags Details
Patch for landing (47.44 KB, patch)
2018-12-13 16:05 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-12-12 14:32:25 PST
We should make HTMLConverter produce a valid attributed string for a selection which crosses shadow boundaries.
Right now, this doesn't work and as a result, you can't copy & paste content across shadow boundaries from Safari to Notes, TextEdit, etc...

<rdar://problem/44968145>
Comment 1 Ryosuke Niwa 2018-12-12 22:21:44 PST
Created attachment 357208 [details]
Fixes the bug
Comment 2 Build Bot 2018-12-12 23:28:37 PST
Comment on attachment 357208 [details]
Fixes the bug

Attachment 357208 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10378667

New failing tests:
editing/mac/attributed-string/attributed-string-across-shadow-boundaries-2.html
editing/mac/attributed-string/attributed-string-across-shadow-boundaries-1.html
editing/mac/attributed-string/attributed-string-across-shadow-boundaries-with-style-2.html
editing/mac/attributed-string/attributed-string-across-shadow-boundaries-4.html
editing/mac/attributed-string/attributed-string-across-shadow-boundaries-3.html
editing/mac/attributed-string/attributed-string-across-shadow-boundaries-with-style-1.html
editing/mac/attributed-string/attributed-string-across-shadow-boundaries-5.html
Comment 3 Build Bot 2018-12-12 23:28:38 PST
Created attachment 357213 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 4 Ryosuke Niwa 2018-12-13 13:06:42 PST
Mac OS Sierra needs its own expected results :(
Comment 5 Wenson Hsieh 2018-12-13 14:13:24 PST
Comment on attachment 357208 [details]
Fixes the bug

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

r=me, with test fixes!

> Source/WebCore/dom/Position.cpp:1606
> +    TreeScope* commonScope = commonTreeScope(a.containerNode(), b.containerNode());

Nit - auto*?

> Source/WebCore/editing/cocoa/HTMLConverter.mm:2321
> +                    Node* child = firstChildInComposedTreeIgnoringUserAgentShadow(node);

Nit - auto*?

> Source/WebCore/editing/cocoa/HTMLConverter.mm:2368
> +            Node* child = firstChildInComposedTreeIgnoringUserAgentShadow(element);

Nit - auto*?

> LayoutTests/editing/mac/attributed-string/attributed-string-across-shadow-boundaries-with-style-1.html:12
> +    dumpAttributedString(container, source, 0, shadowRoot.lastChild, shadowRoot.lastChild.data.indexOf('Kit'));

What is `source` here?

> LayoutTests/editing/mac/attributed-string/attributed-string-across-shadow-boundaries-with-style-2.html:15
> +    dumpAttributedString(container, source, 0, shadowRoot2.querySelector('u'), 1);

Here too?
Comment 6 Ryosuke Niwa 2018-12-13 14:20:46 PST
(In reply to Wenson Hsieh from comment #5)
> Comment on attachment 357208 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357208&action=review
> 
> r=me, with test fixes!
> 
> > Source/WebCore/dom/Position.cpp:1606
> > +    TreeScope* commonScope = commonTreeScope(a.containerNode(), b.containerNode());
> 
> Nit - auto*?

Fixed.

> > Source/WebCore/editing/cocoa/HTMLConverter.mm:2321
> > +                    Node* child = firstChildInComposedTreeIgnoringUserAgentShadow(node);
> 
> Nit - auto*?

Fixed.

> > Source/WebCore/editing/cocoa/HTMLConverter.mm:2368
> > +            Node* child = firstChildInComposedTreeIgnoringUserAgentShadow(element);
> 
> Nit - auto*?

Fixed.

> > LayoutTests/editing/mac/attributed-string/attributed-string-across-shadow-boundaries-with-style-1.html:12
> > +    dumpAttributedString(container, source, 0, shadowRoot.lastChild, shadowRoot.lastChild.data.indexOf('Kit'));
> 
> What is `source` here?

Oops, supposed to be container. Fixed.

> > LayoutTests/editing/mac/attributed-string/attributed-string-across-shadow-boundaries-with-style-2.html:15
> > +    dumpAttributedString(container, source, 0, shadowRoot2.querySelector('u'), 1);
> 
> Here too?

Ditto.
Comment 7 Ryosuke Niwa 2018-12-13 14:26:27 PST
Created attachment 357255 [details]
Patch for landing
Comment 8 Ryosuke Niwa 2018-12-13 14:26:42 PST
Comment on attachment 357255 [details]
Patch for landing

Wait for EWS.
Comment 9 Build Bot 2018-12-13 15:20:16 PST
Comment on attachment 357255 [details]
Patch for landing

Attachment 357255 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10388127

New failing tests:
editing/mac/attributed-string/attributed-string-across-shadow-boundaries-with-style-1.html
editing/mac/attributed-string/attributed-string-across-shadow-boundaries-with-style-2.html
Comment 10 Build Bot 2018-12-13 15:20:18 PST
Created attachment 357263 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 11 Ryosuke Niwa 2018-12-13 16:05:37 PST
Created attachment 357269 [details]
Patch for landing
Comment 12 Ryosuke Niwa 2018-12-13 16:05:50 PST
Comment on attachment 357269 [details]
Patch for landing

Wait for EWS again
Comment 13 Ryosuke Niwa 2018-12-13 17:45:26 PST
Committed r239190: <https://trac.webkit.org/changeset/239190>