Status

defect
RESOLVED FIXED
7 years ago
5 months ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

unspecified
Firefox 16
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
tracking bug for landing toolbar button, menu and panels
Posted file themeimages.zip (obsolete) —
Posted patch socialui.patch (obsolete) — Splinter Review
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)
Posted patch socialui.patch (obsolete) — Splinter Review
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.
Posted patch toolbarui.patch (obsolete) — Splinter Review
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)
Depends on: 765874, 771980, 771877
Blocks: 755136
Posted patch toolbarui.patch (obsolete) — Splinter Review
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)
Posted patch toolbarui.patch (obsolete) — Splinter Review
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)
Posted patch toolbarui.patch (obsolete) — Splinter Review
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)
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.
Posted patch just the UI pieces (obsolete) — Splinter Review
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.
(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!
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)
Depends on: 764872
Blocks: 764872
No longer depends on: 764872
Depends on: 774003
Attachment #642147 - Attachment is obsolete: true
Attachment #642147 - Flags: feedback?(mixedpuppy)
Attachment #642292 - Flags: review?(gavin.sharp)
Attachment #642292 - Attachment is obsolete: true
Attachment #642292 - Flags: review?(gavin.sharp)
Attachment #642304 - Flags: review?(gavin.sharp)
Posted patch w/logout state fixed (obsolete) — Splinter Review
Attachment #642304 - Attachment is obsolete: true
Attachment #642304 - Flags: review?(gavin.sharp)
Attachment #642445 - Flags: review?(gavin.sharp)
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+
Posted patch interdiffSplinter Review
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)
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
Closed: 7 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
No longer depends on: 774176
Target Milestone: --- → Firefox 16
Attachment #642465 - Flags: feedback?(mixedpuppy)
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.
Depends on: 780349
Depends on: 780757
Duplicate of this bug: 763837
Depends on: 808641
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.