1218144 - Context menu entry "View image info" should select the image automatically

Status: RESOLVED WORKSFORME

(Firefox :: Page Info Window, defect)

Description

[08xjcec48]

Attachment: "View image info" bug

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151022152545

Steps to reproduce:

1. Open the direct link to an image: https://upload.wikimedia.org/wikipedia/commons/5/55/Firefox_40_on_Windows_10.png

2. Right-click on it

3. Pick the "View image info" option


Actual results:

Firefox now shows another address (chrome://global/skin/media/imagedoc-darknoise.png) besides the opened image, and I need to click on the proper address to finally see the image info.


Expected results:

"View image info" should open the info to that image alone, and not display "chrome://global/skin/media/imagedoc-darknoise.png".

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Listing the background image there is likely correct (think about a tab with content from within an extension), but at least the image in the foreground should be selected.

Comment 2

[Gingerbread Man]

Push log:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2ee9895e032c&tochange=62469b20ec84

Suspect: bug 1040947

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

What happened here:

The information of the image on which the context menu got used always uses the image element's title attribute as imageText, or if it doesn't find it the alt attribute:
https://dxr.mozilla.org/mozilla-central/rev/16effd5b21ab03629feca04b5b83911bb757394c/browser/base/content/content.js#1039-1047

When the information for all the images (which will be used by the Media tab of the Page Info window) gets collected, imageText only gets set if it's not a background image (doesn't apply for View Image Info) and neither an SVGImageElement nor an ImageDocument:
https://dxr.mozilla.org/mozilla-central/rev/16effd5b21ab03629feca04b5b83911bb757394c/browser/base/content/content.js#1252-1263

But for the View Image page, it's an ImageDocument, so matching in the Page Info window fails:
https://dxr.mozilla.org/mozilla-central/rev/16effd5b21ab03629feca04b5b83911bb757394c/browser/base/content/pageinfo/pageInfo.js#1069

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Bug 1218144 - also select image in Page Info window when opened from View Image page.

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

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8839247 [details]
Bug 1218144 - also select image in Page Info window when opened from View Image page.

https://reviewboard.mozilla.org/r/113940/#review117924

Sorry for the delay here; I was on PTO most of last week.

::: browser/base/content/content.js:1043
(Diff revision 1)
>  
>    getImageInfo(imageElement) {
>      let imageInfo = null;
>      if (imageElement) {
> +      let imageText;
> +      if (!(imageElement instanceof content.SVGImageElement) &&

The fix looks reasonable, but I think we need a short comment here to explain why this test matters, or it's likely someone may regress this in the future.

Also, is there any chance you could add test coverage for this? http://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_bug517902.js can be used as inspiration (although we'll need to tweak this test a bit in bug 1326910).

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8839247 [details]
Bug 1218144 - also select image in Page Info window when opened from View Image page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113940/diff/1-2/

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This might still be more of a feedback request. The passes here but if it would be great if you can advice on two issues:

1) For the first context menu, I have to call |hidePopup()| - awaitPopupHidden doesn't work like for the second one. How should this be done?

2) How does one handle the Page Info window opening and loading async? Services.ww.registerNotification works, but a yield BrowserTestUtils(aSubject, "load") got ignored. It loads about:blank first, so one has to wait for that xul one.

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8839247 [details]
Bug 1218144 - also select image in Page Info window when opened from View Image page.

https://reviewboard.mozilla.org/r/113940/#review119600

Thanks for adding the test!

Does the SVG case not matter enough to be covered too?

::: browser/base/content/test/pageinfo/browser_viewImageInfo_from_viewImagePage.js:10
(Diff revision 2)
> +  let pageSource = encodeURI("data:text/html," +
> +    "<style type='text/css'>%23test-image,%23not-test-image {background-image: url('about:logo?c');}</style>" +
> +    "<img src='about:logo?b' height=300 width=350 alt=2 id='not-test-image'>" +
> +    "<img src='about:logo?b' height=300 width=350 alt=2>" +
> +    "<img src='about:logo?a' height=200 width=250>" +
> +    "<img id='showContextMenu' src='about:logo?b' height=200 width=250 alt=1>" +

I would make this url unique, so that the test really verifies the right image has been selected in the list. Eg. about:logo?d or about:logo?context

Comment 9

[Florian Quèze [:florian]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #7)

Ah, I didn't see this comment before looking at the patch.

> This might still be more of a feedback request. The passes here but if it
> would be great if you can advice on two issues:
> 
> 1) For the first context menu, I have to call |hidePopup()| -
> awaitPopupHidden doesn't work like for the second one. How should this be
> done?

I was surprised by this hidePopup in the code, but if it works reliably, that's good enough for an r+ IMHO.

I would have tried a synthesizeMouseAtCenter call instead of the .click().

> 2) How does one handle the Page Info window opening and loading async?
> Services.ww.registerNotification works, but a yield
> BrowserTestUtils(aSubject, "load") got ignored. It loads about:blank first,
> so one has to wait for that xul one.

Other tests seem to use the page-info-dialog-loaded notification: http://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_pageInfo.js#15 (btw, this test seems a good candidate to be moved to the new folder you created).

Comment 10

[jscher2000]

"View Image Info" was removed from the context menu in Fx87 by bug 1690029. So I think this request is obsolete now, unless there is a change of course.

Comment 11

[08xjcec48]

This problem also happens if you open the Page info / More information dialog from the main menu or the address bar padlock and then click on the Media tab.