Note: There are a few cases of duplicates in user autocompletion which are being worked on.

implement toolbar ui

RESOLVED FIXED in Firefox 16

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 16
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx16])

Attachments

(3 attachments, 12 obsolete attachments)

1.13 MB, image/png
Details
45.13 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
6.85 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
tracking bug for landing toolbar button, menu and panels
(Assignee)

Comment 1

5 years ago
Created attachment 639984 [details]
themeimages.zip
(Assignee)

Comment 2

5 years ago
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
Attachment #639986 - Flags: feedback?(jaws)
Attachment #639986 - Flags: feedback?(gavin.sharp)
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 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).
Attachment #639986 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 5

5 years ago
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.
Attachment #639984 - Attachment is obsolete: true
Attachment #639986 - Attachment is obsolete: true
Attachment #639986 - Flags: feedback?(jaws)
Attachment #640012 - Flags: feedback?(jaws)
Attachment #640012 - Flags: feedback?(gavin.sharp)
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.
(Assignee)

Comment 7

5 years ago
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.
Attachment #640012 - Attachment is obsolete: true
Attachment #640012 - Flags: feedback?(jaws)
Attachment #640012 - Flags: feedback?(gavin.sharp)
Attachment #640867 - Flags: feedback?(jaws)
Attachment #640867 - Flags: feedback?(gavin.sharp)
(Assignee)

Updated

5 years ago
Depends on: 765874, 771980, 771877
(Assignee)

Updated

5 years ago
Blocks: 755136
(Assignee)

Comment 8

5 years ago
Created attachment 641299 [details] [diff] [review]
toolbarui.patch

updated with tests
Attachment #640867 - Attachment is obsolete: true
Attachment #640867 - Flags: feedback?(jaws)
Attachment #640867 - Flags: feedback?(gavin.sharp)
Attachment #641299 - Flags: feedback?(jaws)
Attachment #641299 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 9

5 years ago
Created attachment 641525 [details] [diff] [review]
toolbarui.patch

update of patch to current m-c
Attachment #641299 - Attachment is obsolete: true
Attachment #641299 - Flags: feedback?(jaws)
Attachment #641299 - Flags: feedback?(gavin.sharp)
Attachment #641525 - Flags: feedback?(jaws)
Attachment #641525 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 10

5 years ago
Created attachment 641648 [details] [diff] [review]
toolbarui.patch

updated against latest patches
Attachment #641525 - Attachment is obsolete: true
Attachment #641525 - Flags: feedback?(jaws)
Attachment #641525 - Flags: feedback?(gavin.sharp)
Attachment #641648 - Flags: feedback?(gavin.sharp)
Depends on: 773529
Depends on: 773530
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.
Created attachment 641695 [details] [diff] [review]
just the UI pieces
Attachment #641648 - Attachment is obsolete: true
Attachment #641648 - Flags: feedback?(gavin.sharp)
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.
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!
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.
Attachment #641695 - Attachment is obsolete: true
Attachment #642147 - Flags: feedback?(mixedpuppy)
(Assignee)

Updated

5 years ago
Depends on: 764872
(Assignee)

Updated

5 years ago
Blocks: 764872
No longer depends on: 764872
(Assignee)

Updated

5 years ago
Depends on: 774003
(Assignee)

Comment 16

5 years ago
Created attachment 642292 [details] [diff] [review]
gavins patch plus css and button state fixes
Attachment #642147 - Attachment is obsolete: true
Attachment #642147 - Flags: feedback?(mixedpuppy)
Attachment #642292 - Flags: review?(gavin.sharp)
(Assignee)

Comment 17

5 years ago
Created attachment 642304 [details] [diff] [review]
gavins patch plus css and button state fixes, fixed test
Attachment #642292 - Attachment is obsolete: true
Attachment #642292 - Flags: review?(gavin.sharp)
Attachment #642304 - Flags: review?(gavin.sharp)
(Assignee)

Comment 18

5 years ago
Created attachment 642445 [details] [diff] [review]
w/logout state fixed
Attachment #642304 - Attachment is obsolete: true
Attachment #642304 - Flags: review?(gavin.sharp)
Attachment #642445 - Flags: review?(gavin.sharp)
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.
Attachment #642445 - Attachment is obsolete: true
Attachment #642445 - Flags: review?(gavin.sharp)
Attachment #642464 - Flags: review+
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]).
Attachment #642465 - Flags: feedback?(mixedpuppy)
Depends on: 774174
Depends on: 774176
Depends on: 774178
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca832757d4b7
Flags: in-testsuite+
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 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.
Re-landed with those changes, which fix the a11y test failure:
https://hg.mozilla.org/mozilla-central/rev/934ef44ce5af
Status: NEW → RESOLVED
Last Resolved: 5 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
No longer depends on: 774176
Depends on: 764787
Target Milestone: --- → Firefox 16
Attachment #642465 - Flags: feedback?(mixedpuppy)
Depends on: 774725

Comment 25

5 years ago
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.
Blocks: 777940
Depends on: 780349
Depends on: 780757
No longer depends on: 780757
Duplicate of this bug: 763837

Updated

5 years ago
Depends on: 808641
You need to log in before you can comment on or make changes to this bug.