Closed Bug 1224521 Opened 4 years ago Closed 4 years ago

Replace non-button toasts with snackbars

Categories

(GeckoView :: General, defect)

All
Android
defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1157526 replaced all button toasts with snackbars. This bug is about replacing the remaining (non-button) toasts - where reasonable.
With a quick search I found the following non-button toasts:

* Text copied
* Add-on downloading
* Search engine added
* Adding search engine error
* Full screen alert
* Bookmark removed
* Removed from reading list
* No synced devices
* Bookmark updated
* Bookmark added
* Bookmark already added
* (A bunch of account toasts)
* Page removed
* ..

Anthony: I was wondering whether we want to replace all of them with snackbars or if we still want to use toasts for some of those?
Flags: needinfo?(alam)
At first I thought for just simple feedback a toast might be less disturbing. But now with all the button toasts migrated to snackbars it looks inconsistent to have both. For example:

* (Snackbar) Bookmark added  [OPTIONS]
* (Toast)    Bookmark removed

Maybe they all should be snackbars - with LENGTH_SHORT for the simple ones.
I can't think of any where I'd prefer the toast over the snackbar tbh.

For duration, we can go with short for now (for non-actionable bars) and long (for actionable snackbars).

We can also adjust as necessary afterwards.
Flags: needinfo?(alam)
Summary: Replace (some) non-button toasts with snackbars → Replace non-button toasts with snackbars
(In reply to Alex Johnson(:alex_johnson) from comment #4)
> Created attachment 8692608 [details] [diff] [review]
> Switched the bookmark removed and bookmarked link toasts to snackbar

Oh wait, I have a patch locally for all toasts in browser.js. Maybe I should land this and create a follow-up for all the remaining Toast.makeText() calls that are scattered all over the code base. They might be harder to replace because we will need to either call into BrowserApp or get access to some view node. If you want to you can work on the follow-up. :)
Assignee: nobody → s.kaspari
(In reply to Sebastian Kaspari (:sebastian) from comment #5)
> (In reply to Alex Johnson(:alex_johnson) from comment #4)
> > Created attachment 8692608 [details] [diff] [review]
> > Switched the bookmark removed and bookmarked link toasts to snackbar
> 
> Oh wait, I have a patch locally for all toasts in browser.js. Maybe I should
> land this and create a follow-up for all the remaining Toast.makeText()
> calls that are scattered all over the code base. They might be harder to
> replace because we will need to either call into BrowserApp or get access to
> some view node. If you want to you can work on the follow-up. :)

I filed bug 1228414 for that.
Bug 1224521 - Replace all toasts created from within browser.js with snackbars. r?mcomella
Attachment #8692623 - Flags: review?(michael.l.comella)
Attachment #8692608 - Attachment is obsolete: true
Attachment #8692608 - Flags: review?(s.kaspari)
Comment on attachment 8692623 [details]
MozReview Request: Bug 1224521 - Replace all toasts created from within browser.js with snackbars. r?mcomella

https://reviewboard.mozilla.org/r/26295/#review24123

lgtm however, it looks like there are some test failures. r+ on this patch and I'll r? the test fixes.
Attachment #8692623 - Flags: review?(michael.l.comella) → review+
Seems like the test failures have been caused by a syntax error in browser.js I pushed:
Missing ) in line 12:
https://hg.mozilla.org/try/rev/81d65f5672d9#l1.13

With that fixed the tests passed (testDistribution seems to be flaky..)
https://hg.mozilla.org/integration/fx-team/rev/87f187f2705961aca1ccc09792798fabe64b0d02
Bug 1224521 - Replace all toasts created from within browser.js with snackbars. r=mcomella
https://hg.mozilla.org/mozilla-central/rev/87f187f27059
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Tested using:
Device: Nexus 4 (Android 5.1.1)
Build: Firefox for Android 45.0a1 (2015-12-04)

A snackbar is displayed after:

- a text is copied
- an add-on is downloaded
- a video enters full screen
- a download begins
- a search engine is added
- a page is printed

No snackbar is displayed after:

- page is removed from reading list
- a bookmark is removed
- bookmark is updated
- page is removed from home panels
Is this expected?
(In reply to Teodora Vermesan (:TeoVermesan) from comment #14)
> No snackbar is displayed after:
> - page is removed from reading list
> - a bookmark is removed
> - bookmark is updated
> - page is removed from home panels
Flags: needinfo?(s.kaspari)
(In reply to Teodora Vermesan (:TeoVermesan) from comment #15)
> Is this expected?
> (In reply to Teodora Vermesan (:TeoVermesan) from comment #14)
> > No snackbar is displayed after:
> > - page is removed from reading list
> > - a bookmark is removed
> > - bookmark is updated
> > - page is removed from home panels

Yeah, we are creating them differently in code. They are getting migrated as part of bug 1228414.
Flags: needinfo?(s.kaspari)
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 45 → mozilla45
You need to log in before you can comment on or make changes to this bug.