Closed Bug 1297738 Opened 3 years ago Closed 3 years ago

removing access key underlines in the drop down arrow menu

Categories

(Core :: DOM: Security, defect, P3)

51 Branch
x86_64
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox53 --- verified

People

(Reporter: kjozwiak, Assigned: jkt)

References

(Blocks 1 open bug)

Details

(Whiteboard: [userContextId][domsecurity-backlog])

Attachments

(3 files)

Attached image ubuntuIssue.png
Under the dropdown arrow, the first letter of each container is underlined which usually means that there's an access key that can be used to access this part of the feature. However, I don't think there's a way to access this part of the UI with access keys.

We should either add a keyboard shortcut or remove the underlines. I'm leaning towards removing the underlines.

This only affects Windows/Linux.
Attached image windowsIssue.png
Priority: -- → P2
I can hover on the new containers menu item or use arrows to open that part of the menu; once it is expanded the letters work as expected.

As nothing else does in this menu then maybe it is right. Will check the longpress for the same issue too.
Assignee: nobody → jkt
(In reply to Jonathan Kingston [:jkt] from comment #2)
> I can hover on the new containers menu item or use arrows to open that part
> of the menu; once it is expanded the letters work as expected.
> 

Maybe this isn't a bug then.
Priority: P2 → P3
Adding some context based on the conversation we had a few weeks ago in the containers meeting.

Whenever I see an item under a file menu underlined, I always assume there's a keyboard shortcut that will get me there without having to use the mouse. However, with the down arrow feature, there's no way off initially getting there without using the mouse. You would have to click on the down arrow, either use the arrow keys or mouse to select "New Container Tab" and then use one of the keyboard shortcuts which I think defeats the purpose of having keyboard shortcuts for each container under that particular menu.

For example, the "Open Container Tab" button that's located under the hamburger menu doesn't have a keyboard shortcut to directly access the button, so there's no keyboard shortcuts associated with each container inside that menu.
OS: Mac OS X → Windows
Hardware: x86 → x86_64
Attachment #8814131 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8814131 [details]
Bug 1297738 - Removing accesskey from container menus that are not accessible via a keyboard

https://reviewboard.mozilla.org/r/95412/#review95662

::: browser/base/content/tabbrowser.xml:7123
(Diff revision 1)
>  
>      <handlers>
>        <handler event="popupshowing">
>        <![CDATA[
>          if (event.target.getAttribute("id") == "alltabs_containersMenuTab") {
> -          createUserContextMenu(event);
> +          createUserContextMenu(event, false, 0, false);

3 magical params (false, 0 false) is about the point where I start thinking we should take an options dict/object instead, making it clearer at the callsite what we're passing.
Attachment #8814131 - Flags: review?(gijskruitbosch+bugs)
Yeah, I totally agree also. Wasn't sure where your cutoff would be :). Will fix thank you.
Comment on attachment 8814131 [details]
Bug 1297738 - Removing accesskey from container menus that are not accessible via a keyboard

https://reviewboard.mozilla.org/r/95412/#review95706

::: browser/base/content/nsContextMenu.js:1853
(Diff revision 2)
> -    return createUserContextMenu(aEvent, true,
> -                                 gContextMenuContentData.userContextId);
> +    return createUserContextMenu(aEvent, {
> +                                           iscontextMenu: true,
> +                                           excludeUserContextId: gContextMenuContentData.userContextId
> +                                         });

Nit: trailing comma after `userContextId`.

I'd personally put this in a temp, but it's up to you.
Attachment #8814131 - Flags: review?(gijskruitbosch+bugs) → review+
Keywords: checkin-needed
You have to close the issue so landing can be triggered from mozreview.
Flags: needinfo?(jkt)
Done sorry about that. Thanks!
Flags: needinfo?(jkt)
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/bdaebda382c1
Removing accesskey from container menus that are not accessible via a keyboard r=Gijs
Keywords: checkin-needed
Backed out for failing browser_referrer_open_link_in_container_tab.js on OS X:

https://hg.mozilla.org/integration/autoland/rev/788b9b276f6590d3160b75d91c7ebba7854e748d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bdaebda382c1a6a8e94bb5c3217dfc6be4d0358e
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=7277741&repo=autoland

15:41:46     INFO - TEST-START | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab.js
15:41:49     INFO - TEST-INFO | started process screencapture
15:41:49     INFO - TEST-INFO | screencapture: exit 0
15:41:49     INFO - Buffered messages logged at 15:41:46
15:41:49     INFO - Console message: [JavaScript Error: "TypeError: win is null" {file: "resource:///modules/URLBarZoom.jsm" line: 24}]
15:41:49     INFO - Console message: [JavaScript Error: "TypeError: win is null" {file: "resource:///modules/URLBarZoom.jsm" line: 24}]
15:41:49     INFO - Buffered messages logged at 15:41:47
15:41:49     INFO - Console message: OpenGL compositor Initialized Succesfully.
15:41:49     INFO - Version: 2.1 INTEL-10.6.33
15:41:49     INFO - Vendor: Intel Inc.
15:41:49     INFO - Renderer: Intel Iris OpenGL Engine
15:41:49     INFO - FBO Texture Target: TEXTURE_2D
15:41:49     INFO - browser_referrer_open_link_in_container_tab: policy=[undefined] rel=[undefined] http:// -> http://
15:41:49     INFO - TEST-PASS | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab.js | We have a menupopup. - 
15:41:49     INFO - TEST-PASS | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab.js | We have a first container entry. - 
15:41:49     INFO - TEST-PASS | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab.js | We have a first container entry. - 
15:41:49     INFO - TEST-PASS | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab.js | We have a usercontextid value. - 
15:41:49     INFO - Buffered messages finished
15:41:49     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab.js | A promise chain failed to handle a rejection:  - TypeError: content.document.getElementById(...) is null
15:41:49     INFO - Stack trace:
15:41:49     INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: PendingErrors.register :: line 194
15:41:49     INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.completePromise :: line 715
15:41:49     INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 968
15:41:49     INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 813
15:42:31     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 9
15:43:16     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 8
15:44:01     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 7
15:44:46     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 6
15:45:31     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 5
15:46:16     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 4
15:47:01     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 3
15:47:46     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 2
15:48:02     INFO - Console message: [JavaScript Error: "1480290482116	Browser.Experiments.Experiments	ERROR	Experiments #0::httpGetRequest::onLoad() - Request to http://127.0.0.1:8888/experiments-dummy/manifest returned status 404" {file: "resource://gre/modules/Log.jsm" line: 753}]
15:48:02     INFO - App_append@resource://gre/modules/Log.jsm:753:9
15:48:02     INFO - Logger.prototype.log@resource://gre/modules/Log.jsm:389:7
15:48:02     INFO - LoggerRepository.prototype.getLoggerWithMessagePrefix/proxy.log@resource://gre/modules/Log.jsm:504:44
15:48:02     INFO - Experiments.Experiments/this._log.log@resource:///modules/experiments/Experiments.jsm:327:5
15:48:02     INFO - Logger.prototype.error@resource://gre/modules/Log.jsm:397:5
15:48:02     INFO - Experiments.Experiments.prototype._httpGetRequest/xhr.onload@resource:///modules/experiments/Experiments.jsm:958:9
15:48:02     INFO - EventHandlerNonNull*Experiments.Experiments.prototype._httpGetRequest@resource:///modules/experiments/Experiments.jsm:956:5
15:48:02     INFO - Experiments.Experiments.prototype._loadManifest@resource:///modules/experiments/Experiments.jsm:820:32
15:48:02     INFO - TaskImpl_run@resource://gre/modules/Task.jsm:319:42
15:48:02     INFO - TaskImpl@resource://gre/modules/Task.jsm:277:3
15:48:02     INFO - createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:252:14
15:48:02     INFO - Task_spawn@resource://gre/modules/Task.jsm:166:12
15:48:02     INFO - TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:389:16
15:48:02     INFO - TaskImpl_run@resource://gre/modules/Task.jsm:327:15
15:48:02     INFO - TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:401:7
15:48:02     INFO - TaskImpl_run@resource://gre/modules/Task.jsm:327:15
15:48:02     INFO - TaskImpl@resource://gre/modules/Task.jsm:277:3
15:48:02     INFO - createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:252:14
15:48:02     INFO - Task_spawn@resource://gre/modules/Task.jsm:166:12
15:48:02     INFO - TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:389:16
15:48:02     INFO - TaskImpl_run@resource://gre/modules/Task.jsm:327:15
15:48:02     INFO - TaskImpl@resource://gre/modules/Task.jsm:277:3
15:48:02     INFO - createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:252:14
15:48:02     INFO - Task_spawn@resource://gre/modules/Task.jsm:166:12
15:48:02     INFO - Experiments.Experiments.prototype._run@resource:///modules/experiments/Experiments.jsm:765:24
15:48:02     INFO - Experiments.Experiments.prototype.updateManifest@resource:///modules/experiments/Experiments.jsm:853:12
15:48:02     INFO - ExperimentsService.prototype.notify@jar:file:///builds/slave/test/build/application/Nightly.app/Contents/Resources/browser/omni.ja!/components/ExperimentsService.js:66:7
15:48:02     INFO - TM_notify/<@jar:file:///builds/slave/test/build/application/Nightly.app/Contents/Resources/omni.ja!/components/nsUpdateTimerManager.js:218:11
15:48:02     INFO - TM_notify@jar:file:///builds/slave/test/build/application/Nightly.app/Contents/Resources/omni.ja!/components/nsUpdateTimerManager.js:263:7
15:48:02     INFO -
Flags: needinfo?(jkt)
Change to fix the code was: isContextMenu: true, vs iscontextMenu: true,
Flags: needinfo?(jkt)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e0cb97c2a095
Removing accesskey from container menus that are not accessible via a keyboard r=Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e0cb97c2a095
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Platforms being used:

* Win 10 x64 - PASSED
* Ubuntu 16.04.1 LTS - PASSED

Test Cases Used:

* ensured that the access key indicators have been removed
* ensured that custom containers don't have any access key indicators
* ensured that you can still open containers without any issues
* ensured there's no error messages in the browser console when access containers via the dropdown arrow menu

It looks like the access keys are still working under Ubuntu/Win despite the indicators being removed, but I don't think that's really an issue.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.