776293 - Right-clicking a hyperlinked image and pressing "a" no longer copies the hyperlink URL. (Email ImAge and Copy Link LocAtion have duplicate access key)

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Visit http://www.nomachine.com/select-package.php?os=linux&id=1

 2. Right-click any of the downarrow-image download links
    (e.g. to the right of "NX Free Edition for Linux RPM i386")

 3. Press "a" on your keyboard.

EXPECTED RESULTS: The link target is copied to your clipboard.

ACTUAL RESULTS: "Copy Link Location" is highlighted, but not selected.


NOTES:
"Copy Link Location" used to be the (only) contextmenu item with accesskey "a" in this situation, so I used to get EXPECTED RESULTS, but I now get ACTUAL RESULTS because Bug 770419 introduced an additional contextmenu entry with the same accessKey, "Email Image"

Comment 1

[Daniel Holbert [:dholbert]]

Encountered this in my nightly build, which is from a couple days ago:
  Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0
  Built from http://hg.mozilla.org/mozilla-central/rev/3a05d298599e

Comment 2

[Daniel Holbert [:dholbert]]

Dolske points out in IRC that we have a test that checks for contextMenu items with duplicate access-keys, but it missed this one because it doesn't test a hyperlinked image.

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/test_contextmenu.html?force=1#119

Comment 3

[Justin Dolske [:Dolske]]

I highly suspect we had/have other cases our conflicting accesskeys in our content menus. There are a lot of possible combinations. (Not to mention whatever localizers might do). I suspect it's unavoidable in the end, but it's also worth looking at if we have enough test coverage of common cases.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

To fix this bug, update the test located at /browser/base/content/test/test_contextmenu.html to have a linked image (<a href="#"><img src="...'></a>) and then to test the context menu for that element group.

To update the accesskey, go to /browser/locales/en-US/chrome/browser/browser.dtd and look for <!ENTITY emailImageCmd.accesskey "a">.

Comment 5

[Sankha Narayan Guria [:sankha]]

Can you assign me the bug? I would like to work on it!

Comment 6

[Daniel Holbert [:dholbert]]

Assigned -- thanks!

Comment 7

[Sankha Narayan Guria [:sankha]]

Attachment: Email Image access key changed to "g"

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 646625 [details] [diff] [review]
Email Image access key changed to "g"

Review of attachment 646625 [details] [diff] [review]:
-----------------------------------------------------------------

Can you add the test at the end so the numbers of all the other tests don't need to be updated?

This looks to fix it for images, but doesn't fix it for linked video and audio (but I would bet that is a 0.00000002% case). See 
> data:text/html,<a href="http://www.mozilla.org"><video src="a" width=400 height=400 controls /></a>
for an example. Although if we're doing it for images, we might as well be consistent with our other media-related ones.

The biggest thing that I'm weary about is our frequency of changing the access key and annoying users. This access key is used for non-linked media as well, so users who had to learn a new access key for Firefox 16 will now have to learn another new access key for Firefox 17.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 646625 [details] [diff] [review]
Email Image access key changed to "g"

Dolske, what do you think about us changing the access key again? Is it worth it considering this would mean three straight releases of different access keys?

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Jared Wein [:jaws] from comment #8)
> This access key is used for non-linked media
> as well, so users who had to learn a new access key for Firefox 16 will now
> have to learn another new access key for Firefox 17.

Can't we backport the patch (the version that ends up landing) to aurora, where Firefox 16 currently lives?  That's what I was hoping... that way, beta/release users would never have to be confused by this access-key-change (for copy link location) in the first place.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> beta/release users would never have to be confused by this access-key-change
> (for copy link location) in the first place.

er s/change/clash/

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Can't we backport the patch (the version that ends up landing) to aurora,
> where Firefox 16 currently lives?

Wouldn't that involve a l10n change?

Comment 13

[Justin Dolske [:Dolske]]

Hmm, I'm not too concerned about the churn. We could probably uplift it, since this shouldn't count as a string-change. What were the previous values?

Does seem like we should use a consistent letter. "m" perhaps?

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

"m" is used for Mute in audio and video.

Comment 15

[Sankha Narayan Guria [:sankha]]

(In reply to Jared Wein [:jaws] from comment #8)

> This looks to fix it for images, but doesn't fix it for linked video and
> audio (but I would bet that is a 0.00000002% case). See 
> > data:text/html,<a href="http://www.mozilla.org"><video src="a" width=400 height=400 controls /></a>
> for an example. Although if we're doing it for images, we might as well be
> consistent with our other media-related ones.

In the already existing tests for audio and video, there are separate tests for both valid and invalid sources. To check for audio and video links, should I write the test so that it checks for both valid and invalid video source files?

Comment 16

[Sankha Narayan Guria [:sankha]]

Attachment: Patch updated with tests for linked video and audio

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 647273 [details] [diff] [review]
Patch updated with tests for linked video and audio

Review of attachment 647273 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/test_contextmenu.html
@@ +303,1 @@
>      case 4:

I don't think we'll want to change case 3 to use an imageLink instead of a mailto. Can you change this back to |openContextMenuFor(mailto);| ?

@@ +763,5 @@
>          closeContextMenu();
>  
>          subwindow.close();
>          SimpleTest.finish();
>          return;

Can you move these three lines to the end of case 28, and then change the end of case 25 to call |openContextMenuFor(imageLink);| ? This will allow the cases to be executed in-order and will make it easier to follow the test.

Comment 18

[Sankha Narayan Guria [:sankha]]

Attachment: Patch 3

Updated the patch according to your feedback.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

I pushed the patch to tryserver to see if the tests pass. I had trouble getting them to pass on my local machine but maybe it's just me:
https://tbpl.mozilla.org/?tree=Try&rev=a5e6231fd144

Matt N asked if we could use "E" as the access key and I think it will work.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 648009 [details] [diff] [review]
Patch 3

Review of attachment 648009 [details] [diff] [review]:
-----------------------------------------------------------------

Please make openWindow use a larger height so as to fit all of the elements in the window. I found height=800 to work in my testing. With the smaller window I noticed the test hanging locally.

When running the test, I also got:
> TEST-UNEXPECTED-FAIL | unknown test url | menuitem context-video-showstats has same accesskey as context-openlinkintab

Since this test will add these new elements to subtst_contextmenu.html, we should also fix the access key for context-video-showstats (since it is the lesser used and newer one).

::: browser/base/content/test/subtst_contextmenu.html
@@ +10,5 @@
>  <a id="test-link" href="http://mozilla.com">Click the monkey!</a>
> +<a id="test-image-link" href="#"><img src="ctxmenu-image.png"></a>
> +<a id="test-video-link" href="#"><video src="video.ogg" width="100" height="100" style="background-color: green"></video></a>
> +<a id="test-audio-link" href="#"><video src="audio.ogg" width="100" height="100" style="background-color: red"></video></a>
> +<a id="test-dummyvideo-link" href="#"><video controls src="bogus.duh" width="100" height="100" style="background-color: orange"></video></a>

Please move these new elements to the bottom of the document.

::: browser/base/content/test/test_contextmenu.html
@@ +760,5 @@
>                              "context-viewpartialsource-selection", true
>                             ].concat(inspectItems));
>          }
>          closeContextMenu();
> +        openContextMenuFor(imagelink)

Test 25 expects a text selection to be present, and the following tests do not expect a selection. Please clear the selection before calling openContextMenuFor(imagelink);

@@ +822,5 @@
> +                          "context-copylink",      true,
> +                          "---",                   null,
> +                          "context-media-play",         true,
> +                          "context-media-mute",         true,
> +                          "context-media-showcontrols", true,

Please add the "controls" attribute to the <audio> element (or else the element will be invisible), and then change this to "context-media-hidecontrols"

@@ +833,5 @@
> +        closeContextMenu();
> +        openContextMenuFor(dummyvideolink); // Invoke context menu for next test.
> +        break;
> +        
> +    case 28:

case 29:

@@ +849,5 @@
> +                          "context-video-showstats",    false,
> +                          "context-video-fullscreen",   false,
> +                          "---",                        null,
> +                          "context-viewvideo",          false,
> +                          "context-copyvideourl",       false,

context-viewvideo, context-copyvideourl, context-savevideo, and context-sendvideo are enabled if the resource has a src specified (it isn't checked for a valid response).

Comment 21

[Sankha Narayan Guria [:sankha]]

Attachment: Patch 5

Sorry Jared for taking such a long time in updating this patch.

I have changed the video-showstats hotkey to "h". Is that okay?

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 654212 [details] [diff] [review]
Patch 5

Review of attachment 654212 [details] [diff] [review]:
-----------------------------------------------------------------

Very close, thanks for the updated patch Sankha :)

::: browser/base/content/test/subtst_contextmenu.html
@@ +62,5 @@
>  </div>
>  <div id="test-select-text">Lorem ipsum dolor sit amet, consectetuer adipiscing elit.</div>
>  <div id="test-select-text-link">http://mozilla.com</div>
> +<a id="test-image-link" href="#"><img src="ctxmenu-image.png"></a>
> +<a id="test-video-link" href="#"><video src="video.ogg" width="100" height="100" style="background-color: green"></video></a>

Please add the "controls" attribute to the video link.

@@ +63,5 @@
>  <div id="test-select-text">Lorem ipsum dolor sit amet, consectetuer adipiscing elit.</div>
>  <div id="test-select-text-link">http://mozilla.com</div>
> +<a id="test-image-link" href="#"><img src="ctxmenu-image.png"></a>
> +<a id="test-video-link" href="#"><video src="video.ogg" width="100" height="100" style="background-color: green"></video></a>
> +<a id="test-audio-link" href="#"><video controls src="audio.ogg" width="100" height="100" style="background-color: red"></video></a>

The audio link should use an <audio> tag.

::: browser/base/content/test/test_contextmenu.html
@@ +886,1 @@
>      iframe, video_in_iframe, image_in_iframe, textarea, contenteditable,

nit: please keep the width at < 80 characters per line to be consistent with the rest of this file.

Comment 23

[Sankha Narayan Guria [:sankha]]

Attachment: Updated Patch

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 654534 [details] [diff] [review]
Updated Patch

Review of attachment 654534 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, I've sent this to the tryserver to make sure that the tests pass.

https://tbpl.mozilla.org/?tree=Try&rev=ef5e00213e97

The test results should be ready in a few hours. I'll update the review status once the tests are finished running.

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 654534 [details] [diff] [review]
Updated Patch

Review of attachment 654534 [details] [diff] [review]:
-----------------------------------------------------------------

The tests came back orange due to the following failures:
1774 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | menu frame has same accesskey as context-video-showstats
2914 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | menuitem context-sendvideo has same accesskey as context-copylink
2997 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | menuitem context-sendaudio has same accesskey as context-copylink
3084 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | menuitem context-sendvideo has same accesskey as context-copylink

I'm not sure what we should do here. It's going to be near impossible to find meaningful unique access keys. It is OK if there are duplicate access keys for some of the more rare occasions.

I think we can remove the <a><video /></a> and <a><audio /></a> test cases from test_contextmenu.html since their likelihood on the internet is quite low and to get the test passing with these cases is not worth the effort.

Comment 26

[Sankha Narayan Guria [:sankha]]

Ok. I will make the necessary changes and remove the test cases for <a><video /></a> and <a><audio /></a>. But what do I set the access key for context-video-showstats? Setting "h" is giving a fail in the tests as well.

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Since we are removing those test cases we can undo the change for the context-video-showstats access key.

Comment 28

[Sankha Narayan Guria [:sankha]]

Attachment: Patch with audio video links removed

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 661526 [details] [diff] [review]
Patch with audio video links removed

Review of attachment 661526 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for sticking with this Sankha!

::: browser/base/content/test/test_contextmenu.html
@@ +763,5 @@
> +        subwindow.getSelection().removeAllRanges();
> +        
> +        openContextMenuFor(imagelink)
> +        break;
> +        

Some whitespace got added here but I'll remove it when checking this in.

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

I pushed this patch to mozilla-inbound. It should appear on mozilla-central within a day or two and then show up on the Nightly builds a day after the merge to central.

https://hg.mozilla.org/integration/mozilla-inbound/rev/15f6aaf8ed46

Comment 31

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/15f6aaf8ed46