1021416 - [Rocketbar] Rocketbar does not handle offline well

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Search, defect)

Description

[Valentin Tsatskin [:vt]]

# Build Information

Device: Flame
Gaia      d2cfef555dabab415085e548ed44c48a99be5c32
Gecko     https://hg.mozilla.org/mozilla-central/rev/51b428be6213
BuildID   20140605040202
Version   32.0a1
ro.build.version.incremental=94
ro.build.date=Tue May 20 09:29:20 CST 2014

# Description

When there is no data connection on your device, the rocketbar does not give any indication that results won't load. In fact, it won't really do anything.

# Steps to Reproduce

1) Enable Airplane Mode
2) Type a search term such as "this is a test"

# Expected Results

Search results somehow indicate you don't have an internet connection so results won't work. The results should at least include results from your phone considering we a going for an "offline first" strategy. 

# Actual Results

Nothing appears.

Comment 1

[bhavana bajaj [:bajaj]]

It could be better to have a message but not something I would hold a release for. :kgrandon, this is a good bug to pick-up,f lagging it so it is prioritized for future release.

Comment 2

[Kevin Grandon :kgrandon]

Yeah, I think we should probably show a message here if there's no results (your device may be offline, but you might surface locally isntalled apps). The current rocketbar is a bastardization of the homescreen and future rocketbar, so I'm not sure if this is in the UX spec or not. May need product input.

Comment 3

[Stephany Wilkes]

This is a regression. Added it in the keyword field.

Comment 4

[Kevin Grandon :kgrandon]

Not a regression, we never implemented this. Stephany - can you get us specs for this before we nominate as a blocker?

Comment 5

[Stephany Wilkes]

I am in a UX audit with Francis and Valentin and we discussed it as a regression. Ni? Francis for detail.

Comment 6

[Francis Djabri [:djabber]]

Attachment: spinner for loading results.png

Screenshot from 1.4 showing spinner for loading results

Comment 7

[Francis Djabri [:djabber]]

Attachment: Text showing offline error message

Comment 8

[Francis Djabri [:djabber]]

Added a couple of screenshots showing existing offline/slow connectivity behavior from 1.4

Comment 9

[Valentin Tsatskin [:vt]]

Attachment: visual-style-for-2.0.png

The margins on the error text are non-existent. I've mocked up how the text box should look in the attachment.

The blue vertical lines are the guides for the edges of the error text box. These guides should not be included.

The red thick lines indicate the vertical positioning of the error text box. The message should be vertically centred.

Francis, Eric are you okay with this?

Comment 10

[Francis Djabri [:djabber]]

Thanks Val, looks good to me. What do you think, Eric?

Comment 11

[Eric Pang [:epang] UX]

(In reply to Francis Djabri [:djabber] from comment #10)
> Thanks Val, looks good to me. What do you think, Eric?

Looks good to me too, thanks for doing this Val!

Comment 12

[Candice Serran (:cserran)]

marking as blocker (confirmed by francis)

Comment 13

[Kevin Grandon :kgrandon]

Francis, Peter - Can we have some more spec work done here please? My main question are:

1 - Should we show the offline message if the user types in something that would surface local results (installed apps)?

2 - Should we save the state? Dale is currently doing some work to preserve the state when the user goes to homescreen and back. What would the user see if they are offline, enter a query and see the offline message, exit homescreen, "get online", then re-enter search?

Comment 14

[Francis Djabri [:djabber]]

See my comments below - I'll update the spec and will repost it below:

> 1 - Should we show the offline message if the user types in something that
> would surface local results (installed apps)?
> 
I think no - the message should only be shown if there are no results. 

> 2 - Should we save the state? Dale is currently doing some work to preserve
> the state when the user goes to homescreen and back. What would the user see
> if they are offline, enter a query and see the offline message, exit
> homescreen, "get online", then re-enter search?

It depends on how they exit the search app - if they exit by tapping Close, the search field should be cleared and the next time the user returns to the search app, it should be in the empty state. If they exit by pressing home, then the next time they enter the search app, it should retain the previous search term.

Comment 15

[Kevin Grandon :kgrandon]

(In reply to Francis Djabri [:djabber] from comment #14)
> See my comments below - I'll update the spec and will repost it below:
> It depends on how they exit the search app - if they exit by tapping Close,
> the search field should be cleared and the next time the user returns to the
> search app, it should be in the empty state. If they exit by pressing home,
> then the next time they enter the search app, it should retain the previous
> search term.

Sure, but if they were offline, do we retain the offline messaging, or should we try to refresh the display with updated results?

Comment 17

[Eric Pang [:epang] UX]

Hey Kevin, just wanted to let you know that I made a quick update to the visual styling Valentin posted while adding to the specs.  Pretty much the same, just made the text italic :).  Thx

Spec can be found here:
https://mozilla.box.com/s/ho1brfbe9f4sw5uv3ga4

Comment 18

[Kevin Grandon :kgrandon]

Attachment: search_results_spec.jpg

Ok, making it more clear by attaching that spec and obsoleting the others.

Comment 19

[Francis Djabri [:djabber]]

(In reply to Kevin Grandon :kgrandon from comment #15)
> (In reply to Francis Djabri [:djabber] from comment #14)
> > See my comments below - I'll update the spec and will repost it below:
> > It depends on how they exit the search app - if they exit by tapping Close,
> > the search field should be cleared and the next time the user returns to the
> > search app, it should be in the empty state. If they exit by pressing home,
> > then the next time they enter the search app, it should retain the previous
> > search term.
> 
> Sure, but if they were offline, do we retain the offline messaging, or
> should we try to refresh the display with updated results?

We should try to refresh the display with updated results.

Comment 20

[Kevin Grandon :kgrandon]

Bhavana - we already have this string in other apps, but need to add it to a new app for this bug. Can you check if there is concern from an L10N standpoint if we take this?

Comment 21

[Kevin Grandon :kgrandon]

I'll take this. Hope we can uplift given the l10n issue.

Comment 22

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Comment 23

[Kevin Grandon :kgrandon]

Comment on attachment 8445569 [details]
Github pull request

Pre-mature enter press. This is a work in progress.

Comment 24

[Kevin Grandon :kgrandon]

Comment on attachment 8445569 [details]
Github pull request

Just looking for a review here if either of you have time. Thanks.

Comment 25

[Ran Ben Aharon [:ranbena] EverythingMe]

Seem like bugs:
1 The message appears when providers are still fetching and then disappears when they return results.
2. The offline message should be center aligned.
3. "Internet" should be capitalized according to the spec.

One reservation regarding behavior:
Showing the message only if there are no results seems wrong to me. IMO it should always be present when offline (as web results, marketplace suggestions and autocomplete suggestions are missing) under the currently displayed results.

Comment 26

[Ran Ben Aharon [:ranbena] EverythingMe]

Comment on attachment 8445569 [details]
Github pull request

Uhhh.. updating to "-" is so harsh..

Comment 27

[Kevin Grandon :kgrandon]

(In reply to Ran Ben Aharon (Everything.me) from comment #25)
> Seem like bugs:
> 1 The message appears when providers are still fetching and then disappears
> when they return results.
> 2. The offline message should be center aligned.

Will fix those problems, thanks.

> 3. "Internet" should be capitalized according to the spec.

I think it's import we stay consistent, so I followed this example: https://github.com/mozilla-b2g/gaia/blob/master/apps/collection/locales/collection.en-US.properties#L11


> One reservation regarding behavior:
> Showing the message only if there are no results seems wrong to me. IMO it
> should always be present when offline (as web results, marketplace
> suggestions and autocomplete suggestions are missing) under the currently
> displayed results.

According to Francis in comment 19 this is the desired behavior, as you may be searching for an app (I do all the time). I would definitely not want to hinder this use case, but perhaps we could show local results *and* the message if the user is offline. I would consider that more of an enhancement after this has landed though.


(In reply to Ran Ben Aharon (Everything.me) from comment #26)
> Comment on attachment 8445569 [details]
> Github pull request
> 
> Uhhh.. updating to "-" is so harsh..

No worries, I can provide plenty of R- to you in the future ;-)

Comment 28

[Ran Ben Aharon [:ranbena] EverythingMe]

(In reply to Kevin Grandon :kgrandon from comment #27)
> ...but perhaps we could show local results *and* the
> message if the user is offline. I would consider that more of an enhancement
> after this has landed though.

That's exactly what I meant.
 
> No worries, I can provide plenty of R- to you in the future ;-)

:D

Comment 29

[Ran Ben Aharon [:ranbena] EverythingMe]

One more verification:
Would the offline message appear if online but no results returned from any provider?

Comment 30

[Kevin Grandon :kgrandon]

(In reply to Ran Ben Aharon (Everything.me) from comment #29)
> One more verification:
> Would the offline message appear if online but no results returned from any
> provider?

Currently, yes.. but we can solve that with a quick isOnline check. Currently WebResults *always* return something when online though, right?

Comment 31

[Ran Ben Aharon [:ranbena] EverythingMe]

Yes, but it might change soon (which is what we talked over with Peter earlier today)

Comment 32

[Kevin Grandon :kgrandon]

Comment on attachment 8445569 [details]
Github pull request

Addressed the centering and notice appearance issues. I'm not sure if we should address the other review comments in this PR or not, I'm included to do them in follow-ups if they are needed.

Comment 33

[Dale Harvey (:daleharvey)]

Comment on attachment 8445569 [details]
Github pull request

I left one nit about the case when the use comes online, I think it should either redo the search or do nothing and wait for the next search, thats fairly minor, 

however I think there is one thing that should be fixed that this looks to have regressed, with the original collect function we returned immediately and cleared the previous results before showing the new ones, however now we wait for the promise to resolve so if you do a search, press the clear button, everything hides, start a search and you will see the previous results / offline message until the search completes.

Comment 34

[Kevin Grandon :kgrandon]

Comment on attachment 8445569 [details]
Github pull request

Hey Dale - I changed it so that the offline message is hidden when we call clear(). I was not able to reproduce the problem where previous results appear because we should still be calling clear() before we do a search. Are you able to reproduce that, or do you have a screenshot by chance?

Is there anything you'd like to address here? Thanks for the quick feedback :)

https://github.com/KevinGrandon/gaia/blob/bug_1021416_search_offline/apps/search/js/search.js#L145

Comment 35

[Dale Harvey (:daleharvey)]

Comment on attachment 8445569 [details]
Github pull request

I was assuming or thinking of another bug when it came to the results, clear on the offline works well here.

Comment 36

[Kevin Grandon :kgrandon]

Thanks! Landed: https://github.com/mozilla-b2g/gaia/commit/a82d0004af1e2e58aea2f6fba293d047c521053f

Comment 37

[Kevin Grandon :kgrandon]

Francesco - this just landed with late strings and will be uplifted to 2.0. The string does match the exact format and context of another string, but the variable name is changed for clarity. Can you tell me if there is anything I can do here to make this easier on the process/localizers?

For reference, here is the string in another app: https://github.com/mozilla-b2g/gaia/blob/master/apps/collection/locales/collection.en-US.properties#L11

Comment 38

[Francesco Lodolo [:flod]]

We're way past string freeze, so the question is not for me, but for release-drivers (I see bbajaj already in NI).

Please don't land this without explicit approval from release drivers: I know this bug is flagged as 2.0+, but this was 10 days ago, way before string freeze.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

;)

Comment 40

[Kevin Grandon :kgrandon]

Comment on attachment 8445569 [details]
Github pull request

Not really sure what the next steps are here then. Request approval perhaps?

There is a new string in this app, but it should not need to be translated because we already have it. It should be possible to migrate with a script, but I'm not really familiar with our l10n processes.

I'm more than happy to help out with that.

Comment 41

[Francesco Lodolo [:flod]]

(In reply to Kevin Grandon :kgrandon from comment #40)
> There is a new string in this app, but it should not need to be translated
> because we already have it. It should be possible to migrate with a script,
> but I'm not really familiar with our l10n processes.

We *don't* have it: it's a new string id, it doesn't matter if it's almost identical to another string.
We don't script changes to l10n repositories, they're managed by localizers, sometimes directly on Hg, sometimes by using external tools.

Comment 42

[Kevin Grandon :kgrandon]

(In reply to Francesco Lodolo [:flod] from comment #41)
> We *don't* have it: it's a new string id, it doesn't matter if it's almost
> identical to another string.
> We don't script changes to l10n repositories, they're managed by localizers,
> sometimes directly on Hg, sometimes by using external tools.

If we use the exact same key/content as in another app does that help at all?

Comment 43

[Francesco Lodolo [:flod]]

(In reply to Kevin Grandon :kgrandon from comment #42)
> If we use the exact same key/content as in another app does that help at all?

No, unless you're able to load only that string at run-time, and the context in which the string is used (font, space, etc.) is exactly the same.

Comment 44

[bhavana bajaj [:bajaj]]

(In reply to Kevin Grandon :kgrandon from comment #40)
> Comment on attachment 8445569 [details]
> Github pull request
> 
> Not really sure what the next steps are here then. Request approval perhaps?
> 
> There is a new string in this app, but it should not need to be translated
> because we already have it. It should be possible to migrate with a script,
> but I'm not really familiar with our l10n processes.
> 
> I'm more than happy to help out with that.

My analysis on blocking decision holds true per comment #1. I think this is something we can live with in 2.0 and is not a release show stopper. I am not sure why :cserran/francis blocked on this. As :flod highlights this is breaking the string freeze so unless there are other alternatives I think we should leave this to getting resolved in 2.1

Comment 45

[Kevin Grandon :kgrandon]

Ok, then re-noming based on comment 44.

Comment 46

[Delphine Lebédel [:delphine - Use needinfo]]

I'm not sure I understand here. As per comment 44, this should clearly not block. 
Un-nominating once more

Comment 47

[Jason Smith [:jsmith]]

Stephany, myself, Francis, and Kevin discussed this in IRC. The conclusion is that this should remain as a blocker for the release for the following reasons:

Friendly offline experience is considered a critical use case in our target markets. We're taking potentially a bigger risk not taking this, as we later could get burned if a partner tries to cert block on us later for this issue (historically, we have had certification blockers around friendly offline experience). There's a request one string ID here, which does place a risk on the localization schedule in the release. However, the severity of the localization risk will be far worse if this blocks certification later down the line, so I think it's better to take the hit now.

Note that Kevin has indicated that he's willing to help the localization team with writing a script to port existing translations of the string in question here to the required location to link to this string ID, so feel free to take advantage of the help being offered here.

Comment 48

[Kevin Grandon :kgrandon]

Axel/l10n - Can you give me some pointers of where to look for this one? It would just take me an hour or so to write a script that copies the string from the collection app locales, renames the key, and inserts it into the search app files.

If you give me some links/documentation to the collection app locale files, I'd have no problem getting you a patch which will have translations for the copied string. We can do this before uplift to be safe.

Comment 49

[Axel Hecht [:Pike]]

As much as I appreciate the intent, trying to create scripts doesn't match reality. We have multiple different contributions paths to l10n and many of those just don't give shit about what happens on the repo they're working on. Some teams work on https://mozilla.locamotion.org/, which doesn't take anything other than stuff that's coming through the web ui. Then there's the Polish guys which have a rant with flod about someone touching their repo with trivial stuff. And then there's a marginal amount of locales for which just landing would be helpful. But that's really just a marginal amount, sadly.

The only way to work around this is by build magic, and I don't have a starting point for that.

Comment 50

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Bhavana, this still remains a blocker.  See comment 47.  Can we get uplift approval please?  If not please discuss with the people in comment 47.

Comment 51

[bhavana bajaj [:bajaj]]

Comment on attachment 8445569 [details]
Github pull request

Approving, we had an offline discussion with pike a few minutes ago and given this could be a cert issue approving this. Also pike mentioned this could end-up being shown as english only, we should be fine to live with that given how/when the user hits this case.

Comment 52

[Kevin Grandon :kgrandon]

2.0: https://github.com/mozilla-b2g/gaia/commit/457921c80499ee8f4958aea0b5cfaa2bc824edd3

Comment 53

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Follow up bug 1033041 and bug 1033049

Verified :
2.0, 2.1 flame
Gaia      8fb5e2a9ad1025ee7d247b90af7499766afadd28
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/5da69a493324
BuildID   20140701000201
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
B1TC00011220
Flame