Last Comment Bug 792503 - Keyboard access for Social API ambient notification panels
: Keyboard access for Social API ambient notification panels
Status: RESOLVED FIXED
[qa-]
: access
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 19
Assigned To: (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
:
Mentors:
: 794270 (view as bug list)
Depends on:
Blocks: 759414 801040
  Show dependency treegraph
 
Reported: 2012-09-19 11:41 PDT by David Bolter [:davidb]
Modified: 2012-10-16 16:13 PDT (History)
9 users (show)
jaws: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
WIP patch (4.87 KB, patch)
2012-10-02 16:29 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
markh: feedback+
Details | Diff | Review
Add a keyboard accessible menuitem for the ambient notification icon pages. (11.20 KB, patch)
2012-10-10 17:37 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
markh: review+
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
Details | Diff | Review

Description David Bolter [:davidb] 2012-09-19 11:41:25 PDT
We'll want sumo docs for this too.
Comment 1 David Bolter [:davidb] 2012-09-21 09:55:56 PDT
Shane is there a keyboard shortcut for the jewels?
Comment 2 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-09-25 13:10:03 PDT
(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 3 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-09-25 15:49:07 PDT
*** Bug 794270 has been marked as a duplicate of this bug. ***
Comment 4 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-09-25 15:50:38 PDT
There is keyboard access for the share button. It is currently Ctrl+Shift+L.
Comment 5 David Bolter [:davidb] 2012-09-26 06:11:13 PDT
I think we need the jewels in the tab navigation order. Jared who is the right person to own this bug?
Comment 6 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-09-26 13:29:31 PDT
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 2012-09-26 13:51:10 PDT
(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) [on leave 5/16-7/16] 2012-09-26 13:53:35 PDT
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 2012-09-26 13:55:59 PDT
should the provider take care of shortcuts then?
Comment 10 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-09-26 13:56:49 PDT
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) [on leave 5/16-7/16] 2012-09-26 14:10:32 PDT
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] 2012-09-26 14:38:36 PDT
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] 2012-09-28 15:20:25 PDT
All tracked bugs need assignees. Starting with you jaws.
Comment 14 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-02 16:29:08 PDT
Created attachment 667233 [details] [diff] [review]
WIP patch
Comment 15 Mark Hammond [:markh] 2012-10-02 18:22:36 PDT
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-10 17:37:39 PDT
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.
Comment 17 Mark Hammond [:markh] 2012-10-10 18:03:29 PDT
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-10 19:36:18 PDT
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] 2012-10-10 20:45:46 PDT
Backed out due to osx orange - https://hg.mozilla.org/integration/mozilla-inbound/rev/79e5f4ec83ae
Comment 20 Dão Gottwald [:dao] 2012-10-11 01:05:17 PDT
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-11 18:38:07 PDT
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 23 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-12 11:26:55 PDT
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: gavin@gavinsharp.com] 2012-10-12 15:46:55 PDT
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] (behind on reviews) 2012-10-15 16:28:11 PDT
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-15 17:47:19 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.