Closed
Bug 1307185
Opened 8 years ago
Closed 8 years ago
Images don't load after comment on Github issue
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
VERIFIED
FIXED
mozilla52
People
(Reporter: adamopenweb, Assigned: jdm)
References
()
Details
Attachments
(2 files)
189.48 KB,
image/png
|
Details | |
2.69 KB,
patch
|
edgar
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
STR: 1) Load Firefox nightly 2) Navigate to https://github.com/ 2) Sign in 3) Find an issue to comment on 4) Comment Expected Behavior: All images load after comment is submitted. Actual Behavior: New images to timeline do not appear. Your new comment's avatar for example. Inspector says "Could not load the image." Tested in OSX. Range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=696adef2e21a9fe27f5b4251f00dc0c942a5d4f5&tochange=299a09f24493acfbbe8b9457c36e2eccf318d787
Reporter | ||
Updated•8 years ago
|
URL: https://github.com/
Reporter | ||
Comment 1•8 years ago
|
||
Seems like maybe https://bugzilla.mozilla.org/show_bug.cgi?id=1268182?
Flags: needinfo?(josh)
Assignee | ||
Comment 2•8 years ago
|
||
Yeah, that regression range looks pretty clear to me.
Assignee: nobody → josh
Flags: needinfo?(josh)
Assignee | ||
Updated•8 years ago
|
status-firefox51:
--- → unaffected
tracking-firefox52:
--- → ?
Assignee | ||
Comment 4•8 years ago
|
||
I see the problem, but the solution is not yet clear to me. The images on github are created in a document fragment as part of the AJAX-based HTML updates; this prevents the image loads from starting, but we happily store the URL that we tried to load as the last selected image source. When the image element is appended the actual github document, we try to start the load again and short-circuit because the URL hasn't changed. The complication comes from the fact that there's no indication that the load failed, since we return from LoadImage with NS_OK in the case where the load is blocked.
Assignee | ||
Comment 5•8 years ago
|
||
We can work around this problem by resetting the last selected source if the element changes trees.
Attachment #8797709 -
Flags: review?(echen)
Comment 6•8 years ago
|
||
Comment on attachment 8797709 [details] [diff] [review] Ensure image loads don't short-circuit if the element changed trees since the last load Review of attachment 8797709 [details] [diff] [review]: ----------------------------------------------------------------- This reminds me bug 1264529. It might also help this issue. We can take this fix first and I will work on bug 1264529 during this week. Thank you, Josh.
Attachment #8797709 -
Flags: review?(echen) → review+
Assignee | ||
Comment 7•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d0b44f39d3c
Assignee | ||
Comment 8•8 years ago
|
||
None of the failures on try look related to this change.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d0be8b2713f4 Ensure image loads don't short-circuit if the element changed trees since the last load. r=echen
Keywords: checkin-needed
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d0be8b2713f4
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8797709 [details] [diff] [review] Ensure image loads don't short-circuit if the element changed trees since the last load Approval Request Comment [Feature/regressing bug #]: bug 1268182 [User impact if declined]: Lots of images on github stop loading. [Describe test coverage new/current, TreeHerder]: New test added verifying this case doesn't regress. [Risks and why]: At worst we reload an image unnecessarily. [String/UUID change made/needed]: None. Don't uplift this unless bug 1268182 is uplifted.
Attachment #8797709 -
Flags: approval-mozilla-beta?
Attachment #8797709 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
Hello Adam, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(astevenson)
Comment on attachment 8797709 [details] [diff] [review] Ensure image loads don't short-circuit if the element changed trees since the last load Fixes a recent regression in Fx50, automated tests were updated as well (yay!), Aurora51+, Beta50+
Attachment #8797709 -
Flags: approval-mozilla-beta?
Attachment #8797709 -
Flags: approval-mozilla-beta+
Attachment #8797709 -
Flags: approval-mozilla-aurora?
Attachment #8797709 -
Flags: approval-mozilla-aurora+
Comment 14•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/df69f3a7cacd
Comment 15•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/03e2d6478f09
Reporter | ||
Comment 16•8 years ago
|
||
I can confirm this is fixed in nightly 52.0a1 (2016-10-18). Thanks!
Status: RESOLVED → VERIFIED
Flags: needinfo?(astevenson)
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•