1354084 - Add 'Firefox Account' button to static hamburger menu

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[Mike de Boer [:mikedeboer]]

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.)

Comment 2

[:Gijs (out for now; he/him)]

Attachment: Bug 1354084 - add sync / Firefox account button to hamburger panel,

Review commit: https://reviewboard.mozilla.org/r/141076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141076/

Comment 3

[:Gijs (out for now; he/him)]

(I haven't added in keyboard support yet, but this should otherwise be ready for review, I hope...)

Comment 4

[:Gijs (out for now; he/him)]

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.

Comment 5

[Edouard Oger [:eoger]]

Works (and looks) great 

Comment 6

[Mike de Boer [:mikedeboer]]

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.

Comment 7

[:Gijs (out for now; he/him)]

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.

Comment 8

[:Gijs (out for now; he/him)]

Comment on attachment 8869444 [details]
Bug 1354084 - add sync / Firefox account button to hamburger panel,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141076/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7575b0dd085
add sync / Firefox account button to hamburger panel, r=mikedeboer

Comment 10

[:Gijs (out for now; he/him)]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e7575b0dd085

Comment 12

[u554753]

Verified on Windows, Mac, and Ubuntu.