Created attachment 422368 [details]
HTML file to reproduce the issue
Check out the attached HTML file for an isolated reproduction case.
Found another workaround: Simply adding the picture element first, before appending the source-elements, works too.
// Doesn't work
Which leads me to believe that Safari is very picky about the order of insertion.
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:
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.
Created attachment 427014 [details]
Comment on attachment 427014 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=427014&action=review
> + Ensure dynamically inserted <picture> elements listen for MQ changes.
This is only about media query?
> + auto insertNotificationRequest = HTMLElement::insertedIntoAncestor(insertionType, parentOfInsertedTree);
No need to restore & return this. HTMLElement/Element::insertedIntoAncestor is guaranteed to return InsertedIntoAncestorResult::Done
> + 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.
> +let s1 = document.createElement('source');
Please don't use two-letter variable name like s1. Spell out source1.
Also use const?
> +let s2 = document.createElement('source');
> +let img = document.createElement('img');
Created attachment 427117 [details]
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.
Created attachment 427119 [details]
Please update the patch for trunk.
Created attachment 427219 [details]
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 on attachment 427219 [details]
r=me assuming WPT PR lands first.
Note that we need to wait for WPT PR to land first before we can land this.
Created attachment 427244 [details]
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].