792503 - Keyboard access for Social API ambient notification panels

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

We'll want sumo docs for this too.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Shane is there a keyboard shortcut for the jewels?

Comment 2

[Shane Caraveo (:mixedpuppy)]

(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.

Comment 4

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

There is keyboard access for the share button. It is currently Ctrl+Shift+L.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

I think we need the jewels in the tab navigation order. Jared who is the right person to own this bug?

Comment 6

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

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

[alexander :surkov (:asurkov)]

(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

Comment 8

[Shane Caraveo (:mixedpuppy)]

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

[alexander :surkov (:asurkov)]

should the provider take care of shortcuts then?

Comment 10

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

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.

Comment 11

[Shane Caraveo (:mixedpuppy)]

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?

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 13

[Alex Keybl [:akeybl]]

All tracked bugs need assignees. Starting with you jaws.

Comment 14

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

Attachment: WIP patch

Comment 15

[Mark Hammond [:markh] [: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.

Comment 16

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

Attachment: 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.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 18

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

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

Comment 19

[Mark Hammond [:markh] [:mhammond]]

Backed out due to osx orange - https://hg.mozilla.org/integration/mozilla-inbound/rev/79e5f4ec83ae

Comment 20

[Dão Gottwald [:dao]]

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) {

Comment 21

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

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.

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c33c86c1a020
https://hg.mozilla.org/mozilla-central/rev/510e593b160b

Comment 23

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

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

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Why did we decide to open the URL in a tab? Can't we just open the panel as usual, and focus it?

Comment 25

[Matthew N. [:MattN]]

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

Comment 26

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

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.