Sync Icon for the horizontal menu

VERIFIED FIXED in Firefox 29

Status

()

defect
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: mmaslaney, Assigned: jaws)

Tracking

29 Branch
Firefox 30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified, firefox30 verified)

Details

(Whiteboard: [Australis:P3][qa!])

Attachments

(5 attachments, 3 obsolete attachments)

Assets for the default and busy states
Blocks: 905997
Component: Toolbars and Customization → FxA
Whiteboard: [Australis] → [Australis] p=0
Whiteboard: [Australis] p=0 → [Australis] p=0, [qa+]
Posted patch Patch (obsolete) — Splinter Review
Thanks for the icons Michael.

Tim, I've flagged you for review since this touches browser-syncui.js and I wanted to let you know that we'll now be setting the [status="active"] attribute on the #PanelUI-fxa-status button.

Gijs, I followed the same naming scheme for the Windows icons as is being pursued in bug 960517.
Assignee: mmaslaney → jaws
Status: NEW → ASSIGNED
Attachment #8375318 - Flags: review?(ttaubert)
Attachment #8375318 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8375318 [details] [diff] [review]
Patch

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

browser-syncui changes looks fine to me.
Attachment #8375318 - Flags: review?(ttaubert) → review+
Comment on attachment 8375318 [details] [diff] [review]
Patch

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

r- because you're still referencing button even though it's no longer a local variable.

::: browser/base/content/browser-syncui.js
@@ +108,5 @@
> +      let button = document.getElementById(id);
> +      if (!button)
> +        return;
> +      button.removeAttribute("status");
> +    });

Please file a followup bug about how this should interact with customization (if you customize the button away while syncing, it'll forever keep its [status] - but that was a bug before you fixed this).

@@ +114,3 @@
>      this._updateLastSyncTime();
>      if (needsSetup)
>        button.removeAttribute("tooltiptext");

This will break now, please don't do that.
Attachment #8375318 - Flags: review?(gijskruitbosch+bugs) → review-
Posted patch Patch v2 (obsolete) — Splinter Review
Thanks for catching that extra `button` reference.
Attachment #8375318 - Attachment is obsolete: true
Attachment #8375598 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8375598 [details] [diff] [review]
Patch v2

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

::: browser/base/content/browser-syncui.js
@@ +109,5 @@
> +      if (!button)
> +        return;
> +      button.removeAttribute("status");
> +      if (needsSetup)
> +        button.removeAttribute("tooltiptext");

But now we're removing the text from the Panel button as well... is that correct? :-\
Attachment #8375598 - Flags: review?(gijskruitbosch+bugs)
Posted patch Patch v3Splinter Review
There is no tooltiptext applied to the panel button when it needs setup, so it was a no-op. But for defensive programming, if someone added a tooltip there in the future they may not expect it to get removed by this.

Patch adjusted as we discussed on IRC.
Attachment #8375598 - Attachment is obsolete: true
Attachment #8375876 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8375876 [details] [diff] [review]
Patch v3

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

LGTM bar the nit below.

::: browser/base/content/browser-syncui.js
@@ +114,2 @@
>  
> +    if (needsSetup)

Nit: if needsSetup && syncButton
Attachment #8375876 - Flags: review?(gijskruitbosch+bugs) → review+
Posted patch Patch for Aurora (obsolete) — Splinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature for Sync + Australis
User impact if declined: wrong icon on the menu panel
Testing completed (on m-c, etc.): locally, just pushed to fx-team
Risk to taking this patch (and alternatives if risky): none expected
String or IDL/UUID changes made by this patch: none
Attachment #8375892 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a36ff79ca14d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
This doesn't look good on HiDPI OSX.
Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8376295 - Flags: review?(mdeboer)
No longer blocks: fxdesktopbacklog
Whiteboard: [Australis] p=0, [qa+] → [Australis:P3]
Comment on attachment 8375892 [details] [diff] [review]
Patch for Aurora

We should uplift the union of this and the patch I just added (assuming it gets review etc.)
Attachment #8375892 - Flags: approval-mozilla-aurora?
Comment on attachment 8376295 [details] [diff] [review]
968828-fix-sync-icon-osx-hidpi

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

That makes sense ;)
Attachment #8376295 - Flags: review?(mdeboer) → review+
Whiteboard: [Australis:P3] → [Australis:P3][qa+]
Attachment #8375876 - Attachment is obsolete: true
Attachment #8375892 - Attachment is obsolete: true
Comment on attachment 8376579 [details] [diff] [review]
Patch for Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Sync button/Australis
User impact if declined: button doesn't have the correct icon
Testing completed (on m-c, etc.): m-c, local
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none
Attachment #8376579 - Flags: approval-mozilla-aurora?
Attachment #8375876 - Attachment is obsolete: false
Attachment #8376579 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [Australis:P3][qa+] → [Australis:P3][qa+] p=0
No longer blocks: fxdesktopbacklog
Whiteboard: [Australis:P3][qa+] p=0 → [Australis:P3][qa+]
Keywords: verifyme
QA Contact: twalker
QA Contact: twalker
base state looks good with Nightly 20140218. It's a little trickier to catch a sync in progress.
Status: RESOLVED → VERIFIED
QA Contact: twalker
Keywords: verifyme
Whiteboard: [Australis:P3][qa+] → [Australis:P3][qa!]
You need to log in before you can comment on or make changes to this bug.