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

VERIFIED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

({access})

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

Firefox Tracking Flags

(firefox17 verified, firefox18 verified, firefox19 verified)

Details

(Whiteboard: [Fx17])

Attachments

(1 attachment, 1 obsolete attachment)

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).
Assignee: nobody → felipc
Status: NEW → ASSIGNED
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().
Assignee: felipc → jaws
Attachment #672504 - Flags: review?(felipc)
Attachment #672504 - Attachment is obsolete: true
Attachment #672504 - Flags: review?(felipc)
Created attachment 672506 [details] [diff] [review]
Patch v1.1

Making the menuitem IDs unique.
Attachment #672506 - Flags: review?(felipc)
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
Attachment #672506 - Flags: review?(felipc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b453dca73b1a
Flags: in-testsuite+
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
Attachment #672506 - Flags: approval-mozilla-beta?
Attachment #672506 - Flags: approval-mozilla-aurora?
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?
Attachment #672506 - Flags: approval-mozilla-beta?
Attachment #672506 - Flags: approval-mozilla-beta+
Attachment #672506 - Flags: approval-mozilla-aurora?
Attachment #672506 - Flags: approval-mozilla-aurora+
Thanks for the feedback, I fixed both of these issues when relanding.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d82e14184e9f
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).
Relanded as https://hg.mozilla.org/integration/mozilla-inbound/rev/c9fa8d280644 since this was not the cause of the bustage.
https://hg.mozilla.org/mozilla-central/rev/c9fa8d280644
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Depends on: 805206
Didn't uplift this one yet as bug 805206 should be uplifted at the same time
https://hg.mozilla.org/releases/mozilla-aurora/rev/621c53bb4b0d
https://hg.mozilla.org/releases/mozilla-beta/rev/a1113192a7d7
status-firefox17: --- → fixed
status-firefox18: --- → fixed
status-firefox19: --- → fixed
Depends on: 810419
Marking verified fixed since this landed with tests. Please add verifyme keyword if there's something you need QA to test.
Status: RESOLVED → VERIFIED
status-firefox17: fixed → verified
status-firefox18: fixed → verified
status-firefox19: fixed → verified
You need to log in before you can comment on or make changes to this bug.