Last Comment Bug 801040 - Add Social API status-area menuitems to the keyboard accessible Social API menu
: Add Social API status-area menuitems to the keyboard accessible Social API menu
Status: VERIFIED FIXED
[Fx17]
: access
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 19
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
Mentors:
Depends on: 792503 805206 810419
Blocks: 759414
  Show dependency treegraph
 
Reported: 2012-10-12 11:34 PDT by Jared Wein [:jaws] (please needinfo? me)
Modified: 2012-12-04 14:57 PST (History)
4 users (show)
jaws: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified


Attachments
Patch (8.42 KB, patch)
2012-10-17 13:44 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Review
Patch v1.1 (9.50 KB, patch)
2012-10-17 13:50 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
gavin.sharp: approval‑mozilla‑aurora+
gavin.sharp: approval‑mozilla‑beta+
Details | Diff | Review

Description Jared Wein [:jaws] (please needinfo? me) 2012-10-12 11:34:51 PDT
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) 2012-10-17 13:44:28 PDT
Created attachment 672504 [details] [diff] [review]
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) 2012-10-17 13:50:37 PDT
Created attachment 672506 [details] [diff] [review]
Patch v1.1

Making the menuitem IDs unique.
Comment 3 :Felipe Gomes (needinfo me!) 2012-10-18 17:37:15 PDT
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) 2012-10-18 18:06:33 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b453dca73b1a
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2012-10-18 18:07:15 PDT
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: gavin@gavinsharp.com] 2012-10-18 18:48:39 PDT
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) 2012-10-21 23:58:00 PDT
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) 2012-10-22 02:11:58 PDT
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) 2012-10-23 12:33:42 PDT
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] 2012-10-23 19:56:58 PDT
https://hg.mozilla.org/mozilla-central/rev/c9fa8d280644
Comment 11 :Felipe Gomes (needinfo me!) 2012-10-25 13:48:42 PDT
Didn't uplift this one yet as bug 805206 should be uplifted at the same time
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 14:57:14 PST
Marking verified fixed since this landed with tests. Please add verifyme keyword if there's something you need QA to test.

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