Closed
Bug 1255773
Opened 9 years ago
Closed 9 years ago
Only one image is shown in the Media list for each node with multiple images
Categories
(Firefox :: Page Info Window, defect)
Tracking
()
VERIFIED
FIXED
Firefox 48
People
(Reporter: ulli.luftpumpe, Assigned: mconley)
References
Details
(Keywords: regression)
Attachments
(2 files)
139.42 KB,
application/zip
|
Details | |
58 bytes,
text/x-review-board-request
|
florian
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160210153822
Steps to reproduce:
I tried to find the URI of an image visible on the page by using Page Info -> Media.
Actual results:
It is not in the list.
Expected results:
I expect all image files the page includes in that media list.
If loaded by JavaScript, it should be there too once it was requested, thus the list is updated (refreshed) live when staying open.
If Javascript changes the CSS class of an element that triggers loading of images, those have to be in that list too.
When right clicking on the top most image anywhere in a page I expect Firefox to realize that and show graphic options (save as, info) also for CSS backgrounds. It is quiet common today to include images via CSS rather than classic image elements.
![]() |
||
Updated•9 years ago
|
status-firefox45:
--- → affected
status-firefox46:
--- → affected
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox-esr38:
--- → unaffected
status-firefox-esr45:
--- → affected
Component: Untriaged → Page Info Window
Keywords: regression
Version: 48 Branch → 42 Branch
![]() |
||
Comment 1•9 years ago
|
||
STR
Open testcase
Open Page Info and select Media
Actual Results:
Only 1 image
Expected Results:
There are 2 images
Regression window:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=cf6138f13ae94f0bfa8edf95db1cfa12f17c8b46&tochange=698d20c94746543bcfece4766a440e0d78f90e85
Regressed by: Bug 1175794
Comment 2•9 years ago
|
||
[Tracking Requested - why for this release]:
Regression from 43, not sure if we need to track this.
mconley, can you help me find an owner for this? May be e10s related from the look of the regressing bug.
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #2)
> [Tracking Requested - why for this release]:
>
> Regression from 43, not sure if we need to track this.
> mconley, can you help me find an owner for this? May be e10s related from
> the look of the regressing bug.
I can own this. I'll see what I come up with.
Flags: needinfo?(mconley)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mconley
Assignee | ||
Comment 4•9 years ago
|
||
Looks like when we refactored Page Info for e10s, we lost the ability to add rows for multiple background images on nodes.
I think I know how to fix this.
Flags: needinfo?(jimmyw22)
Summary: Page Info - Media is incomplete → Only one image is shown in the Media list for each node with multiple images
Assignee | ||
Comment 5•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/44371/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44371/
Attachment #8738188 -
Flags: review?(florian)
Comment 6•9 years ago
|
||
Comment on attachment 8738188 [details]
MozReview Request: Bug 1255773 - Account for multiple media elements per node for Page Info. r?florian
https://reviewboard.mozilla.org/r/44371/#review41049
Looks reasonable. It's unfortunate that this isn't covered by tests, but I don't think blocking the regression fix on us writing a test would be the right thing to do.
::: browser/base/content/content.js:1075
(Diff revision 1)
> for (let doc of frameList) {
> let iterator = doc.createTreeWalker(doc, content.NodeFilter.SHOW_ELEMENT);
>
> // Goes through all the elements on the doc. imageViewRows takes only the media elements.
> while (iterator.nextNode()) {
> - let mediaNode = this.getMediaNode(document, strings, iterator.currentNode);
> + let mediaNodes = this.getMediaNodes(document, strings, iterator.currentNode);
I find the name "getMediaNodes" confusing, as there's still only one DOM node there. How about a longer but more explicit name like "getMediaItemsFromNode" or "getMediaItemsForNode"? (and "mediaNodes" -> "mediaItems")
Attachment #8738188 -
Flags: review?(florian) → review+
Assignee | ||
Comment 7•9 years ago
|
||
https://reviewboard.mozilla.org/r/44371/#review41049
> I find the name "getMediaNodes" confusing, as there's still only one DOM node there. How about a longer but more explicit name like "getMediaItemsFromNode" or "getMediaItemsForNode"? (and "mediaNodes" -> "mediaItems")
Yeah, good call. I've switched it to `getMediaItems`.
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8738188 [details]
MozReview Request: Bug 1255773 - Account for multiple media elements per node for Page Info. r?florian
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44371/diff/1-2/
Comment 10•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8738188 [details]
MozReview Request: Bug 1255773 - Account for multiple media elements per node for Page Info. r?florian
Approval Request Comment
[Feature/regressing bug #]:
Regressed by Bug 1175794
[User impact if declined]:
Users attempting to View Page Info for a page that has several images per node (for example, several background images on a DOM node), will only see one of those images listed, per node.
[Describe test coverage new/current, TreeHerder]:
Unfortunately, we have very little test coverage for Page Info. This was manually tested, and has had a night to bake on mozilla-central.
[Risks and why]:
I would say that this is low risk.
[String/UUID change made/needed]:
None.
Attachment #8738188 -
Flags: approval-mozilla-beta?
Attachment #8738188 -
Flags: approval-mozilla-aurora?
Hello Mark, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(ulli.luftpumpe)
Tracked since this is e10s related.
Hello Alice0775White, could you please help verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(alice0775)
I'd like for this to bake in Nightly some more and/or get a verification in Nightly before uplifting to Aurora.
![]() |
||
Comment 16•9 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #14)
> Hello Alice0775White, could you please help verify this issue is fixed as
> expected on a latest Nightly build? Thanks!
I can verify that Nightly48.0a1[1] no longer reproduce the problem,
[1]
https://hg.mozilla.org/mozilla-central/rev/d9b1a9829c8ee2862955043f28183efa07de3d2b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0 ID:20160408030212
Flags: needinfo?(alice0775)
Comment 17•9 years ago
|
||
Comment on attachment 8738188 [details]
MozReview Request: Bug 1255773 - Account for multiple media elements per node for Page Info. r?florian
Fix for pageinfo regression, verified on nightly.
Let's uplift this for aurora and for beta 10.
Attachment #8738188 -
Flags: approval-mozilla-beta?
Attachment #8738188 -
Flags: approval-mozilla-beta+
Attachment #8738188 -
Flags: approval-mozilla-aurora?
Attachment #8738188 -
Flags: approval-mozilla-aurora+
Comment 18•9 years ago
|
||
Comment 19•9 years ago
|
||
Updated•9 years ago
|
Flags: qe-verify+
(In reply to Alice0775 White from comment #16)
> (In reply to Ritu Kothari (:ritu) from comment #14)
> > Hello Alice0775White, could you please help verify this issue is fixed as
> > expected on a latest Nightly build? Thanks!
>
> I can verify that Nightly48.0a1[1] no longer reproduce the problem,
>
> [1]
> https://hg.mozilla.org/mozilla-central/rev/
> d9b1a9829c8ee2862955043f28183efa07de3d2b
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
> ID:20160408030212
Great! Thanks.
Status: RESOLVED → VERIFIED
Comment 21•9 years ago
|
||
Reproduced this issue on Windows 10 x64 using Firefox 45.0.2.
Confirming the fix on Latest 48.0a1 Nightly, Latest 47.0a2 Aurora and Firefox 46 beta 10.
Removing the needinfo since it's no longer required.
Flags: needinfo?(ulli.luftpumpe)
You need to log in
before you can comment on or make changes to this bug.
Description
•