Bug 192640

Summary:

Make HTMLConverter work across shadow boundaries

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

HTML Editing

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin, ews-watchlist, megan_gardner, rniwa, sam, thorton, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

192613, 192653

Bug Blocks:

Attachments:

Description	Flags
Fixes the bug	none
Archive of layout-test-results from ews103 for mac-sierra	none
Patch for landing	none
Archive of layout-test-results from ews102 for mac-sierra	none
Patch for landing	none

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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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>