Add 'Firefox Account' button to static hamburger menu

VERIFIED FIXED in Firefox 55

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
Last month

People

(Reporter: mikedeboer, Assigned: Gijs)

Tracking

(Blocks 2 bugs)

Trunk
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [photon-structure], )

Attachments

(1 attachment)

This button will look and behave the same as the Account button in the current menu panel.

The icon/ gravatar picture will be smaller; the size to be the same as all other menu item icons.

The caption will no longer be the name of the user who signed in, but will become static: 'Firefox Account'.

The associated command will stay the same as it currently is.

Please take a possible future hotkey assignment into account. (no pun intended.)
Blocks: 1354087
No longer blocks: 1354087
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon] → [photon-structure]
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
Depends on: 1358167
Duplicate of this bug: 1354087
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 55.5 - May 15
Priority: P2 → P1
Iteration: 55.5 - May 15 → 55.6 - May 29
(I haven't added in keyboard support yet, but this should otherwise be ready for review, I hope...)
Bryan, can you provide final normal/hover/active colours for the sync 'warning' states?


I'm purposefully keeping the current iconography, with the assumption that we'll bulk-replace things in bug 1355455.
Flags: needinfo?(bbell)
Works (and looks) great 
Comment on attachment 8869444 [details]
Bug 1354084 - add sync / Firefox account button to hamburger panel,

https://reviewboard.mozilla.org/r/141076/#review145056

Nice work! It looks really good in the new menu. I have a few comments here and there, but nothing that requires another round. Thanks!

::: browser/base/content/browser-sync.js:52
(Diff revision 1)
>    get remoteClients() {
>      return Weave.Service.clientsEngine.remoteClients
>             .sort((a, b) => a.name.localeCompare(b.name));
>    },
>  
> +  _generateGetters(usePhoton) {

nit: `_generateNodeGetters` might be a bit more descriptive.
Also, I think we wanna keep this even when we clean up the non-photon stuff later, by moving it to the `init()` method.

::: browser/base/content/browser-sync.js:66
(Diff revision 1)
> +      });
> +    }
> +  },
> +
> +  _maybeUpdateUIState() {
> +    // Update the UI

nit: missing dot.

::: browser/base/content/browser-sync.js:70
(Diff revision 1)
> +  _maybeUpdateUIState() {
> +    // Update the UI
> +    if (UIState.isReady()) {
> +      const state = UIState.get();
> +      // If we are not configured, the UI is already in the right state when
> +      // we open the window. We can avoid a repaint.

Well, you're probably right in this case :/ Normally one would expect that setting the state with its current value(s) does not cause a repaint. Oh well!

::: browser/base/content/browser-sync.js:94
(Diff revision 1)
> +    XPCOMUtils.defineLazyPreferenceGetter(this, "gPhotonStructure",
> +      "browser.photon.structure.enabled", (pref, old, newValue) => {
> +      this._generateGetters(newValue);
> +      this._maybeUpdateUIState();
> +    });
> +    this._generateGetters(this.gPhotonStructure);

Since you're not using the 'lazy' part of `defineLazyPreferenceGetter`, but because of its observer utilirty, can you tell us about that above its declaration in a comment?

::: browser/themes/shared/customizableui/panelUI.inc.css:717
(Diff revision 1)
> -
>  .panel-banner-item > .toolbarbutton-text {
>    width: 0; /* Fancy cropping solution for flexbox. */
>  }
>  
> +/* fxa indicator bits */

nit: missing dot.

::: browser/themes/shared/customizableui/panelUI.inc.css:744
(Diff revision 1)
> +
> +#appMenu-fxa-avatar,
> +#appMenu-fxa-label > .toolbarbutton-icon,
> +#appMenu-fxa-icon > .toolbarbutton-icon {
> +  width: 16px;
> +  height: 16px;

Please add these selectors to `#PanelUI-remotetabs-tabslist > toolbarbutton[itemtype="tab"] > .toolbarbutton-icon,` and friends below.
You might want to move `#PanelUI-containersItems > .subviewbutton > .toolbarbutton-icon` on line 1682 as well.

::: browser/themes/shared/customizableui/panelUI.inc.css:752
(Diff revision 1)
> +#appMenu-fxa-icon > .toolbarbutton-text {
> +  display: none;
> +}
> +
> +#appMenu-fxa-icon[syncstatus="active"] {
> +  list-style-image: url(chrome://browser/skin/syncProgress-horizontalbar.png);

I think we'll want to make this SVG using CSS anims too, eventually. Certainly because it looks like a rotate transform with different fill color! Can you file a follow-up for that?

::: browser/themes/shared/customizableui/panelUI.inc.css:760
(Diff revision 1)
> +#appMenu-fxa-avatar {
> +  pointer-events: none;
> +  list-style-image: url(chrome://browser/skin/fxa/default-avatar.svg);
> +}
> +
> +@media (min-resolution: 1.1dppx) {

...because I'm sooo happy to see less and less of these blocks :)

::: browser/themes/shared/customizableui/panelUI.inc.css:800
(Diff revision 1)
> +}
> +
> +#appMenu-fxa-container[fxastatus="signedin"] > toolbarseparator {
> +  -moz-appearance: none;
> +  height: 24px;
> +  margin: 0; 

nit: trailing space.

::: browser/themes/shared/customizableui/panelUI.inc.css:830
(Diff revision 1)
> +}
> +
> +#appMenu-fxa-container[fxastatus="login-failed"] > #appMenu-fxa-status:hover:active,
> +#appMenu-fxa-container[fxastatus="unverified"] > #appMenu-fxa-status:hover:active {
> +  background-color: hsl(56, 96%, 60%);
> +  box-shadow: 0 1px 0 hsla(210, 4%, 10%, .05) inset;

Can we put these state color values in CSS variables, please? I think that'd make things a bit easier when Bryan decides to change them later on.
Attachment #8869444 - Flags: review?(mdeboer) → review+
Blocks: 1366844
Comment on attachment 8869444 [details]
Bug 1354084 - add sync / Firefox account button to hamburger panel,

https://reviewboard.mozilla.org/r/141076/#review145238

::: browser/base/content/browser-sync.js:70
(Diff revision 1)
> +  _maybeUpdateUIState() {
> +    // Update the UI
> +    if (UIState.isReady()) {
> +      const state = UIState.get();
> +      // If we are not configured, the UI is already in the right state when
> +      // we open the window. We can avoid a repaint.

FWIW, this was pre-existing and moved out from the init() code. I do know that re-setting the same (XUL) attribute does count as e.g. a mutation for a mutationobserver, and I wouldn't be at all surprised if it dirtied layout state.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e7575b0dd085
add sync / Firefox account button to hamburger panel, r=mikedeboer
I ended up landing this as-is but reusing the add-on banner colours. I expect it still needs looking at, but we can do that in a separate bug if necessary. I used variables as requested so it should be easy to do. Leaving ni to check.
https://hg.mozilla.org/mozilla-central/rev/e7575b0dd085
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1367409
Flags: needinfo?(bbell)
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: 1387512
Depends on: 1429288
Blocks: 1560398
No longer blocks: 1560398
You need to log in before you can comment on or make changes to this bug.