1224521 - Replace non-button toasts with snackbars

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Bug 1157526 replaced all button toasts with snackbars. This bug is about replacing the remaining (non-button) toasts - where reasonable.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 3

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

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.

Comment 4

[Alex Johnson (:alex_johnson)]

Attachment: Switched the bookmark removed and bookmarked link toasts to snackbar

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

(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. :)

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1224521 - Replace all toasts created from within browser.js with snackbars. r?mcomella

Bug 1224521 - Replace all toasts created from within browser.js with snackbars. r?mcomella

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

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..)

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/87f187f2705961aca1ccc09792798fabe64b0d02
Bug 1224521 - Replace all toasts created from within browser.js with snackbars. r=mcomella

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87f187f27059

Comment 14

[Teodora Vermesan (:TeoVermesan)]

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

Comment 15

[Teodora Vermesan (:TeoVermesan)]

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

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.