Closed Bug 1318624 Opened 3 years ago Closed 2 years ago

Should not start a load of an image if it's document is not an active document

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(2 files, 3 obsolete files)

We try to start the load of an image which is in a document fragment. This seem not follow the spec, see step 1 of https://html.spec.whatwg.org/multipage/embedded-content.html#updating-the-image-data.

* Test script:
http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4675

* Expected result:
no error event.

* Actual result:
got an unexpected error event.
Attached patch Patch, v1 (obsolete) — Splinter Review
WIP patch, working on the test cases ...
(In reply to Edgar Chen [:edgar] from comment #0)
> We try to start the load of an image which is in a document fragment. This
> seem not follow the spec, see step 1 of
> https://html.spec.whatwg.org/multipage/embedded-content.html#updating-the-
> image-data.
> 
> * Test script:
> http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4675
> 
> * Expected result:
> no error event.
> 
> * Actual result:
> got an unexpected error event.

Test other browsers: Chrome, Safari, and Edge all have no error event fired.
Attachment #8868954 - Attachment is obsolete: true
Comment on attachment 8868953 [details] [diff] [review]
Should not perform a load of a image if it's document is not active document, v2

Review of attachment 8868953 [details] [diff] [review]:
-----------------------------------------------------------------

Originally, I planned to fix this along with the non-responsive load behavior changes in bug 1076583. However, bug 1076583 is blocked since there is still some possible spec issues (in terms of web-compatibility) need to be considered. So It's better to fix this separately here.

In this patch, I add checks for non-responsive mode only, since we already do same check in http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/dom/html/HTMLImageElement.cpp#945 for the responsive mode.

Josh, may I have your review? Thank you.
Attachment #8868953 - Flags: review?(josh)
Comment on attachment 8868959 [details] [diff] [review]
Add wpt for img in non-active document, v2

Review of attachment 8868959 [details] [diff] [review]:
-----------------------------------------------------------------

I have run this test on Chrome, Safari, and Edge. All of them can pass.
Attachment #8868959 - Flags: review?(josh)
Attachment #8868953 - Flags: review?(josh) → review+
Comment on attachment 8868959 [details] [diff] [review]
Add wpt for img in non-active document, v2

Review of attachment 8868959 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/html/semantics/embedded-content/the-img-element/non-active-document.html
@@ +18,5 @@
> +    var d = p.parseFromString('<img>', 'text/html');
> +    var i = d.querySelector('img');
> +    i.onerror = t.unreached_func('got unexpected error event');
> +    i.onload = t.unreached_func('got unexpected load event');
> +    i.src = '/images/green-1x1.png';

We should add a `<img src=/images/green-1x1.png>` in this file so that any loads here would be cached.
Attachment #8868959 - Flags: review?(josh) → review+
Addressed review comment #9. Thanks for the review, Josh.
Attachment #8868959 - Attachment is obsolete: true
Attachment #8869302 - Flags: review+
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c9166bc787d
Should not perform a load of a image if it's document is not active document; r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/55ccbc9a9bfc
Add wpt for img in non-active document; r=jdm
https://hg.mozilla.org/mozilla-central/rev/9c9166bc787d
https://hg.mozilla.org/mozilla-central/rev/55ccbc9a9bfc
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1345588
Depends on: 1409992
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.