Bug 222801 – Wrong image displayed for <picture> elements created through JavaScript

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Marvin Hagemeister]

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.

[Marvin Hagemeister]

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.

[Radar WebKit Bug Importer]

<rdar://problem/75359328>

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

Created attachment 427014 [details]
Patch

[Ryosuke Niwa]

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?

[Cameron McCormack (:heycam)]

Created attachment 427117 [details]
Patch

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

Created attachment 427119 [details]
Patch

[Ryosuke Niwa]

Please update the patch for trunk.

[Cameron McCormack (:heycam)]

Created attachment 427219 [details]
Patch

[EWS Watchlist]

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

[Ryosuke Niwa]

Comment on attachment 427219 [details]
Patch

r=me assuming WPT PR lands first.

[Ryosuke Niwa]

Note that we need to wait for WPT PR to land first before we can land this.

[Cameron McCormack (:heycam)]

Created attachment 427244 [details]
Patch

[EWS]

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