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

Status: VERIFIED FIXED

(Firefox :: Page Info Window, defect)

Description

[Mark]

Attachment: 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.

Comment 1

[Alice0775 White]

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

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

[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.

Comment 3

[Mike Conley (: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.

Comment 4

[Mike Conley (: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.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1255773 - Account for multiple media elements per node for Page Info. r?florian

Review commit: https://reviewboard.mozilla.org/r/44371/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44371/

Comment 6

[Florian Quèze [:florian]]

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")

Comment 7

[Mike Conley (:mconley) (:⚙️)]

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 8

[Mike Conley (:mconley) (:⚙️)]

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 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e884e4e3211

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e884e4e3211

Comment 11

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Mark, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked since this is e10s related.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Alice0775White, could you please help verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I'd like for this to bake in Nightly some more and/or get a verification in Nightly before uplifting to Aurora.

Comment 16

[Alice0775 White]

(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

Comment 17

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f1668f8aebd7

Comment 19

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-beta/rev/c6cbc6079272

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(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.

Comment 21

[Cornel Ionce [:cornel_ionce], Desktop Release QA]

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.