Closed Bug 1215026 Opened 4 years ago Closed 4 years ago

Create JSM snackbar API

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

In bug 1157526 we want to replace various toasts with snackbars. Often we rely on NativeWindow.toast to display the toasts, for example when closing a tab (bug 1204760). We should create a NativeWindow.snackbar API. This allows us as well as add-ons to use snackbars. The toast and snackbar APIs are very similar.

NativeWindow.toast:
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/NativeWindow/toast

Android Snackbar API:
https://developer.android.com/reference/android/support/design/widget/Snackbar.html
I was thinking about creating a follow-up bug to deprecate NativeWindow.toast afterwards. But seeing that Android did not deprecate toasts in favor of snackbars, I don't think we should push into this direction.
Let's not add anything to NativeWindow. I wish I had never created NativeWindow. Let's just create a new JSM.

I was going to also suggest that we just change the implementation of Toast to show snackbars instead of toasts, but then Sebastian mentioned that Android still allows both toasts and snackbars to be used, so maybe we should *add* Snackbar and not *replace* Toast. I would hope that the APIs would be similar though.
(In reply to Mark Finkle (:mfinkle) from comment #2)
> Let's not add anything to NativeWindow. I wish I had never created
> NativeWindow. Let's just create a new JSM.

Agreed! I just looked at how toasts are shown from JS. I'm okay with implementing whatever is the best solution nowadays.
Summary: Create NativeWindow.snackbar API → Create JSM snackbar API
(In reply to Mark Finkle (:mfinkle) from comment #2)
> Let's not add anything to NativeWindow. I wish I had never created
> NativeWindow. Let's just create a new JSM.

+1 to this.

> I was going to also suggest that we just change the implementation of Toast
> to show snackbars instead of toasts, but then Sebastian mentioned that
> Android still allows both toasts and snackbars to be used, so maybe we
> should *add* Snackbar and not *replace* Toast. I would hope that the APIs
> would be similar though.

Not replacing the existing API leaves a potentially difficult deprecation to the future.  I prefer replacing the existing API, although I could be convinced otherwise.
Keywords: dev-doc-needed
(In reply to Nick Alexander :nalexander from comment #4)
> Not replacing the existing API leaves a potentially difficult deprecation to
> the future.  I prefer replacing the existing API, although I could be
> convinced otherwise.

There's a risk of the Snackbar changing/vanishing too. Toasts are a system feature since Android 1.0. Snackbars are only in the Android Design Support Library right now.

The Material design guidelines mention both Toasts and Snackbars - without preferring one over the other. However it then continues with just describing guidelines for Snackbars:
https://www.google.de/design/spec/components/snackbars-toasts.html
I created bug 1216051 for deciding what to do with NativeWindow.toast.
Attached patch 1215026-snackbar-api.patch (obsolete) — Splinter Review
I've build this by looking at the toast implementation and other JSMs. :)
Attachment #8676401 - Flags: review?(mark.finkle)
Comment on attachment 8676401 [details] [diff] [review]
1215026-snackbar-api.patch


>diff --git a/mobile/android/modules/Snackbars.jsm b/mobile/android/modules/Snackbars.jsm

>+var Snackbars = {

>+  _callbacks: {},
>+
>+  init: function() {
>+    Services.obs.addObserver(this, "Snackbar:Click", false)
>+  },
>+
>+  show: function(aMessage, aDuration, aOptions) {
>+    let msg = {
>+      type: 'Snackbar:Show',
>+      message: aMessage,
>+      duration: aDuration,
>+    };
>+
>+    if (aOptions && aOptions.action) {
>+      msg.action = {
>+        id: uuidgen.generateUUID().toString()
>+      };
>+
>+      if (aOptions.action.label) {
>+        msg.action.label = aOptions.action.label;
>+      }
>+
>+      this._callbacks[msg.action.id] = aOptions.action.callback;
>+    }
>+
>+    Services.androidBridge.handleGeckoMessage(msg);
>+  },
>+
>+  observe: function(aSubject, aTopic, aData) {
>+    if (this._callbacks[aData]) {
>+      this._callbacks[aData]();
>+      delete this._callbacks[aData];
>+    }
>+  }
>+};
>+
>+Snackbars.init();

Take a look at the Toasts.jsm part of the patch here:
https://bug1052276.bmoattachments.org/attachment.cgi?id=8471763

We should not need to hold callbacks and do an init() if we use Messaging.jsm

Also, I have an idea for a really simple test, modeled after testTrackingProtection.java / .js where we only test for the "Snackbar:Show" message and data, and not try to verify the Snackbar UI was actually shown.
Attachment #8676401 - Flags: review?(mark.finkle) → review-
(In reply to Mark Finkle (:mfinkle) from comment #8)

> Also, I have an idea for a really simple test, modeled after
> testTrackingProtection.java / .js where we only test for the "Snackbar:Show"
> message and data, and not try to verify the Snackbar UI was actually shown.

+1 let's add a test!
Thanks! So much nicer with EventCallback.

This patch also adds a simple test. I had to refactor the callback handling a bit because Snackbar does not guarantee to only call the action callback xor the dismiss callback.
Attachment #8676401 - Attachment is obsolete: true
Attachment #8676800 - Flags: review?(mark.finkle)
Comment on attachment 8676800 [details] [diff] [review]
1215026-snackbar-api-v2.patch

LGTM!
Attachment #8676800 - Flags: review?(mark.finkle) → review+
Depends on: 1217416
https://hg.mozilla.org/mozilla-central/rev/dc5816aa157c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
QA Contact: teodora.vermesan
You need to log in before you can comment on or make changes to this bug.