Bug 157630 – REGRESSION (r192098): Content missing after copy and paste to Notes App on retina displays

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

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.

[Brent Fulgham]

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]

<rdar://problem/25277577>

[Brent Fulgham]

Created attachment 278738 [details]
Patch

[WebKit Commit Bot]

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 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]

(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 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]

Created attachment 278945 [details]
Patch (nearly all of this is test case)

[Brent Fulgham]

Created attachment 278961 [details]
Patch (most is just test case). Updated for iOS

[Darin Adler]

Comment on attachment 278961 [details]
Patch (most is just test case). Updated for iOS

Still not compiling on iOS EWS yet.

[Brent Fulgham]

Created attachment 278986 [details]
Patch (nearly all of this is just test case)

[Brent Fulgham]

(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 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]

Committed r200945: <http://trac.webkit.org/changeset/200945>