Last Comment Bug 771826 - implement toolbar ui
: implement toolbar ui
Status: RESOLVED FIXED
[Fx16]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Shane Caraveo (:mixedpuppy)
:
Mentors:
: 763837 (view as bug list)
Depends on: 808641 763837 764787 765874 771877 771980 773529 773530 774003 774174 774178 774725 780349
Blocks: 755136 764872 777940
  Show dependency treegraph
 
Reported: 2012-07-07 12:14 PDT by Shane Caraveo (:mixedpuppy)
Modified: 2013-12-27 14:35 PST (History)
10 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
themeimages.zip (10.01 KB, application/x-zip-compressed)
2012-07-07 12:14 PDT, Shane Caraveo (:mixedpuppy)
no flags Details
socialui.patch (29.63 KB, patch)
2012-07-07 12:15 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
socialui.patch (32.87 KB, patch)
2012-07-07 18:12 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
toolbarui.patch (46.01 KB, patch)
2012-07-10 17:24 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
toolbarui.patch (57.80 KB, patch)
2012-07-11 18:10 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
toolbarui.patch (56.77 KB, patch)
2012-07-12 10:39 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
toolbarui.patch (57.70 KB, patch)
2012-07-12 16:34 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
just the UI pieces (55.70 KB, patch)
2012-07-12 19:19 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
Mockup: Different states of social toolbar ui (1.13 MB, image/png)
2012-07-13 15:30 PDT, Jennifer Morrow [:Boriss] (UX)
no flags Details
just the UI pieces, trimmed down (46.21 KB, patch)
2012-07-13 17:16 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
gavins patch plus css and button state fixes (39.78 KB, patch)
2012-07-14 17:18 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
gavins patch plus css and button state fixes, fixed test (39.82 KB, patch)
2012-07-14 18:51 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
w/logout state fixed (40.92 KB, patch)
2012-07-15 16:21 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
patch with a few tweaks (45.13 KB, patch)
2012-07-15 19:17 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
gavin.sharp: review+
Details | Diff | Splinter Review
interdiff (6.85 KB, patch)
2012-07-15 19:19 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review

Description Shane Caraveo (:mixedpuppy) 2012-07-07 12:14:07 PDT
tracking bug for landing toolbar button, menu and panels
Comment 1 Shane Caraveo (:mixedpuppy) 2012-07-07 12:14:33 PDT
Created attachment 639984 [details]
themeimages.zip
Comment 2 Shane Caraveo (:mixedpuppy) 2012-07-07 12:15:30 PDT
Created attachment 639986 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-07-07 15:13:59 PDT
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: gavin@gavinsharp.com] 2012-07-07 15:48:17 PDT
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) 2012-07-07 18:12:11 PDT
Created attachment 640012 [details] [diff] [review]
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) 2012-07-07 18:56:11 PDT
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) 2012-07-10 17:24:24 PDT
Created attachment 640867 [details] [diff] [review]
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) 2012-07-11 18:10:16 PDT
Created attachment 641299 [details] [diff] [review]
toolbarui.patch

updated with tests
Comment 9 Shane Caraveo (:mixedpuppy) 2012-07-12 10:39:39 PDT
Created attachment 641525 [details] [diff] [review]
toolbarui.patch

update of patch to current m-c
Comment 10 Shane Caraveo (:mixedpuppy) 2012-07-12 16:34:35 PDT
Created attachment 641648 [details] [diff] [review]
toolbarui.patch

updated against latest patches
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-12 19:18:43 PDT
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: gavin@gavinsharp.com] 2012-07-12 19:19:04 PDT
Created attachment 641695 [details] [diff] [review]
just the UI pieces
Comment 13 Jared Wein [:jaws] (please needinfo? me) 2012-07-13 10:46:23 PDT
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) 2012-07-13 15:30:53 PDT
Created attachment 642100 [details]
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: gavin@gavinsharp.com] 2012-07-13 17:16:25 PDT
Created attachment 642147 [details] [diff] [review]
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) 2012-07-14 17:18:53 PDT
Created attachment 642292 [details] [diff] [review]
gavins patch plus css and button state fixes
Comment 17 Shane Caraveo (:mixedpuppy) 2012-07-14 18:51:16 PDT
Created attachment 642304 [details] [diff] [review]
gavins patch plus css and button state fixes, fixed test
Comment 18 Shane Caraveo (:mixedpuppy) 2012-07-15 16:21:25 PDT
Created attachment 642445 [details] [diff] [review]
w/logout state fixed
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-15 19:17:51 PDT
Created attachment 642464 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-07-15 19:19:16 PDT
Created attachment 642465 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-07-16 00:39:38 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca832757d4b7
Comment 22 Ed Morley [:emorley] 2012-07-16 02:11:08 PDT
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) 2012-07-16 08:23:16 PDT
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: gavin@gavinsharp.com] 2012-07-16 08:52:32 PDT
Re-landed with those changes, which fix the a11y test failure:
https://hg.mozilla.org/mozilla-central/rev/934ef44ce5af
Comment 25 Todd 2012-07-19 08:41:57 PDT
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.
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-24 18:04:57 PDT
*** Bug 763837 has been marked as a duplicate of this bug. ***

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