Closed Bug 1234148 Opened 4 years ago Closed 4 years ago

Replace toasts in JS modules with snackbars

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: sebastian, Assigned: alex_johnson, Mentored)

References

Details

(Whiteboard: [lang=js][good next bug])

Attachments

(1 file)

I just saw that we still create toasts in some JS modules:
https://dxr.mozilla.org/mozilla-central/search?q=toast.show+path%3A.js&redirect=false&case=true

We already show snackbars here because we delegate all calls to toast.show to show a snackbar instead (Bug 1216051). But we should clean up the code eventually and use Snackbars.jsm directly:
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/Snackbars.jsm
Alex: Do you want to pick this up? You know Snackbars.jsm and the toast code. :)
Flags: needinfo?(me)
Yeah, I'd love to.  :)
Assignee: nobody → me
Status: NEW → ASSIGNED
Flags: needinfo?(me)
Attachment #8700665 - Flags: review?(s.kaspari) → review+
Comment on attachment 8700665 [details]
MozReview Request: Bug 1234148 - Replaced toasts in JS modules with snackbars. r?sebastian

https://reviewboard.mozilla.org/r/28779/#review25559

::: mobile/android/chrome/content/MasterPassword.js:66
(Diff revision 1)
> -    NativeWindow.toast.show(Strings.browser.GetStringFromName("masterPassword.incorrect"), "short");
> +    Snackbars.show(Strings.browser.GetStringFromName("masterPassword.incorrect"), Snackbars.LENGTH_SHORT);

This probably is not going to work: I assume this is called from within preferences. But currently we only show snackbars in BrowserApp.

However this is not a problem introduced by this patch: The NativeWindow.toast code will try to show a snackbar too.

I'll file a follow-up bug where we can see how to handle this problem.

::: mobile/android/chrome/content/aboutLogins.js:30
(Diff revision 1)
>  function copyStringAndToast(string, notifyString) {

NIT: We could update the method name too.

::: mobile/android/modules/DownloadNotifications.jsm:76
(Diff revision 1)
>      // If this is a new download, show a toast as well.

NIT: We could update the comment too.
Depends on: 1234295
Comment on attachment 8700665 [details]
MozReview Request: Bug 1234148 - Replaced toasts in JS modules with snackbars. r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28779/diff/1-2/
Sebastian, should I set checkin-needed on this, or should I wait for bug 1234295 to land?
Flags: needinfo?(s.kaspari)
Go ahead and land this. bug 1234295 is more or less independent from the changes here.
Flags: needinfo?(s.kaspari)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5a814b26505b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
You need to log in before you can comment on or make changes to this bug.