Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Keyboard access for Social API ambient notification panels

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: davidb, Assigned: jaws)

Tracking

({access})

Trunk
Firefox 19
access
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17+ fixed, firefox18 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
We'll want sumo docs for this too.
(Reporter)

Comment 1

5 years ago
Shane is there a keyboard shortcut for the jewels?
(In reply to David Bolter [:davidb] from comment #1)
> Shane is there a keyboard shortcut for the jewels?

Currently there are no key bindings for the toolbar button and its jewels.
Whiteboard: [Fx17]
Duplicate of this bug: 794270
tracking-firefox17: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [Fx17]
Version: unspecified → Trunk
There is keyboard access for the share button. It is currently Ctrl+Shift+L.
(Reporter)

Comment 5

5 years ago
I think we need the jewels in the tab navigation order. Jared who is the right person to own this bug?
Is it normal for us to have keyboard accessibility for toolbar buttons? Do these extra items need to be added to one of the Alt+ menus?

Comment 7

5 years ago
(In reply to Jared Wein [:jaws] from comment #6)
> Is it normal for us to have keyboard accessibility for toolbar buttons?

Usually they aren't in tab navigation order (I'm not really sure why though). Keyboard shortcuts might be a good idea.

> Do
> these extra items need to be added to one of the Alt+ menus?

if no tab navigation and shortcuts then yes
How do we determine appropriate keyboard shortcuts for buttons that are a) defined remotely by the provider and b) may be of varying numbers (1 - 3 buttons are definable by the provider).

Comment 9

5 years ago
should the provider take care of shortcuts then?
I think a submenu on the Tools menuitem for the social provider would work. We wouldn't have access keys, but keyboard navigation and a screen reader would let the user navigate.

What would happen when a user selected one of these menuitems is another question all-together, since our panel-based approach is probably not sufficient.
I guess we could expand the ambient notification api to include a user readable name for each icon (to be used in the menu) and a web-url to be opened in a tab if launched from the menu.  It feels suboptimal.  I'm not familiar with how current toolbar buttons deal with this, e.g. how does the downloads button/panel work with this issue?
(Reporter)

Comment 12

5 years ago
I'm not sure how up to date this is but this was the general position on toolbar buttons: https://developer.mozilla.org/en-US/docs/XUL_accessibility_guidelines#Toolbarbuttons
All tracked bugs need assignees. Starting with you jaws.
Assignee: nobody → jaws
tracking-firefox17: ? → +
Created attachment 667233 [details] [diff] [review]
WIP patch
Attachment #667233 - Flags: feedback?(mhammond)
Comment on attachment 667233 [details] [diff] [review]
WIP patch

I wonder if the menu, menupopup etc elements should be defined in the .xul, in the same way the "containers" for the toolbar icons etc are?

> throw new Error("Required parameters for notifications are:

Is that going to break existing providers?  ie, are our existing providers already supplying all those fields?

No tests :)  If this is time critical, then a new bug should be opened to add them.
Attachment #667233 - Flags: feedback?(mhammond) → feedback+
Created attachment 670195 [details] [diff] [review]
Add a keyboard accessible menuitem for the ambient notification icon pages.

Thanks for the feedback. I moved the standard menu-bits to browser-menubar.inc and wrote some tests for it too. This patch also doesn't require the |label| or |menuURL| properties, but if they are there then it will use it.

In the future this should be an API requirement though.
Attachment #667233 - Attachment is obsolete: true
Attachment #670195 - Flags: review?(mhammond)
Attachment #670195 - Attachment description: Patch → Add a keyboard accessible menuitem for the ambient notification icon pages.
Comment on attachment 670195 [details] [diff] [review]
Add a keyboard accessible menuitem for the ambient notification icon pages.

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

LGTM

::: browser/base/content/browser-menubar.inc
@@ +474,5 @@
>                    label="&toolsMenu.label;"
>                    accesskey="&toolsMenu.accesskey;">
>                <menupopup id="menu_ToolsPopup"
>  #ifdef MOZ_SERVICES_SYNC
> +                         onpopupshowing="gSyncUI.updateUI(); SocialMenu.populate();"

Can't we do SocialMenu.populate in the menu_socialAmbientMenuPopup element?

::: browser/base/content/browser-social.js
@@ +742,5 @@
>        } else {
>          let box = document.createElement("box");
>          box.classList.add("toolbarbutton-1");
>          box.setAttribute("id", iconId);
> +        if (icon.label)

'label' seems a poor choice for the item that will be displayed as tooltip text - is there any reason we can't use 'tooltiptext'?  (eg, I could imagine 'label' being used as an aria-label, which may use different text than the tooltip.  I don't feel too strongly about this though.
Attachment #670195 - Flags: review?(mhammond) → review+
We can't do SocialMenu.populate in the menu_socialAmbientMenuPopup element because we might need to hide the menu_socialAmbientMenu element if there are no children menuitems.

'label' is a poor choice, but it is a reuse of the accessible menu text for the menuitem. I added a comment in the code to explain this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/83038b3ca98d
Flags: in-testsuite+
Backed out due to osx orange - https://hg.mozilla.org/integration/mozilla-inbound/rev/79e5f4ec83ae
Comment on attachment 670195 [details] [diff] [review]
Add a keyboard accessible menuitem for the ambient notification icon pages.

>+var SocialMenu = {
>+  populate: function SocialMenu_populate() {
>+    // This menu is only accessible through keyboard navigation.
>+    let submenu = document.getElementById("menu_socialAmbientMenuPopup");
>+    while(submenu.hasChildNodes())

while (

>+          submenu.removeChild(submenu.firstChild);

indentation is off

>+    let provider = Social.provider;
>+    if (Social.active && provider) {
>+      let iconNames = Object.keys(provider.ambientNotificationIcons);
>+      for each(let name in iconNames) {

for (let name of iconNames) {
https://hg.mozilla.org/integration/mozilla-inbound/rev/c33c86c1a020
https://hg.mozilla.org/integration/mozilla-inbound/rev/510e593b160b

Thanks for the feedback Dao. The tests that check the menubar are skipped on Mac.
https://hg.mozilla.org/mozilla-central/rev/c33c86c1a020
https://hg.mozilla.org/mozilla-central/rev/510e593b160b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Comment on attachment 670195 [details] [diff] [review]
Add a keyboard accessible menuitem for the ambient notification icon pages.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: keyboard users have no way to access social api panels
Testing completed (on m-c, etc.): locally, landed on m-c
Risk to taking this patch (and alternatives if risky): none expected
String or UUID changes made by this patch: no string changes
Attachment #670195 - Flags: approval-mozilla-beta?
Attachment #670195 - Flags: approval-mozilla-aurora?
Summary: Ensure keyboard access to all social features. → Keyboard access for Social API ambient notification panels
Blocks: 801040

Updated

5 years ago
Attachment #670195 - Flags: approval-mozilla-beta?
Attachment #670195 - Flags: approval-mozilla-beta+
Attachment #670195 - Flags: approval-mozilla-aurora?
Attachment #670195 - Flags: approval-mozilla-aurora+
Why did we decide to open the URL in a tab? Can't we just open the panel as usual, and focus it?
I didn't see Gavin's comment until now.  I guess comment 24 can be addressed in a follow-up, if necessary.

https://hg.mozilla.org/releases/mozilla-aurora/rev/faf198fbb535
https://hg.mozilla.org/releases/mozilla-beta/rev/a9257aa0d193
status-firefox17: --- → fixed
status-firefox18: --- → fixed
Followup bustage fix to change .contains to .indexOf for Fx17 since it was disabled for Fx17:

https://hg.mozilla.org/releases/mozilla-beta/rev/4a6eed491eaf

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=801964 based on comment #24.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.