801040 - Add Social API status-area menuitems to the keyboard accessible Social API menu

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

Bug 792503 added a keyboard accessible menu for the social API panels. If the user accesses this menu, they should also see the same items that are found in the status area menu (who they are logged in as, the ability to hide the sidebar, the ability to disable desktop notifications).

Comment 1

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

Attachment: Patch

This patch adds the menuitems and also moves the population of the menupopup to the point where the ambient notification icons are added instead of onpopupshowing. There was also a call to a non-existant SocialMenu.updateMenu() function, which should have been changed to SocialMenu.populate().

Comment 2

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

Attachment: Patch v1.1

Making the menuitem IDs unique.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 672506 [details] [diff] [review]
Patch v1.1

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

::: browser/base/content/browser-social.js
@@ +610,5 @@
>      // This menu is only accessible through keyboard navigation.
>      let submenu = document.getElementById("menu_socialAmbientMenuPopup");
> +    let ambientMenuItems = submenu.getElementsByClassName("ambient-menuitem");
> +    for (let ambientMenuItem of ambientMenuItems)
> +      submenu.removeChild(ambientMenuItem);

wrap this for loop in {} please for readability

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b453dca73b1a

Comment 5

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

Comment on attachment 672506 [details] [diff] [review]
Patch v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: poor a11y support for keyboard users with social api
Testing completed (on m-c, etc.): locally and automated tests
Risk to taking this patch (and alternatives if risky): none expected
String or UUID changes made by this patch: none

Comment 6

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

Comment on attachment 672506 [details] [diff] [review]
Patch v1.1

>diff --git a/browser/base/content/browser-menubar.inc b/browser/base/content/browser-menubar.inc

>               <menu id="menu_socialAmbientMenu"
>                     class="show-only-for-keyboard"
>                     command="Social:Toggle">

Not related to this bug, but that "command" attribute shouldn't be there.

>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

>   init: function SocialToolbar_init() {
>     this.button.setAttribute("image", Social.provider.iconURL);
>     this.updateButton();
>+    SocialMenu.populate();

Does this need to happen here? I don't see any dependencies between SocialMenu and SocialToolbar, so why not put this call in _providerReady?

Comment 7

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

Thanks for the feedback, I fixed both of these issues when relanding.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d82e14184e9f

Comment 8

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

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/35ab233bf290.

There were a bunch of errors in the logs:
> TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_social_chatwindow.js | Console message: [JavaScript Error: "sbrowser.docShell is undefined" {file: "chrome://browser/content/browser.js" line: 4814}]
> TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_social_chatwindow.js | Console message: [JavaScript Error: "sbrowser.docShell is undefined" {file: "chrome://browser/content/browser.js" line: 4814}]
> TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_social_chatwindow.js | Console message: [JavaScript Error: "FrameWorker: Port MessagePort(portType='client', portId=null) handler failed: TypeError: aURI is undefined
> Social_getShareablePageUrl@resource://gre/modules/Social.jsm:76
> Social_isPageShared@resource://gre/modules/Social.jsm:85
> SSB_updateShareState@chrome://browser/content/browser.js:4448
> SSB_updateProfileInfo/port.onmessage<@chrome://browser/content/browser.js:4332
> fw_AbstractPort_onmessage@resource://gre/modules/MessagePortBase.jsm:55
> _messageHandler@resource://gre/modules/FrameWorker.jsm:363
> messageHandler@resource://gre/modules/FrameWorker.jsm:373
> " {file: "resource://gre/modules/FrameWorker.jsm" line: 435}]

The only test-unexpected-fail though was the following:
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_workerAPI.js | Exception thrown at resource://gre/modules/SocialService.jsm:112 - Error: SocialService.addProvider: provider with this origin already exists 

I'm not sure if it was this patch or the one from bug 787767 (which I also backed out).

Comment 9

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

Relanded as https://hg.mozilla.org/integration/mozilla-inbound/rev/c9fa8d280644 since this was not the cause of the bustage.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c9fa8d280644

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Didn't uplift this one yet as bug 805206 should be uplifted at the same time

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/621c53bb4b0d
https://hg.mozilla.org/releases/mozilla-beta/rev/a1113192a7d7

Comment 13

[u279076]

Marking verified fixed since this landed with tests. Please add verifyme keyword if there's something you need QA to test.