Bug 209970 – Image `referrerpolicy` mutations should be considered "relevant mutations"

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Dominic Farolino]

This is a pretty simple addition. Please see https://github.com/whatwg/html/issues/5244 and https://github.com/whatwg/html/pull/5434.

[Rob Buis]

Created attachment 402014 [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

[Rob Buis]

Created attachment 402023 [details]
Patch

[Rob Buis]

Created attachment 402028 [details]
Patch

[youenn fablet]

Comment on attachment 402028 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/relevant-mutations-expected.txt:17
> +PASS crossorigin anonymous to absent, src absent 

I do not see the code used to reload in case cross origin is changed.
Do you know how it works?
It seems this should be roughly the same code path as for referrer policy.

[Rob Buis]

Comment on attachment 402028 [details]
Patch

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

>> LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/relevant-mutations-expected.txt:17
>> +PASS crossorigin anonymous to absent, src absent 
> 
> I do not see the code used to reload in case cross origin is changed.
> Do you know how it works?
> It seems this should be roughly the same code path as for referrer policy.

I was halfway implementing that but there are two problems:
1. it does not fit the Bug title
2. it is tricky. I don't think we have code to compute cross origin attribute state (like for referrer policy). Also it seems we use omit rather than anonymous?
Given that, I thought it is better to not tackle it here, but I can make a follow up bug of course. The other FAILs are not high prio for me (seem picture related).

[Dominic Farolino]

Just a drive-by. If there are any questions about or bugs in the test(s) let me know. Happy to help

[Rob Buis]

(In reply to Dominic Farolino from comment #7)
> Just a drive-by. If there are any questions about or bugs in the test(s) let
> me know. Happy to help

Thnx Dominic, my current status is described in Comment 6. What do you think about the crossorigin attribute part, makes sense to be its own bug? And would the other FAILs be one bug (picture related) or more?

[Dominic Farolino]

You're asking if making Safari "consider crossorigin attribute changes == relevant mutations should be a separate issue from here?

Yeah that sounds fine, as long as it is tracked somewhere. I assume that means you'll be failing some of the `crossorigin` relevant mutations tests until that is fixed then?

[Rob Buis]

(In reply to Dominic Farolino from comment #9)
> You're asking if making Safari "consider crossorigin attribute changes ==
> relevant mutations should be a separate issue from here?

Exactly.

> Yeah that sounds fine, as long as it is tracked somewhere. I assume that
> means you'll be failing some of the `crossorigin` relevant mutations tests
> until that is fixed then?

Unfortunately that would be the case :) For the reasons stated in comment 6 that seems to be the best path forward to me. I can work on fixing crossorigin and other FAILs but they are lower prio for me currently, so I will get to them a bit later, but with bug reports there at least I will not forget.

[youenn fablet]

Oh, I misread the output of the relevant mutation tests, the crossOrigin ones are failing when there is a src.
Sounds good then.

[youenn fablet]

Comment on attachment 402028 [details]
Patch

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

> Source/WebCore/html/HTMLImageElement.cpp:252
> +    HTMLElement::attributeChanged(name, oldValue, newValue, reason);

We should probably call this method even for referrerpolicyAttr

[Rob Buis]

Created attachment 402121 [details]
Patch

[Rob Buis]

Comment on attachment 402028 [details]
Patch

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

>> Source/WebCore/html/HTMLImageElement.cpp:252
>> +    HTMLElement::attributeChanged(name, oldValue, newValue, reason);
> 
> We should probably call this method even for referrerpolicyAttr

Done.

[EWS]

Committed r263167: <https://trac.webkit.org/changeset/263167>

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

[Radar WebKit Bug Importer]

<rdar://problem/64454781>

[Rob Buis]

(In reply to Dominic Farolino from comment #9)
> You're asking if making Safari "consider crossorigin attribute changes ==
> relevant mutations should be a separate issue from here?
> 
> Yeah that sounds fine, as long as it is tracked somewhere. I assume that
> means you'll be failing some of the `crossorigin` relevant mutations tests
> until that is fixed then?

I have opened https://bugs.webkit.org/show_bug.cgi?id=213335 to track this.