594488 - move status bar UI to an optional toolbar button

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Mike Connor [:mconnor]]

users will be able to add and place this as they see fit.

Comment 1

[Mike Connor [:mconnor]]

Attachment: wip

Comment 2

[Mike Connor [:mconnor]]

Attachment: v1

This is placeholder CSS until we get new images, but I want to get this in ASAP.

Comment 3

[Mike Connor [:mconnor]]

Alex/Stephen: basically, we're moving the statusbar UI to an optional toolbar button.  I've used the existing sync/sync-throbber icons, but those don't fit especially well, so I assume you'll want to do something better for this.  Separate bug, I assume?

Comment 4

[Stephen Horlander [:shorlander]]

(In reply to comment #3)
> Alex/Stephen: basically, we're moving the statusbar UI to an optional toolbar
> button.  I've used the existing sync/sync-throbber icons, but those don't fit
> especially well, so I assume you'll want to do something better for this. 
> Separate bug, I assume?

Yeah we will want to match the other glyphs. So another bug would be just fine. Thanks!

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

Will we just use a tooltip for showing the time since last sync?  Another option is a split menu button, and in the menu have the same sub-menu that we have under tools.

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

Follow up bug 594663 to cover the secondary UI command in the Firefox menu.

Comment 7

[Mike Connor [:mconnor]]

Yeah, I think the tooltip is sufficient, tbh.

Comment 8

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

Comment on attachment 473391 [details] [diff] [review]
v1

>diff --git a/browser/base/content/browser-syncui.js b/browser/base/content/browser-syncui.js

>-      if (!this._isLoggedIn()) {
>-        //XXXzpao When we move the string bundle, we can add more and make this
>-        //        say "needs setup" or something similar. (bug 583381)
>-        button.removeAttribute("tooltiptext");
>-      }
>+      document.getElementById("sync-button").removeAttribute("status");

Do you also need to remove tooltiptext here?

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>+      <toolbarbutton id="sync-button"

Need to add this to browser/themes/browserShared.inc.

>diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd

>+<!ENTITY syncToolbarButton.label      "Sync">
>\ No newline at end of file

fix plz

>diff --git a/browser/themes/gnomestripe/browser/browser.css b/browser/themes/gnomestripe/browser/browser.css

>+toolbar:not([iconsize="small"]) .toolbarbutton-1#sync-button > .toolbarbutton-icon {

Why ".toolbarbutton-1" here? Seems redundant. I don't even understand why these rules are needed, though. Shouldn't the styling match other buttons? Dao would be a better reviewer for this part.

>diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css

There's a #sync-status-button reference in here that needs removing.

Comment 9

[Mike Connor [:mconnor]]

(In reply to comment #8)
> >-      if (!this._isLoggedIn()) {
> >-        //XXXzpao When we move the string bundle, we can add more and make this
> >-        //        say "needs setup" or something similar. (bug 583381)
> >-        button.removeAttribute("tooltiptext");
> >-      }
> >+      document.getElementById("sync-button").removeAttribute("status");
> 
> Do you also need to remove tooltiptext here?

The only tooltip is the last sync time, so I don't think we need to remove that.

> >diff --git a/browser/themes/gnomestripe/browser/browser.css b/browser/themes/gnomestripe/browser/browser.css
> 
> >+toolbar:not([iconsize="small"]) .toolbarbutton-1#sync-button > .toolbarbutton-icon {
> 
> Why ".toolbarbutton-1" here? Seems redundant. I don't even understand why these
> rules are needed, though. Shouldn't the styling match other buttons? Dao would
> be a better reviewer for this part.

That's bug 594657

> >diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css
> 
> There's a #sync-status-button reference in here that needs removing.

will do, though this CSS is all placeholder until bug 594657 is fixed.

Comment 10

[Mike Connor [:mconnor]]

Attachment: v2

Comments addressed, keep in mind that the theme stuff is all placeholdery until shorlander has new icons.

Comment 11

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

Comment on attachment 474095 [details] [diff] [review]
v2

>diff --git a/browser/base/content/browser-syncui.js b/browser/base/content/browser-syncui.js

>     if (gBrowser) {
>+      document.getElementById("sync-button").removeAttribute("status");
>+    }
>+    if (needsSetup) {
>+      document.getElementById("sync-button").removeAttribute("tooltiptext");
>     }

Shouldn't this be under the if (gBrowser) check?

(Is it actually possible to return to the "needsSetup" state after a sync has been completed?)

Comment 12

[Mike Connor [:mconnor]]

Attachment: v2 unbitrotted

Comment 13

[Mike Connor [:mconnor]]

Attachment: v2 unbitrotted

the previous attachment brought to you by e, the text editor, and a silly disrespect for existing file encodings!

Comment 14

[Stephen Horlander [:shorlander]]

>#zoom-out-button {
>   list-style-image: url("moz-icon://stock/gtk-zoom-out?size=toolbar");
>-}

The new patch removes the closing bracket on the gnomestripe #zoom-out-button.

Comment 15

[Mike Connor [:mconnor]]

http://hg.mozilla.org/mozilla-central/rev/363c8bae0fb2

merge-fail sucks.  shorlander is adding fix in the patch I'll push for bug 594657

Comment 16

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

Comment on attachment 475521 [details] [diff] [review]
v2 unbitrotted

>diff --git a/browser/base/content/browser-syncui.js b/browser/base/content/browser-syncui.js

>+      document.getElementById("sync-button").removeAttribute("status");


>+    if (needsSetup) {
>+      document.getElementById("sync-button").removeAttribute("tooltiptext");

Oops, missed this - these need null-checks, given that the button is optional an all.

Comment 17

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

Gah, and the entry in browserShared.inc is wrong, and comment 11 wasn't addressed :(

Comment 18

[Philipp von Weitershausen [:philikon]]

(In reply to comment #16)
> Oops, missed this - these need null-checks, given that the button is optional
> an all.

Filed followup bug 596799, feel free to review ;). For some reason I thought dolske had reviewed this bug so I asked him...

Comment 19

[Philipp von Weitershausen [:philikon]]

(In reply to comment #18)
> (In reply to comment #16)
> > Oops, missed this - these need null-checks, given that the button is optional
> > an all.
> 
> Filed followup bug 596799, feel free to review ;). For some reason I thought
> dolske had reviewed this bug so I asked him...

Actually, I just realised you meant something else. Moving over to bug 596799...

Comment 20

[Tony Chung [:tchung]]

Verified fix on  Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre