1417520 - Replace sync illustration in panel for unverified user

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Ryan Feeley [:rfeeley]]

Attachment: sync-illustration-broken.svg

We now have a specific (broken hearted) illustration for the Synced Tabs edge cases.

Please replace the illustration in the following states with the SVG. The size is identical to the other (new) illustration.

1. Before verifying
2. Before connecting second device
3. If tab syncing is disabled

See URL for context.

Comment 2

[Edouard Oger [:eoger]]

Attachment: Bug 1417520 p1 - Show message to unverified users in synced tabs sidebar.

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

Comment 3

[Edouard Oger [:eoger]]

Attachment: Bug 1417520 p2 - Introduce sync-illustration-issues.

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

Comment 4

[Edouard Oger [:eoger]]

The p2 overlaps with bug 1417202 (mostly CSS).

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8929151 [details]
Bug 1417520 p1 - Show message to unverified users in synced tabs sidebar.

https://reviewboard.mozilla.org/r/200442/#review205792

::: browser/components/syncedtabs/SyncedTabsDeckComponent.js:119
(Diff revision 1)
>      }
>    },
>  
>    // There's no good way to mock fxAccounts in browser tests where it's already
>    // been instantiated, so we have this method for stubbing.
> -  _accountStatus() {
> +  _getSignedInUser() {

There's a subtle behaviour change here - if the account has been deleted on the server and we haven't synced yet, then getSignedInUser will return data while accountStatus would have returned false.

OTOH though, it's not really a big deal as the rest of the UI doesn't make this distinction, so I guess it's fine and just mention it to demonstrate I was paying attention :)

::: browser/modules/test/browser/browser_BrowserUITelemetry_syncedtabs.js:95
(Diff revision 1)
>  
>    await SidebarUI.show("viewTabsSidebar");
>  
>    let syncedTabsDeckComponent = SidebarUI.browser.contentWindow.syncedTabsDeckComponent;
>  
> -  syncedTabsDeckComponent._accountStatus = () => Promise.resolve(true);
> +  syncedTabsDeckComponent._getSignedInUser = () => Promise.resolve({verified: true});

we should get at least one test somewhere that has verified: false

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8929152 [details]
Bug 1417520 p2 - Introduce sync-illustration-issues.

https://reviewboard.mozilla.org/r/200444/#review205796

The svg needs some love and I think broken should change (although I'm actually missing where that's set?). I also think you should request review from dao on part2 (and in general, probably anything that's non-trivial CSS changes should have someone on the front-end team look over them)

::: browser/components/customizableui/content/panelUI.inc.xul:437
(Diff revision 1)
>              </vbox>
>              <!-- Sync is ready to Sync but the "tabs" engine isn't enabled-->
>              <hbox id="PanelUI-remotetabs-tabsdisabledpane" pack="center" flex="1">
>                <vbox class="PanelUI-remotetabs-instruction-box" align="center">
>                  <hbox pack="center">
> -                  <image class="fxaSyncIllustration"/>
> +                  <image class="fxaSyncIllustration broken"/>

I don't think "broken" is a good choice here (and might be confused with moz-broken) - something more descriptive that reflects the fact nothing's actually broken, but just that the user hasn't verified or similar. Ditto in the filename.

::: browser/themes/shared/fxa/sync-illustration-broken.svg:1
(Diff revision 1)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

This SVG doesn't comply with a few things in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines

Comment 7

[Ryan Feeley [:rfeeley]]

I also did not run it through an optimizer.

Comment 8

[Edouard Oger [:eoger]]

Attachment: Bug 1417520 p3 - Remove No Synced tabs yet title.

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

Comment 9

[Edouard Oger [:eoger]]

Comment on attachment 8929151 [details]
Bug 1417520 p1 - Show message to unverified users in synced tabs sidebar.

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

Comment 10

[Edouard Oger [:eoger]]

Comment on attachment 8929152 [details]
Bug 1417520 p2 - Introduce sync-illustration-issues.

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

Comment 11

[Edouard Oger [:eoger]]

Thanks Mark, I amended my patch with your comments and have added a 3rd commit at rfeeley's request.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8929152 [details]
Bug 1417520 p2 - Introduce sync-illustration-issues.

https://reviewboard.mozilla.org/r/200444/#review206868

::: browser/components/customizableui/content/panelUI.inc.xul:437
(Diff revision 2)
>              </vbox>
>              <!-- Sync is ready to Sync but the "tabs" engine isn't enabled-->
>              <hbox id="PanelUI-remotetabs-tabsdisabledpane" pack="center" flex="1">
>                <vbox class="PanelUI-remotetabs-instruction-box" align="center">
>                  <hbox pack="center">
> -                  <image class="fxaSyncIllustration"/>
> +                  <image class="fxaSyncIllustration issue"/>

Please find a more distinct class name. panelUI.inc.xul is included in browser.xul, so you're sharing this "namespace" with loads of other code.

Comment 13

[Edouard Oger [:eoger]]

Comment on attachment 8929151 [details]
Bug 1417520 p1 - Show message to unverified users in synced tabs sidebar.

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

Comment 14

[Edouard Oger [:eoger]]

Comment on attachment 8929152 [details]
Bug 1417520 p2 - Introduce sync-illustration-issues.

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

Comment 15

[Edouard Oger [:eoger]]

Comment on attachment 8929587 [details]
Bug 1417520 p3 - Remove No Synced tabs yet title.

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

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8929152 [details]
Bug 1417520 p2 - Introduce sync-illustration-issues.

https://reviewboard.mozilla.org/r/200444/#review208754

::: browser/themes/shared/customizableui/panelUI.inc.css:706
(Diff revision 3)
>    list-style-image: url(chrome://browser/skin/fxa/sync-illustration.svg);
>    -moz-context-properties: fill;
>    fill: #cdcdcd;
>  }
>  
> +.fxaSyncIllustration.fxaSyncIllustrationIssue {

I'd have thought it a little cleaner to have all references to fxaSyncIllustrationIssue use *just* fxaSyncIllustrationIssue and adjust the CSS - ie, something like:
`
<image class="fxaSyncIllustrationIssue"/>

and:
.fxaSyncIllustration,
.fxaSyncIllustrationIssue {
 /* common stuff, but no list-tyle-image
}

.fxaSyncIllustration {
  list-style-image: ...
}

.fxaSyncIllustrationIssue {
  list-style-image: ...
}
`
(and ditto for the ones without the leading fxa)
however, Dao's opinion is the one you care about :)

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8929587 [details]
Bug 1417520 p3 - Remove No Synced tabs yet title.

https://reviewboard.mozilla.org/r/200856/#review208756

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8929152 [details]
Bug 1417520 p2 - Introduce sync-illustration-issues.

https://reviewboard.mozilla.org/r/200444/#review209012

::: browser/themes/shared/customizableui/panelUI.inc.css:706
(Diff revision 3)
>    list-style-image: url(chrome://browser/skin/fxa/sync-illustration.svg);
>    -moz-context-properties: fill;
>    fill: #cdcdcd;
>  }
>  
> +.fxaSyncIllustration.fxaSyncIllustrationIssue {

Whether you follow Mark's suggestion or not, you can remove .fxaSyncIllustration from this selector.

Comment 20

[Edouard Oger [:eoger]]

Comment on attachment 8929151 [details]
Bug 1417520 p1 - Show message to unverified users in synced tabs sidebar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200442/diff/3-4/

Comment 21

[Edouard Oger [:eoger]]

Comment on attachment 8929152 [details]
Bug 1417520 p2 - Introduce sync-illustration-issues.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200444/diff/3-4/

Comment 22

[Edouard Oger [:eoger]]

Comment on attachment 8929587 [details]
Bug 1417520 p3 - Remove No Synced tabs yet title.

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e009ac67e88
p1 - Show message to unverified users in synced tabs sidebar. r=markh
https://hg.mozilla.org/integration/autoland/rev/536dc372395c
p2 - Introduce sync-illustration-issues. r=dao,markh
https://hg.mozilla.org/integration/autoland/rev/8696159a170c
p3 - Remove No Synced tabs yet title. r=markh

Comment 24

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/3e009ac67e88
https://hg.mozilla.org/mozilla-central/rev/536dc372395c
https://hg.mozilla.org/mozilla-central/rev/8696159a170c