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
|
||
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 |
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 |
Comment 15•8 years ago
|
||
bugherder uplift |
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•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
•