If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 18

Status

()

Firefox
Menus
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: sankha)

Tracking

16 Branch
Firefox 18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox16 affected)

Details

(Whiteboard: [good first bug][mentor=jaws][lang=js])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

5 years ago
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"
(Reporter)

Comment 1

5 years ago
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
(Reporter)

Comment 2

5 years ago
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
status-firefox16: --- → affected
OS: Linux → All
Hardware: x86_64 → All
Version: Trunk → 16 Branch

Updated

5 years ago
Flags: in-testsuite?
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.
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">.
Whiteboard: [good first bug][mentor=jaws][lang=js]
(Assignee)

Comment 5

5 years ago
Can you assign me the bug? I would like to work on it!
(Reporter)

Comment 6

5 years ago
Assigned -- thanks!
Assignee: nobody → sankha93
Status: NEW → ASSIGNED
(Assignee)

Comment 7

5 years ago
Created attachment 646625 [details] [diff] [review]
Email Image access key changed to "g"
Attachment #646625 - Flags: review?(jaws)
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.
Attachment #646625 - Flags: review?(jaws) → feedback+
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?
Attachment #646625 - Flags: feedback?(dolske)
(Reporter)

Comment 10

5 years ago
(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.
(Reporter)

Comment 11

5 years ago
(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/
(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?
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?
"m" is used for Mute in audio and video.
(Assignee)

Comment 15

5 years ago
(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?
(Assignee)

Comment 16

5 years ago
Created attachment 647273 [details] [diff] [review]
Patch updated with tests for linked video and audio
Attachment #646625 - Attachment is obsolete: true
Attachment #646625 - Flags: feedback?(dolske)
Attachment #647273 - Flags: review?(jaws)
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.
Attachment #647273 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 18

5 years ago
Created attachment 648009 [details] [diff] [review]
Patch 3

Updated the patch according to your feedback.
Attachment #647273 - Attachment is obsolete: true
Attachment #648009 - Flags: review?(jaws)
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 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).
Attachment #648009 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 21

5 years ago
Created attachment 654212 [details] [diff] [review]
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?
Attachment #648009 - Attachment is obsolete: true
Attachment #654212 - Flags: review?(jaws)
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.
Attachment #654212 - Flags: review?(jaws) → review-
(Assignee)

Comment 23

5 years ago
Created attachment 654534 [details] [diff] [review]
Updated Patch
Attachment #654212 - Attachment is obsolete: true
Attachment #654534 - Flags: review?(jaws)
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 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.
Attachment #654534 - Flags: review?(jaws) → review-
(Assignee)

Comment 26

5 years ago
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.
Since we are removing those test cases we can undo the change for the context-video-showstats access key.
(Assignee)

Comment 28

5 years ago
Created attachment 661526 [details] [diff] [review]
Patch with audio video links removed
Attachment #654534 - Attachment is obsolete: true
Attachment #661526 - Flags: review?(jaws)
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.
Attachment #661526 - Flags: review?(jaws) → review+
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
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/15f6aaf8ed46
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18

Updated

5 years ago
Depends on: 796928
You need to log in before you can comment on or make changes to this bug.