1233250 - Indicate cached version of site is being displayed

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_cached_notif.png

When we're trying "to be smart" by showing a cached version of a site to our users (cause we have it), we could probably indicate this status through something simple like a snackbar.

copy tbd.

Same padding and as snackbar:
Roboto Regular, white, 14sp
Background: link blue (#0096DD)

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I think this is what we talked about in our convo today

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Anthony Lam (:antlam) from comment #1)
> I think this is what we talked about in our convo today

It is. We also talked about keeping the "snackbar" persistent until the user dismissed it. I'll use a normal snackbar in bug 935190 until we get something more specific in place.

Comment 3

[:Margaret Leibovic]

Is the design attached to this bug what we want to implement?

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Yes! We currently have the standard snackbar for this "loading cached version" situation, and I think it'd be better with the spec in comment 0 :)

Comment 5

[:Grisha Kruglov]

What's the final copy for this? This snackbar currently appears after the page finished loading (once "pageshow" event fires), so the message should take that into account.

Current message: "Loading page from offline storage".

- Showing cached version
- ?

Alternatively, we might consider showing this message when page begins loading, which I feel might be a better experience as page loads might take a while on certain pages (even from cache, it seems).

Comment 6

[:Grisha Kruglov]

From offline conversation, "Showing offline version".

Comment 7

[:Grisha Kruglov]

Attachment: MozReview Request: Bug 1233250: Pre - allow specifying snackbar's backgroundColor via Snackbars.jsm r=sebastian

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

Comment 8

[:Grisha Kruglov]

Attachment: MozReview Request: Bug 1233250: Part 1 - Set offline snackbar background to link_blue r=sebastian

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

Comment 9

[:Grisha Kruglov]

Attachment: MozReview Request: Bug 1233250: Part 2 - Better copy for "loaded from cache" snackbar r=sebastian

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

Comment 10

[:Grisha Kruglov]

Comment on attachment 8749469 [details]
MozReview Request: Bug 1233250: Part 2 - Better copy for "loaded from cache" snackbar r=sebastian

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

Comment 11

[:Grisha Kruglov]

Attachment: Screenshot_2016-05-05_16-54-19.png

How does this look?

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8749470 [details]
Screenshot_2016-05-05_16-54-19.png

nice! looks good!

Comment 13

[:Grisha Kruglov]

Comment on attachment 8749467 [details]
MozReview Request: Bug 1233250: Pre - allow specifying snackbar's backgroundColor via Snackbars.jsm r=sebastian

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

Comment 14

[:Grisha Kruglov]

Comment on attachment 8749468 [details]
MozReview Request: Bug 1233250: Part 1 - Set offline snackbar background to link_blue r=sebastian

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

Comment 15

[:Grisha Kruglov]

Comment on attachment 8749469 [details]
MozReview Request: Bug 1233250: Part 2 - Better copy for "loaded from cache" snackbar r=sebastian

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

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8749467 [details]
MozReview Request: Bug 1233250: Pre - allow specifying snackbar's backgroundColor via Snackbars.jsm r=sebastian

https://reviewboard.mozilla.org/r/50975/#review48043

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:92
(Diff revision 2)
> +            final String providedColor = object.getString("backgroundColor");
> +            try {
> +                backgroundColor = Color.parseColor(providedColor);

Does getString() return null if "backgroundColor" does not exist? Can Color.parseColor() handle null values?

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:94
(Diff revision 2)
> +        Integer backgroundColor = null;
> +
> +        if (object.has("backgroundColor")) {
> +            final String providedColor = object.getString("backgroundColor");
> +            try {
> +                backgroundColor = Color.parseColor(providedColor);

Nice! I didn't know about parseColor().

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:107
(Diff revision 2)
> -        showSnackbarWithAction(activity,
> +        showSnackbarWithActionAndColors(activity,
>                  message,
>                  duration,
>                  action != null ? action.optString("label", null) : null,
> -                new SnackbarHelper.SnackbarEventCallback(callback));
> +                new SnackbarHelper.SnackbarEventCallback(callback),
> +                null, backgroundColor, null);

nit: Let's follow the style here and put every parameter on one line.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8749468 [details]
MozReview Request: Bug 1233250: Part 1 - Set offline snackbar background to link_blue r=sebastian

https://reviewboard.mozilla.org/r/50977/#review48047

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8749469 [details]
MozReview Request: Bug 1233250: Part 2 - Better copy for "loaded from cache" snackbar r=sebastian

https://reviewboard.mozilla.org/r/50979/#review48051

::: mobile/android/locales/en-US/chrome/browser.properties:426
(Diff revision 3)
>  openInApp.pageAction = Open in App
>  openInApp.ok = OK
>  openInApp.cancel = Cancel
>  
>  #Network Offline
> -networkOffline.message = Loading page from offline storage
> +networkOffline.message = Showing offline version

When updating an existing string we add a counter so that the translation tools understand the change:
> networkOffline.message2

Comment 19

[:Grisha Kruglov]

https://reviewboard.mozilla.org/r/50975/#review48043

> Does getString() return null if "backgroundColor" does not exist? Can Color.parseColor() handle null values?

We're checking if `backgroundColor` exists right above. `parseColor` will throw if it can't handle its input, and it's wrapped in try/catch.

Comment 20

[:Grisha Kruglov]

Comment on attachment 8749467 [details]
MozReview Request: Bug 1233250: Pre - allow specifying snackbar's backgroundColor via Snackbars.jsm r=sebastian

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

Comment 21

[:Grisha Kruglov]

Comment on attachment 8749468 [details]
MozReview Request: Bug 1233250: Part 1 - Set offline snackbar background to link_blue r=sebastian

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

Comment 22

[:Grisha Kruglov]

Comment on attachment 8749469 [details]
MozReview Request: Bug 1233250: Part 2 - Better copy for "loaded from cache" snackbar r=sebastian

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

Comment 23

[:Grisha Kruglov]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad28ba459bac

Comment 24

[:Grisha Kruglov]

https://hg.mozilla.org/integration/fx-team/rev/ebb9291ca4eabeb23d2625ba3e1bfb2f057f5e75
Bug 1233250: Pre - allow specifying snackbar's backgroundColor via Snackbars.jsm r=sebastian

Comment 25

[:Grisha Kruglov]

https://hg.mozilla.org/integration/fx-team/rev/04e8183848e9312e121b1645e58390f58f5549bb
Bug 1233250: Part 1 - Set offline snackbar background to link_blue r=sebastian

Comment 26

[:Grisha Kruglov]

https://hg.mozilla.org/integration/fx-team/rev/91714307a799b978b0a94911cd99ee75a1de0ac5
Bug 1233250: Part 2 - Better copy for "loaded from cache" snackbar r=sebastian

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Grisha Kruglov from comment #26)
> https://hg.mozilla.org/integration/fx-team/rev/
> 91714307a799b978b0a94911cd99ee75a1de0ac5
> Bug 1233250: Part 2 - Better copy for "loaded from cache" snackbar
> r=sebastian

You changed the string label here but not in browser.js:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7235

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ebb9291ca4ea
https://hg.mozilla.org/mozilla-central/rev/04e8183848e9
https://hg.mozilla.org/mozilla-central/rev/91714307a799

Comment 29

[:Grisha Kruglov]

https://hg.mozilla.org/integration/fx-team/rev/6b46587c1650471c6c112374f8f230b8d50929a9
Bug 1233250: use correct string name for offline message r=me

Comment 30

[:Grisha Kruglov]

(In reply to Sebastian Kaspari (:sebastian) from comment #27)
> (In reply to :Grisha Kruglov from comment #26)
> > https://hg.mozilla.org/integration/fx-team/rev/
> > 91714307a799b978b0a94911cd99ee75a1de0ac5
> > Bug 1233250: Part 2 - Better copy for "loaded from cache" snackbar
> > r=sebastian
> 
> You changed the string label here but not in browser.js:
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#7235

Bah! Thanks for the catch!

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6b46587c1650

Comment 32

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot from 2016-05-13 10:55:34.png

Tested using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 49.0a1 (2016-05-12)