Bug 157630

Summary: REGRESSION (r192098): Content missing after copy and paste to Notes App on retina displays
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: Layout and RenderingAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, commit-queue, darin, dino, hyatt, simon.fraser, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch (nearly all of this is test case)
none
Patch (most is just test case). Updated for iOS
none
Patch (nearly all of this is just test case) simon.fraser: review+

Brent Fulgham
Reported 2016-05-12 11:03:27 PDT
Web content that uses the <img> 'sizes' attribute, and modifies the "src" attribute of the <img> element to reflect the "best" image for the current display resolution does not work properly after r192098 (which exposed the 'sizes' attribute). Test case: 1. Copy text/images from Safari Technology Preview. (Example: Copy content from reader view of: http://www.bonappetit.com/test-kitchen/ingredients/article/breadcrumb-substitutions) 2. Create a new note in the Notes application. 3. Paste Result: Only the first and last image from the blog post makes it to the Note.
Attachments
Patch (2.53 KB, patch)
2016-05-12 11:09 PDT, Brent Fulgham
no flags
Patch (nearly all of this is test case) (615.22 KB, patch)
2016-05-14 14:19 PDT, Brent Fulgham
no flags
Patch (most is just test case). Updated for iOS (615.23 KB, patch)
2016-05-14 22:56 PDT, Brent Fulgham
no flags
Patch (nearly all of this is just test case) (615.24 KB, patch)
2016-05-15 17:59 PDT, Brent Fulgham
simon.fraser: review+
Brent Fulgham
Comment 1 2016-05-12 11:05:49 PDT
Darin and I debugged the cause of the problem. I need to create a reduced test case we can add to the LayoutTests, but I am posting the patch for discussion since I would like to hear from hyatt or perhaps others to make sure this is the right approach.
Brent Fulgham
Comment 2 2016-05-12 11:06:04 PDT
Brent Fulgham
Comment 3 2016-05-12 11:09:27 PDT
WebKit Commit Bot
Comment 4 2016-05-12 11:10:58 PDT
Attachment 278738 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:9: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Adler
Comment 5 2016-05-12 19:48:29 PDT
Comment on attachment 278738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278738&action=review > Source/WebCore/html/HTMLImageElement.cpp:545 > + addSubresourceURL(urls, document().completeURL(imageSourceURL())); Why is calling document.completeURL(imageSourceURL()) better than calling currentSrc() for this?
Brent Fulgham
Comment 6 2016-05-13 10:04:15 PDT
(In reply to comment #5) > Comment on attachment 278738 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=278738&action=review > > > Source/WebCore/html/HTMLImageElement.cpp:545 > > + addSubresourceURL(urls, document().completeURL(imageSourceURL())); > > Why is calling document.completeURL(imageSourceURL()) better than calling > currentSrc() for this? addSubresourceURLs expects a URL, but currentSrc() returns a string. We could convert the string value to a URL, but since currentSrc() is populated by the URL returned by "document().completeURL(imageSourceURL())" (converted to a string), it just seemed safer to avoid the URL->string->URL conversion.
Darin Adler
Comment 7 2016-05-13 18:55:57 PDT
Comment on attachment 278738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278738&action=review >>> Source/WebCore/html/HTMLImageElement.cpp:545 >>> + addSubresourceURL(urls, document().completeURL(imageSourceURL())); >> >> Why is calling document.completeURL(imageSourceURL()) better than calling currentSrc() for this? > > addSubresourceURLs expects a URL, but currentSrc() returns a string. > > We could convert the string value to a URL, but since currentSrc() is populated by the URL returned by "document().completeURL(imageSourceURL())" (converted to a string), it just seemed safer to avoid the URL->string->URL conversion. The real problem here is that currentSrc() returns a string. Seems kinda crazy that it does.
Brent Fulgham
Comment 8 2016-05-14 14:19:57 PDT
Created attachment 278945 [details] Patch (nearly all of this is test case)
Brent Fulgham
Comment 9 2016-05-14 22:56:15 PDT
Created attachment 278961 [details] Patch (most is just test case). Updated for iOS
Darin Adler
Comment 10 2016-05-15 01:33:30 PDT
Comment on attachment 278961 [details] Patch (most is just test case). Updated for iOS Still not compiling on iOS EWS yet.
Brent Fulgham
Comment 11 2016-05-15 17:59:07 PDT
Created attachment 278986 [details] Patch (nearly all of this is just test case)
Brent Fulgham
Comment 12 2016-05-15 17:59:39 PDT
(In reply to comment #10) > Comment on attachment 278961 [details] > Patch (most is just test case). Updated for iOS > > Still not compiling on iOS EWS yet. Ugh! Forgot to fix WK1 DumpRenderTree!
Simon Fraser (smfr)
Comment 13 2016-05-15 21:27:33 PDT
Comment on attachment 278986 [details] Patch (nearly all of this is just test case) View in context: https://bugs.webkit.org/attachment.cgi?id=278986&action=review > Tools/DumpRenderTree/mac/TestRunnerMac.mm:1249 > + NSPredicate *iPredicate = [NSPredicate predicateWithFormat:WebSubframeArchivesKey]; iPredicate is a pretty gross name. Just 'predicate'?
Brent Fulgham
Comment 14 2016-05-16 10:34:34 PDT
Note You need to log in before you can comment on or make changes to this bug.