Bug 164578

Summary: Copied text would contain text inside CDATA sections and comments
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, darin, enrica, rniwa, sam, zcorpan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 164501    
Bug Blocks:    
Attachments:
Description Flags
Fixes the bug
none
Archive of layout-test-results from ews101 for mac-yosemite
none
Patch for landing none

Description Ryosuke Niwa 2016-11-09 20:02:51 PST
WebKit currently serializes text inside comments and CDATA sections but we shouldn't.

<rdar://problem/19834542>
Comment 1 Ryosuke Niwa 2016-11-10 01:04:18 PST
Created attachment 294343 [details]
Fixes the bug
Comment 2 Build Bot 2016-11-10 02:02:57 PST
Comment on attachment 294343 [details]
Fixes the bug

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

New failing tests:
editing/mac/attributed-string/comment-cdata-section.html
Comment 3 Build Bot 2016-11-10 02:03:00 PST
Created attachment 294350 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 4 Darin Adler 2016-11-10 09:03:13 PST
Won’t review yet: The new test is failing on the bot. It seems peculiar to always skip CDATA; can’t those be rendered as text?
Comment 5 Darin Adler 2016-11-10 09:08:44 PST
Comment on attachment 294343 [details]
Fixes the bug

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

Not sure why the test is failing.

> Source/WebCore/ChangeLog:12
> +        In the long term, we should check the visibility of the text as done in MarkupAccumulator.

Any chance that long term we could TextIterator to convert to attributed text, instead of a separate algorithm that tries to accomplish the same thing?
Comment 6 Ryosuke Niwa 2016-11-10 12:36:44 PST
(In reply to comment #5)
> Comment on attachment 294343 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294343&action=review
> 
> Not sure why the test is failing.
> 
> > Source/WebCore/ChangeLog:12
> > +        In the long term, we should check the visibility of the text as done in MarkupAccumulator.
> 
> Any chance that long term we could TextIterator to convert to attributed
> text, instead of a separate algorithm that tries to accomplish the same
> thing?

I think in order to do that, TextIterator needs to support emitting table & list because attributed string today supports those two features.
Comment 7 Ryosuke Niwa 2016-11-10 12:59:58 PST
(In reply to comment #4)
> Won’t review yet: The new test is failing on the bot. It seems peculiar to
> always skip CDATA; can’t those be rendered as text?

No, CDATASections are like comments. They're never rendered.
Comment 8 Ryosuke Niwa 2016-11-10 13:01:01 PST
(In reply to comment #5)
> Comment on attachment 294343 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294343&action=review
> 
> Not sure why the test is failing.

I think Yosemite needs its own result due to formatting difference:

@@ -15,13 +15,11 @@
     LineBreakMode: 0
     Tabs: ()
     DefaultTabInterval: 36
-    Blocks: (
-)
-    Lists: (
-)
+    Blocks: (null)
+    Lists: (null)
     BaseWritingDirection: 0
     HyphenationFactor: 0
-    TighteningForTruncation: YES
+    TighteningFactor: 0.05
     HeaderLevel: 0
 [hello world. ]
     NSFont: Times-Roman 16.00 pt.
Comment 9 Ryosuke Niwa 2016-11-10 13:02:45 PST
Created attachment 294398 [details]
Patch for landing
Comment 10 Ryosuke Niwa 2016-11-10 13:07:19 PST
Comment on attachment 294398 [details]
Patch for landing

Wait for EWS
Comment 11 WebKit Commit Bot 2016-11-10 14:24:40 PST
Comment on attachment 294398 [details]
Patch for landing

Clearing flags on attachment: 294398

Committed r208565: <http://trac.webkit.org/changeset/208565>
Comment 12 WebKit Commit Bot 2016-11-10 14:24:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Darin Adler 2016-11-10 20:36:09 PST
(In reply to comment #6)
> I think in order to do that, TextIterator needs to support emitting table &
> list because attributed string today supports those two features.

Sure, the code using TextIterator needs to emit table and list. I think that can be built based on the ability to inspect the element that the text is inside, which is part of TextIterator's interface.
Comment 14 Simon Pieters (:zcorpan) 2016-12-07 13:04:58 PST
> No, CDATASections are like comments. They're never rendered.

No? They're like Text nodes. They're rendered.

data:text/xml,<html%20xmlns="http://www.w3.org/1999/xhtml"><body>Hello%20<![CDATA[%20World%20]]></body></html>