Closed
Bug 1317901
Opened 8 years ago
Closed 8 years ago
Cloned images from <template> do not load.
Categories
(Core :: DOM: Core & HTML, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla53
People
(Reporter: TailsTheKitsune, Assigned: edgar)
References
Details
(Keywords: dev-doc-complete, regression, site-compat)
Attachments
(4 files, 1 obsolete file)
1.09 KB,
text/html
|
Details | |
3.75 KB,
patch
|
jdm
:
review+
|
Details | Diff | Splinter Review |
3.82 KB,
patch
|
jcristau
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
3.75 KB,
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161104212021
Steps to reproduce:
1. Store an <img /> inside <template>
2. Clone the <img /> from the <template>'s content property.
3. Append it somewhere in the document
Actual results:
The image will not load. Its alt text will be displayed instead. Per the DOM Inspector, the src is preserved but shows "Could not load the image" when hovering over it.
Expected results:
The image should be shown. There were no problems in Firefox 49. Bug has shown up in Firefox 50.0 and is present in Nightly.
Comment 1•8 years ago
|
||
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=696adef2e21a9fe27f5b4251f00dc0c942a5d4f5&tochange=299a09f24493acfbbe8b9457c36e2eccf318d787
Suspect: c4b39cdfb64 Josh Matthews — Bug 1268182 - Allow image loads to short-circuit after selecting a source if the new source URL matches the previous one URL. r=echen
Blocks: 1268182
Status: UNCONFIRMED → NEW
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox-esr45:
--- → unaffected
Component: Untriaged → DOM
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Updated•8 years ago
|
Flags: needinfo?(josh)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → echen
Flags: needinfo?(echen)
Updated•8 years ago
|
Keywords: dev-doc-needed,
site-compat
Assignee | ||
Comment 3•8 years ago
|
||
The problem is similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1307185#c4, the image element is created in document fragment and then adopted into active document. But UnbindFromTree() doesn't be triggered to clear mLastSelectedSource since the element has no parent.
And I see another problem, 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. Filed bug 1318624.
Assignee | ||
Comment 4•8 years ago
|
||
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Edgar Chen [:edgar][:echen] from comment #5)
> Created attachment 8812134 [details] [diff] [review]
> Patch, v2
>
> Fix type.
s/type/typo
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8812134 [details] [diff] [review]
Patch, v2
Hi Josh, could you help to review this patch? Thank you.
Flags: needinfo?(josh)
Attachment #8812134 -
Flags: review?(josh)
Assignee | ||
Comment 8•8 years ago
|
||
Comment 9•8 years ago
|
||
Comment on attachment 8812134 [details] [diff] [review]
Patch, v2
Review of attachment 8812134 [details] [diff] [review]:
-----------------------------------------------------------------
If the test fails without the code change, this looks fine to me.
::: dom/html/reftests/image-load-shortcircuit-2.html
@@ +3,5 @@
> +<template id="template">
> +<img src="pass.png" alt="Alt Text" />
> +</template>
> +<script>
> + document.body.appendChild(document.getElementById('template').content.children[0].cloneNode(1));
Are we sure that this test does not pass with these changes? I'm surprised that a cloned node has this problem.
Attachment #8812134 -
Flags: review?(josh) → review+
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Josh Matthews [:jdm] from comment #9)
> Comment on attachment 8812134 [details] [diff] [review]
> Patch, v2
>
> Review of attachment 8812134 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> If the test fails without the code change, this looks fine to me.
>
> ::: dom/html/reftests/image-load-shortcircuit-2.html
> @@ +3,5 @@
> > +<template id="template">
> > +<img src="pass.png" alt="Alt Text" />
> > +</template>
> > +<script>
> > + document.body.appendChild(document.getElementById('template').content.children[0].cloneNode(1));
>
> Are we sure that this test does not pass with these changes? I'm surprised
> that a cloned node has this problem.
Yes, I have verified it. The test doesn't pass without the code change.
The problem is,
The cloned image starts a load even if it's document is not active one [1] and fail to load, but store URL as last selected source. And since it has no parent, appending/adopting into active document won't trigger UnbindFromTree() to clear mLastSelectedSource. Then we try to start the load again and short-circuit because the URL hasn't changed.
[1] http://searchfox.org/mozilla-central/source/dom/html/HTMLImageElement.cpp#829
Comment 11•8 years ago
|
||
Ritu, this may be a candidate for the 50.1.0 release. Loic suggested it in bug 1268182 since it affects some corporate sites. This may also fix bug 1318889 and the other dependencies of bug 1268182.
tracking-firefox50:
--- → ?
Flags: needinfo?(rkothari)
Comment 12•8 years ago
|
||
Once you all have landed a possible fix let's verify it and uplift at least to beta 51.
Flags: needinfo?(echen)
Comment 13•8 years ago
|
||
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab60afa100d3
Ensure image loads don't short-circuit if element's adopting steps are run; r=jdm
Comment 14•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Comment 16•8 years ago
|
||
Flags: needinfo?(echen)
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8813492 [details] [diff] [review]
[beta] Patch, v2, r=jdm
Approval Request Comment
[Feature/regressing bug #]:
bug 1268182
[User impact if declined]:
Lots of images on some corporate sites stop loading.
[Describe test coverage new/current, TreeHerder]:
New test added verifying this case.
[Risks and why]:
At worst we reload an image unnecessarily.
[String/UUID change made/needed]:
None.
Attachment #8813492 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8813492 [details] [diff] [review]
[beta] Patch, v2, r=jdm
Approval Request Comment
[Feature/regressing bug #]:
bug 1268182.
[User impact if declined]:
Lots of images on some corporate sites stop loading.
[Describe test coverage new/current, TreeHerder]:
New test added verifying this case.
[Risks and why]:
At worst we reload an image unnecessarily.
[String/UUID change made/needed]:
None.
Attachment #8813492 -
Flags: approval-mozilla-beta?
Updated•8 years ago
|
Flags: qe-verify+
Comment 19•8 years ago
|
||
Hi Andrei,
Can someone from QE help verify the fix in latest nightly?
Flags: needinfo?(andrei.vaida)
Comment 21•8 years ago
|
||
Hi Brindusa,
Can you help verify if the patch fix bug 1318889, bug 1319046, and this bug on latest nightly?
Flags: needinfo?(andrei.vaida) → needinfo?(brindusa.tot)
Comment 22•8 years ago
|
||
Hi,
I'm the one who created https://bugzilla.mozilla.org/show_bug.cgi?id=1319046 and I've tried the latest nightly build for WIN 10 (firefox 53) and can verify that the bug is fixed.
Reporter | ||
Comment 23•8 years ago
|
||
Creator of this bug here, confirming fixed.
Comment 24•8 years ago
|
||
Comment on attachment 8813492 [details] [diff] [review]
[beta] Patch, v2, r=jdm
fix loading cloned images in template, take in aurora52
Attachment #8813492 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 25•8 years ago
|
||
Comment on attachment 8813492 [details] [diff] [review]
[beta] Patch, v2, r=jdm
Fix a regression and was verified. Beta51+. Should be in 51 beta 3.
Attachment #8813492 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•8 years ago
|
Flags: needinfo?(brindusa.tot)
Comment 26•8 years ago
|
||
Hi,
I tested this bug and the bugs from comment 21 with FF Nightly 53.0a1 on Mac OS X 10.12, Windows 10 x64 and Ubuntu 16.04 x64 and here are the results:
bug 1319046 can't be verified because the test case from the bug doesn't work on my end but I see in comment 22 that the reporter of that bug confirmed the fix.
I can confirm the fix on bug 1217901 and bug 1318889, everything works as expected.
Status: RESOLVED → VERIFIED
Comment 27•8 years ago
|
||
failed to apply to aurora
named 1317901 -> bug_1317901_aurora_v2.patch
applying bug_1317901_aurora_v2.patch
patching file dom/html/HTMLImageElement.h
Hunk #1 FAILED at 87
1 out of 1 hunks FAILED -- saving rejects to file dom/html/HTMLImageElement.h.rej
patching file dom/html/reftests/reftest.list
Hunk #1 succeeded at 32 with fuzz 2 (offset 0 lines).
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug_1317901_aurora_v2.patch
Flags: needinfo?(echen)
Assignee | ||
Comment 28•8 years ago
|
||
Comment on attachment 8813492 [details] [diff] [review]
[beta] Patch, v2, r=jdm
This patch can be applied to beta.
Attachment #8813492 -
Attachment description: [aurora/beta] Patch, v2, r=jdm → [beta] Patch, v2, r=jdm
Attachment #8813492 -
Attachment filename: bug_1317901_aurora_v2.patch → bug_1317901_beta_v2.patch
Comment 30•8 years ago
|
||
bugherder uplift |
Comment 31•8 years ago
|
||
thanks edgar, landed this in https://hg.mozilla.org/releases/mozilla-beta/rev/a2ccc1a66d96a413213b2433d3348c4a6884038e
Flags: needinfo?(cbook)
Hello XFox, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(rkothari) → needinfo?(TailsTheKitsune)
Reporter | ||
Comment 33•8 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #32)
> Hello XFox, could you please verify this issue is fixed as expected on a
> latest Nightly build? Thanks!
Confirmed working in today's Nightly.
Comment 34•8 years ago
|
||
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/img-in-template-will-not-be-loaded-when-added-to-document/
Keywords: dev-doc-needed → dev-doc-complete
Comment 36•8 years ago
|
||
Verified fixed FX 52.0a2 (2016-12-19), 51b9 on Win 7.
Updated•8 years ago
|
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•