Closed Bug 1255773 Opened 4 years ago Closed 4 years ago

Only one image is shown in the Media list for each node with multiple images

Categories

(Firefox :: Page Info Window, defect)

42 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox45 --- affected
firefox46 - verified
firefox47 blocking verified
firefox48 --- verified
firefox-esr38 --- unaffected
firefox-esr45 --- affected

People

(Reporter: ulli.luftpumpe, Assigned: mconley)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached file testhtml.zip
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.
Version: 44 Branch → 48 Branch
Component: Untriaged → Page Info Window
Keywords: regression
Version: 48 Branch → 42 Branch
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
Blocks: 1175794
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jimmyw22)
[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.
Flags: needinfo?(mconley)
(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: nobody → mconley
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
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+
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`.
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/
https://hg.mozilla.org/mozilla-central/rev/9e884e4e3211
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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)
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.
(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 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+
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
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.
You need to log in before you can comment on or make changes to this bug.