Bug 222801 - Wrong image displayed for <picture> elements created through JavaScript
Summary: Wrong image displayed for <picture> elements created through JavaScript
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: Safari Technology Preview
Hardware: All macOS 11
: P2 Normal
Assignee: Cameron McCormack (:heycam)
URL:
Keywords: InRadar
Depends on: 225044
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-05 05:38 PST by Marvin Hagemeister
Modified: 2021-04-28 01:39 PDT (History)
16 users (show)

See Also:


Attachments
HTML file to reproduce the issue (2.11 KB, text/html)
2021-03-05 05:38 PST, Marvin Hagemeister
no flags Details
Patch (5.91 KB, patch)
2021-04-25 17:29 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch (3.62 KB, patch)
2021-04-26 21:54 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch (7.28 KB, patch)
2021-04-26 22:19 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch (7.28 KB, patch)
2021-04-27 17:29 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch (7.44 KB, patch)
2021-04-28 00:35 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marvin Hagemeister 2021-03-05 05:38:31 PST
Created attachment 422368 [details]
HTML file to reproduce the issue

Picture elements that are created in JavaScript and appended to the DOM don't re-evaluate the image source on resize. Instead the initial image, from when the element was attached to the DOM is shown. Changing any attribute on one of the source elements seems to trigger the browser to re-evaluate the image choice and display the correct image.

Check out the attached HTML file for an isolated reproduction case.
Comment 1 Marvin Hagemeister 2021-03-05 08:36:59 PST
Found another workaround: Simply adding the picture element first, before appending the source-elements, works too.

```js
// Works
container.appendChild(picture);
picture.appendChild(source1);
picture.appendChild(img);
```

vs

```js
// Doesn't work
picture.appendChild(source1);
picture.appendChild(img);
container.appendChild(picture);
```

Which leads me to believe that Safari is very picky about the order of insertion.
Comment 2 Radar WebKit Bug Importer 2021-03-12 05:39:12 PST
<rdar://problem/75359328>
Comment 3 Cameron McCormack (:heycam) 2021-04-06 23:57:22 PDT
Thanks for the reduced test case, Marvin.

Rob, this issue looks to be a regression from https://trac.webkit.org/changeset/263345/webkit.  This is because when doing things in this order:

  * create source element
  * set source element's media attribute
  * create picture element
  * append source element to picture element
  * insert picture element into document

we never end up registering the <img> as a dynamic media query dependent image with the document.  This is because we only get into selectImageSource() in response to the attribute changes, which happen before the <img> has a <picture> parent, and so we don't get to the addDynamicMediaQueryDependentImage() call.

What's the purpose of checking whether a newly inserted <picture> element is in the document here:

https://webkit-search.igalia.com/webkit/rev/2ba963a3b7c21a24310b76fbdfa2d72b0eefbb81/Source/WebCore/html/HTMLImageElement.cpp#387-390

If we always called selectImageSource() here regardless of whether the <picture> is already in the document, that should cause us to correctly track MQ result changes.
Comment 4 Cameron McCormack (:heycam) 2021-04-25 17:29:51 PDT
Created attachment 427014 [details]
Patch
Comment 5 Ryosuke Niwa 2021-04-25 17:52:16 PDT
Comment on attachment 427014 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Ensure dynamically inserted <picture> elements listen for MQ changes.

This is only about media query?

> Source/WebCore/html/HTMLPictureElement.cpp:52
> +    auto insertNotificationRequest = HTMLElement::insertedIntoAncestor(insertionType, parentOfInsertedTree);

No need to restore & return this. HTMLElement/Element::insertedIntoAncestor is guaranteed to return InsertedIntoAncestorResult::Done

> Source/WebCore/html/HTMLPictureElement.cpp:53
> +    sourcesChanged();

This isn't quite right if it's meant to track media query changes.
In that case, what we want to do is to check insertionType.connectedToDocument.

> LayoutTests/fast/picture/picture-mq-insert.html:20
> +let s1 = document.createElement('source');

Please don't use two-letter variable name like s1. Spell out source1.
Also use const?

> LayoutTests/fast/picture/picture-mq-insert.html:25
> +let s2 = document.createElement('source');

Ditto.

> LayoutTests/fast/picture/picture-mq-insert.html:30
> +let img = document.createElement('img');

const?
Comment 6 Cameron McCormack (:heycam) 2021-04-26 21:54:10 PDT
Created attachment 427117 [details]
Patch
Comment 7 Cameron McCormack (:heycam) 2021-04-26 21:56:44 PDT
WPT PR for this change: https://github.com/web-platform-tests/wpt/pull/28708

Safari fails this test currently (since we don't track the MQ change), but passes after this patch.  Firefox fails as it doesn't track MQ changes.  Chrome fails since while it does track MQ changes, it fails on the last assertion, which checks that removing the <img> from the <picture> re-does image source selection.
Comment 8 Cameron McCormack (:heycam) 2021-04-26 22:19:19 PDT
Created attachment 427119 [details]
Patch
Comment 9 Ryosuke Niwa 2021-04-27 16:45:57 PDT
Please update the patch for trunk.
Comment 10 Cameron McCormack (:heycam) 2021-04-27 17:29:00 PDT
Created attachment 427219 [details]
Patch
Comment 11 EWS Watchlist 2021-04-27 17:29:58 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 12 Ryosuke Niwa 2021-04-27 23:46:57 PDT
Comment on attachment 427219 [details]
Patch

r=me assuming WPT PR lands first.
Comment 13 Ryosuke Niwa 2021-04-27 23:47:14 PDT
Note that we need to wait for WPT PR to land first before we can land this.
Comment 14 Cameron McCormack (:heycam) 2021-04-28 00:35:59 PDT
Created attachment 427244 [details]
Patch
Comment 15 EWS 2021-04-28 01:39:09 PDT
Committed r276697 (237111@main): <https://commits.webkit.org/237111@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427244 [details].