Closed Bug 1021416 Opened 5 years ago Closed 5 years ago

[Rocketbar] Rocketbar does not handle offline well

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.0 S5 (4july)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: vtsatskin, Assigned: kgrandon)

References

Details

(Keywords: late-l10n, Whiteboard: [2.0-FL-bug-bash][systemsfe])

User Story

Visual Spec:
https://mozilla.box.com/s/ho1brfbe9f4sw5uv3ga4

Attachments

(2 files, 3 obsolete files)

# 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.
User Story: (updated)
blocking-b2g: --- → 2.0?
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.
Flags: needinfo?(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.
Flags: needinfo?(kgrandon)
blocking-b2g: 2.0? → backlog
Component: Gaia → Gaia::Search
No longer blocks: rocketbar-search-mvp
This is a regression. Added it in the keyword field.
blocking-b2g: backlog → 2.0?
Keywords: regression
Not a regression, we never implemented this. Stephany - can you get us specs for this before we nominate as a blocker?
blocking-b2g: 2.0? → ---
Flags: needinfo?(swilkes)
Keywords: regression
I am in a UX audit with Francis and Valentin and we discussed it as a regression. Ni? Francis for detail.
Flags: needinfo?(swilkes) → needinfo?(fdjabri)
Attached image spinner for loading results.png (obsolete) —
Screenshot from 1.4 showing spinner for loading results
Flags: needinfo?(fdjabri)
Added a couple of screenshots showing existing offline/slow connectivity behavior from 1.4
Attached image visual-style-for-2.0.png (obsolete) —
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?
Flags: needinfo?(fdjabri)
Flags: needinfo?(epang)
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking-]
Thanks Val, looks good to me. What do you think, Eric?
Flags: needinfo?(fdjabri)
(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!
Flags: needinfo?(epang)
marking as blocker (confirmed by francis)
blocking-b2g: --- → 2.0+
Whiteboard: [2.0-FL-bug-bash] → [2.0-FL-bug-bash][systemsfe]
Blocks: vertical-homescreen
No longer blocks: vertical-home-next
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking-] → [VH-FL-blocking-][VH-FC-blocking+]
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?
Flags: needinfo?(pdolanjski)
Flags: needinfo?(fdjabri)
Blocks: 1015336
No longer blocks: vertical-homescreen
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.
(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?
Summary: Rocketbar does not handle offline well → [Rocketbar] Rocketbar does not handle offline well
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
User Story: (updated)
Flags: needinfo?(kgrandon)
Attached image search_results_spec.jpg
Ok, making it more clear by attaching that spec and obsoleting the others.
Attachment #8440981 - Attachment is obsolete: true
Attachment #8440985 - Attachment is obsolete: true
Attachment #8441013 - Attachment is obsolete: true
Flags: needinfo?(kgrandon)
(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.
Flags: needinfo?(fdjabri)
Flags: needinfo?(pdolanjski)
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?
Flags: needinfo?(bbajaj)
Keywords: late-l10n
I'll take this. Hope we can uplift given the l10n issue.
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
Attached file Github pull request
Comment on attachment 8445569 [details]
Github pull request

Pre-mature enter press. This is a work in progress.
Attachment #8445569 - Attachment description: Git → Github pull request
Target Milestone: --- → 2.0 S5 (4july)
Comment on attachment 8445569 [details]
Github pull request

Just looking for a review here if either of you have time. Thanks.
Attachment #8445569 - Flags: review?(ran)
Attachment #8445569 - Flags: review?(dale)
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.
Flags: needinfo?(kgrandon)
Comment on attachment 8445569 [details]
Github pull request

Uhhh.. updating to "-" is so harsh..
Attachment #8445569 - Flags: review?(ran) → review-
(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 ;-)
Flags: needinfo?(kgrandon)
(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
One more verification:
Would the offline message appear if online but no results returned from any provider?
Flags: needinfo?(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?
Flags: needinfo?(kgrandon) → needinfo?(ran)
Yes, but it might change soon (which is what we talked over with Peter earlier today)
Flags: needinfo?(ran)
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.
Attachment #8445569 - Flags: review-
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.
Attachment #8445569 - Flags: review?(dale)
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
Attachment #8445569 - Flags: review?(dale)
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.
Attachment #8445569 - Flags: review?(dale) → review+
Thanks! Landed: https://github.com/mozilla-b2g/gaia/commit/a82d0004af1e2e58aea2f6fba293d047c521053f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Flags: needinfo?(francesco.lodolo)
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.
Flags: needinfo?(francesco.lodolo)
;)
Whiteboard: [2.0-FL-bug-bash][systemsfe] → [2.0-FL-bug-bash][systemsfe][NO_UPLIFT]
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.
Attachment #8445569 - Flags: approval-gaia-v2.0?(bbajaj)
(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.
(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?
(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.
(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
Flags: needinfo?(bbajaj)
Ok, then re-noming based on comment 44.
blocking-b2g: 2.0+ → 2.0?
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+] → [VH-FL-blocking-][VH-FC-blocking?]
I'm not sure I understand here. As per comment 44, this should clearly not block. 
Un-nominating once more
blocking-b2g: 2.0? → ---
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.
blocking-b2g: --- → 2.0+
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?] → [VH-FL-blocking-][VH-FC-blocking+]
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.
Flags: needinfo?(l10n)
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.
Flags: needinfo?(l10n)
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.
Flags: needinfo?(bbajaj)
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.
Attachment #8445569 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
Flags: needinfo?(bbajaj)
2.0: https://github.com/mozilla-b2g/gaia/commit/457921c80499ee8f4958aea0b5cfaa2bc824edd3
Whiteboard: [2.0-FL-bug-bash][systemsfe][NO_UPLIFT] → [2.0-FL-bug-bash][systemsfe]
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.