771826 - implement toolbar ui

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

tracking bug for landing toolbar button, menu and panels

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: themeimages.zip

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: socialui.patch

full ui patch including the toolbarbutton, sidebar panel, strings (dtd), show/hide and enable/disable command setup.

this patch is for early feedback

Comment 3

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

FWIW, if you |hg add| the image files, they'll show up in the hg diffs (you might also need to enable [diff] git = 1 in your ~/.hgrc if you haven't already, see https://developer.mozilla.org/en/Installing_Mercurial#Configuration).

Comment 4

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

Comment on attachment 639986 [details] [diff] [review]
socialui.patch

>diff -r a8f682801a6d browser/base/content/browser-doctype.inc

>+<!ENTITY % socialDTD SYSTEM "chrome://browser/locale/social/social.dtd">
>+%socialDTD;

Let's just add strings to browser.dtd

>diff -r a8f682801a6d browser/base/content/browser-menubar.inc

>+# XXX temporary addition of menuitem to help visual testing of enable/disable
>+              <menuitem id="social-togglenable-toolsmenu"
>+                        command='cmd_socialBrowsingToggle'
>+                        type="checkbox"/>

There's some conflict with bug 764872 here (social.js conflicts too) - perhaps this work should be based on top of that. And then the sidebar pieces should probably be on top of this (in bug 755136).

>diff -r a8f682801a6d browser/base/content/browser.xul

>+<?xml-stylesheet href="chrome://browser/skin/social/social.css" type="text/css"?>

Similarly, this stuff should go in browser.css.

>diff -r a8f682801a6d browser/locales/en-US/chrome/browser/social/social.dtd

>+<!ENTITY social.label               "Social">
>+<!ENTITY socialShare.accesskey      "F2">
>+<!ENTITY social.toolbartext         "Social Browsing">
>+<!ENTITY social.prefslabel          "Manage Social Services...">

These strings look unused.

>+<!ENTITY socialsidebar.label        "Show Social sidebar">

We'll probably want to add this to the View->Sidebar menu, so the label would just be "Social"

>diff -r a8f682801a6d toolkit/components/social/SocialServiceWindow.jsm

>+function getWebProgressListener(aOrigin) {

We'll probably want to give this a more unique name (getSameOriginEnforcingListener?)

>+  return {
>+    QueryInterface: function(aIID) {

You could use XPCOMUtils.generateQI. Though, is nsIWeakReference really necessary? I thought the webprogress only held strong refs to listeners. If so, we can just remove the QI impl entirely (xpconnect deals just fine without one).

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: socialui.patch

built on top of patches from bug 765874 and bug 764872

Jared, this stomps a bit on your work in bug 764872, but I just needed to see enabled and sidebar toggling work from the toolbar.

Comment 6

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

As a heads up, I'm in the middle of fixing up the review comments from Gavin and splitting the work from 765874 in to a UI-agnostic module and the UI-facing part will most likely bit rot this work.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: toolbarui.patch

for initial feedback.  patch implements ui for toolbar as well has hooking up ambient notifications, status panels and feature enable/disable toggling.  needs tests still.  This is built on top of patches from bugs 771877, 771980, and 765874.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: toolbarui.patch

updated with tests

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: toolbarui.patch

update of patch to current m-c

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: toolbarui.patch

updated against latest patches

Comment 11

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

I've split some pieces out into their own patches:
- bug 773530 (workerAPI changes)
- bug 773529 (SocialService add/remove methods)

I'll attach the remained from this patch as a patch rebased on top of those.

Comment 12

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

Attachment: just the UI pieces

Comment 13

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

Comment on attachment 641695 [details] [diff] [review]
just the UI pieces

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

The parts of this patch that focus on enabling the social browsing features should be part of bug 764872. I'll update that patch today.

Comment 14

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Different states of social toolbar ui

(In reply to Jared Wein [:jaws] from comment #6)
> As a heads up, I'm in the middle of fixing up the review comments from Gavin
> and splitting the work from 765874 in to a UI-agnostic module and the
> UI-facing part will most likely bit rot this work.

For this design to be cohesive in the Firefox toolbar, we need to apply some of our own styles onto the icons supplied by social networks so that they blend better with native icons and styling.  I'm attaching a mockup of the potential states the toolbar could be in - if there's any I'm forgetting, please let me know!

Comment 15

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

Attachment: just the UI pieces, trimmed down

This makes a few changes and trims down the code a little bit. This regresses the appearance somewhat on Mac, but results in simpler code - I think we should work on improved styling in followups. It also removes some functionality - the toggle menu items, the provider list, and removes strings from the "profile" area to avoid 110n issues. All stuff we can revisit on top of this patch - I just wanted to reduce scope and get this landed ASAP. We're going to need to iterate a bunch on the exact functionality and some edge cases, too.

The windows styling in this patch is probably broken, I haven't tested it at all.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: gavins patch plus css and button state fixes

Comment 17

[Shane Caraveo (:mixedpuppy)]

Attachment: gavins patch plus css and button state fixes, fixed test

Comment 18

[Shane Caraveo (:mixedpuppy)]

Attachment: w/logout state fixed

Comment 19

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

Attachment: patch with a few tweaks

Trimmed a few other things per discussion on IRC. I'll attach an interdiff for review before landing this with r=me.

Comment 20

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

Attachment: interdiff

This is the interdiff between the patch I attached (attachment 642464 [details] [diff] [review]) and Shane's latest patch (attachment 642445 [details] [diff] [review]).

Comment 21

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

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

Comment 22

[Ed Morley [:emorley]]

Push backed out for mochitest-a11y failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13567358&tree=Mozilla-Inbound

{
20840 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tree/test_dochierarchy.html | Wrong child document count of root accessible - got 2, expected 1
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/082542b01af8

Comment 23

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

Comment on attachment 642464 [details] [diff] [review]
patch with a few tweaks

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

::: browser/base/content/browser.xul
@@ +220,5 @@
>  #endif
>        </hbox>
>      </panel>
>  
> +    <panel id="social-notification-panel" type="arrow">

I think this panel should have hidden="true" and noautofocus="true" attributes.

Comment 24

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

Re-landed with those changes, which fix the a11y test failure:
https://hg.mozilla.org/mozilla-central/rev/934ef44ce5af

Comment 25

[Todd]

This may be related to 774725.

On Windows the toolbar is not showing up properly.  Turn on social.enabled and the issue will be obvious.