1130646 - Find non-hacky way to make back button dismiss reader mode popup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect, P3)

Description

[:Margaret Leibovic]

Follow-up to bug 907079.

To do this, I think we should find a way to let JS consumers register to handle the back button press in our chrome code before actually navigating back.

This is a bit tricky because currently the Java side doesn't wait for JS to unsuccessfully navigate back in session history before proceeding with more back press logic, like exiting the app (right now we just use a canGoBack flag to keep track of whether or not that session history back navigation would work).

Comment 1

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1130646/margaret

/r/3505 - Bug 1130646 - Allow JS to intercept Android back press so that we can dismiss the reader mode popup. r=mfinkle

Pull down this commit:

hg pull review -r 666f41d7e8367d72d1551a1567fa67e5f9162316

Comment 2

[:Margaret Leibovic]

I'm prepared for you to tell me that this is terrible, but I feel like it's not as bad as I thought it would be... One thing I don't like is that this back press message is separate from the "Session:Back" we send in the tab, but we call tab.doBack() in other places, so refactoring this would be a bit of a mess.

With this patch, we can only have one back press listener at a time on the JS side, but I think that's okay, because if we allowed for multiple back press handlers, how would we know which one to try to call first?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Before even looking at this patch, I want to state that I would be OK if we dropped the approach altogether. As the Desktop and Android readers diverge it's possible we could use a native UI (yes, I said it) to display the controls. Such a popup would allow us to more easily handle the "BACK to close" behavior.

Comment 4

[Brian Nicholson (:bnicholson)]

https://reviewboard.mozilla.org/r/3505/#review2837

This doesn't actually seem that terrible at all to me, though I have a couple comments below.

::: mobile/android/base/GeckoApp.java
(Diff revision 1)
> +            public void onError(NativeJSObject error) {

This part seems hacky, especially where you call this.onError(null) from onResponse. Assuming your intention here is to fall back to the default behavior in case we/an add-on throws in their listener, I'd create a separate helper method called by onResponse/onError to make the intent clearer.

::: mobile/android/chrome/content/browser.js
(Diff revision 1)
> +    this._backPressListener = listener;

If anyone else uses this API, it will clobber the reader mode back press listener (or vice versa). Since you've gone this far in making a fairly generic API that can be hooked into, you might as well have  a \_backPressListeners array, then replace setBackPressListener with addBackPressListener/removeBackPressListener. Then you can iterate the listeners array and use the first listener that returns true.

Edit: just saw you mentioned this in the bug. How about over iterate them in reverse order so the most recently added listener has the highest priority (i.e., a stack)? I think it's OK to assume that consumers of this API will be good citizens and remove their listeners when they're popped from the back stack, just like you do in this patch.

Comment 5

[:Margaret Leibovic]

antlam, how do you feel about this? As of Fx38, the back button doesn't close the font style popup anymore. We should decide if we want to fix this bug to add that back, or if we're okay living without that behavior we can just close this out as WONTFIX.

Comment 6

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

To keep consistent with our other UI like Menus, tabs tray, I'd very much like to try and fix this. :)

Comment 8

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1130646 - Allow JS to intercept Android back press so that we can dismiss the reader mode popup. r=mfinkle

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8619393 [details]
MozReview Request: Bug 1130646 - Allow JS to intercept Android back press so that we can dismiss the reader mode popup. r=mfinkle

https://reviewboard.mozilla.org/r/3505/#review11091

::: mobile/android/chrome/content/browser.js:4924
(Diff revision 1)
> +    }, "Browser:OnBackPressed");

Looks like only one hadler is allowed per message in Messaging.jsm. Would that be a problem if more than one about page is using this mechanism?

Comment 10

[Mark Capella [:capella]]

I'd like to see this in place. I always hit the back key to close the Drawer and wind up navigating backwards instead.

Answering marks question in comment #9, if there are two tabs in readerView and both have the drawer opened, hitting BACK in the first one correctly closes the drawer, but if you tab switch to the other tab and hit BACK, you wind up navigating back instead, even though the drawer is still visibly open, as the single listener has been removed.

margaret, can I help move this along?

Comment 11

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #10)
> I'd like to see this in place. I always hit the back key to close the Drawer
> and wind up navigating backwards instead.
> 
> Answering marks question in comment #9, if there are two tabs in readerView
> and both have the drawer opened, hitting BACK in the first one correctly
> closes the drawer, but if you tab switch to the other tab and hit BACK, you
> wind up navigating back instead, even though the drawer is still visibly
> open, as the single listener has been removed.
> 
> margaret, can I help move this along?

Sure! I haven't had time to work on this myself, so you can probably make more progress than I can.

Do you have ideas for how to fix this? I haven't thought about this patch in a while, so I'd need to refresh my memory to brainstorm better ways to solve this problem.

Comment 12

[Mark Capella [:capella]]

Attachment: MY_bug1130646.diff

I've been testing this, it's basically your patch, with a few tweaks.

Comment 13

[Mark Capella [:capella]]

Review nag ping?

Comment 14

[:Margaret Leibovic]

Comment on attachment 8639310 [details] [diff] [review]
MY_bug1130646.diff

Review of attachment 8639310 [details] [diff] [review]:
-----------------------------------------------------------------

Adding mcomella also, in case he can get to this before I can.

Comment 15

[Mark Capella [:capella]]

Keep alive ping.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8639310 [details] [diff] [review]
MY_bug1130646.diff

Review of attachment 8639310 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for being so slow here. This is generally looking good, but I have a few questions/concerns.

Also, I'm about to go on PTO for a while, but maybe mcomella or bnicholson could help finish reviewing this when you have a new version. To be fair, this is pretty similar to my original patch, so it would be good to get another set of eyes :)

::: mobile/android/base/GeckoApp.java
@@ +2351,5 @@
> +
> +            @Override
> +            public void onError(NativeJSObject error) {
> +                // Default behavior is Gecko didn't prevent, via failure.
> +                Log.e(LOGTAG, "Browser:OnBackPressed fails, falling back to default action.");

I don't think we need to log an error here.

::: mobile/android/chrome/content/Reader.js
@@ +71,5 @@
> +
> +      case "Reader:DropdownOpened": {
> +        BrowserEventHandler.setBackPressListener(() => {
> +          // Provide positive response whether tab dropdown closed.
> +          let resultArray = this._syncMS.sendSyncMessage("Reader:CloseDropdown", message.data);

Can you explain why this sync message sender service is needed? I'm not as familiar with the message manager APIs, but is it not possible to just do `message.target.messageManager.sendSyncMessage`?

Also, why do we need a sync message here instead of an async message?

::: mobile/android/chrome/content/browser.js
@@ +5052,5 @@
>  };
>  
>  var BrowserEventHandler = {
> +
> +  _backPressListener: [],

If you're updating this to be an array, I would rename this variable to be plural.

@@ +5088,5 @@
> +  /**
> +   * Provides string key for _backPressListener array based on currently selected tabId.
> +   */
> +  _getListenerKey: function() {
> +    return "key_" + BrowserApp.selectedTab.id;

Instead of the "key_" prefix, why not just use the tab id directly?

@@ +5094,5 @@
> +
> +  /**
> +   * Remove the _backPressListener entry for this tabId.
> +   */
> +  clearBackPressListener: function() {

I think we should update this function to take a tab id, to avoid having this function behave differently depending on what tab is currently selected.

If we are registering back press listener per tab (which sounds like a good way to avoid problems with switching tabs), then we should make that a proper part of the API.

We should also make sure that we unregister listeners when tabs are closed, to avoid memory leaks.

::: toolkit/components/reader/AboutReader.jsm
@@ +185,5 @@
> +      return this._viewId;
> +    }
> +
> +    return this._viewId = Cc["@mozilla.org/uuid-generator;1"].
> +      getService(Ci.nsIUUIDGenerator).generateUUID().toString();

Instead of setting a separate _viewId property, I think you should be able to replace the viewId getter with its new value. E.g:

get viewId() {
  return this.viewId = CC...;
}

@@ +203,5 @@
>        }
> +
> +      case "Reader:CloseDropdown": {
> +        return (message.data === this.viewId) ?
> +          this._closeDropdown() : false;

Why do you need to provide a return value here?

Also nit: this could all go on one line.

Comment 17

[Mark Capella [:capella]]

Enjoy your PTO ! I'm not really in a hurry here, I have the patch in /my/ queue \o/

I've tried to suck mcomella into my little corner of the world but he won't corrupt yet :p so I'll leave these comments and plan on picking up with you later after my next post.

As usual, bnicholson or anyone else feeling frisky is always invited to drive-by.


After nailing down the nits we're left with:

::: toolkit/components/reader/AboutReader.jsm
@@ +185,5 @@
> +      return this._viewId;
> +    }
> +
> +    return this._viewId = Cc["@mozilla.org/uuid-generator;1"].
> +      getService(Ci.nsIUUIDGenerator).generateUUID().toString();

Instead of setting a separate _viewId property, I think you should be able to replace the viewId getter with its new value. E.g:
get viewId() {
  return this.viewId = CC...;
}

----------> That updates the value each time called. I need the field to remain static with the first valid value for later compare on the "Reader:CloseDropdown" message received in AboutReader.jsm, to prevent the wrong view from closing.

@@ +71,5 @@
> +
> +      case "Reader:DropdownOpened": {
> +        BrowserEventHandler.setBackPressListener(() => {
> +          // Provide positive response whether tab dropdown closed.
> +          let resultArray = this._syncMS.sendSyncMessage("Reader:CloseDropdown", message.data);

Can you explain why this sync message sender service is needed?
Is it not possible to just do `message.target.messageManager.sendSyncMessage`?
Also, why do we need a sync message here instead of an async message?
Why do you need to provide a return value here?

----------> These all relate to the same thing. I changed your approach from async to sync as there was a spot in the middle where we return a value "true" to Java assuming success of dispatched message to Gecko. Minor annoyance on my part that I had time to tinker. I can switch it back easily enough, seems like a negligable risk.

@@ +5088,5 @@
> +  /**
> +   * Provides string key for _backPressListener array based on currently selected tabId.
> +   */
> +  _getListenerKey: function() {
> +    return "key_" + BrowserApp.selectedTab.id;

Instead of the "key_" prefix, why not just use the tab id directly?

----------> I was thinking memory footprint. Using the tab id#, for a user that has 100 tabs and tab 1 and 100 in readermode, we get a listener array with length 100, but only two valid entries, that we then carry around internally until process termination. I started out with using the tab id, had time to explore .... :)  I can change that to use tab id again.

@@ +5094,5 @@
> +
> +  /**
> +   * Remove the _backPressListener entry for this tabId.
> +   */
> +  clearBackPressListener: function() {

I think we should update this function to take a tab id, to avoid having this function behave differently depending on what tab is currently selected.
If we are registering back press listener per tab (which sounds like a good way to avoid problems with switching tabs), then we should make that a proper part of the API.

----------> Yes, we depend on the assumption that the currently selected tab is the logically valid one for the entire codepath. Can we tell the Android tab id directly in AboutReader.jsm where the "Reader:DropdownClosed" or "Reader:DropdownOpened" UI messages are initiated? It seems OS agnostic there (being shared with desktop).


RE: We should also make sure that we unregister listeners when tabs are closed, to avoid memory leaks.

----------> The thought was we always get a page "unload" in AboutReader.jsm triggering a "Reader:CloseDropdown" to clear up the listener. I tested this somewhat but I can look closer for leaks.

Comment 18

[Mark Capella [:capella]]

Attachment: bug1130646_Allow_For_ListenAfterClose_flag.diff

maragaret may have a good point on second reading, regarding asking bnicholson for input. I now realize you were involved earlier so this might seem familiar.

After addressing margarets final comments in the last version, I noticed something in testing that requires this patch as a pre-req, and may require more explaining than it's worth :-)

When a tab is swiped shut, we catch the "unload" and call AboutReader._closeDropdown(). If the font-dropdown is open, it's closed, and a "Reader:DropdownClosed" message sent to Reader.js to call into browser.js via |BrowserEventHandler.clearBackPressInfo(viewID)| to complete the (per-tab) BackPressInfo cleanup.

This wasn't working.

So for tabs that are swiped closed while they have the font dropdown open, we leak BackPress state info in browser.js.

I dug in enough to learn:
   ) By the time a page script gets an "unload" message, |FrameMessageManager| in a closed state. [0]
   ) Async Messages such as "Reader:DropdownClosed" can be sent by the script during "unload" handling,
     and enter the inbound side of the |SameProcessMessageQueue| shared by |nsInProcessTabChildGlobal| and
     |nsFrameMessageManager| successfully.
   ) The messages can then be dropped on the floor when being considered for retrieval [1]

Ohhhhhh, THAT's why my code was failing !!! :-<

There's a save here, though. The listener.mListenWhenClosed flag turns out to be a convenient override. See: Bug 1126089 of around March this year... Note the doc out of date: [2], but the .IDL source shows: [3]

So, anyhow, I need to patch browser.js to allow for message level values for this flag in the |Lazily-loaded browser scripts that use message listeners| definition.


[0] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFrameMessageManager.cpp?rev=ed6830f48c58&mark=1383#1372
[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFrameMessageManager.cpp?rev=ed6830f48c58&mark=1082-1082,1112-1114#1080
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIMessageListenerManager
[3] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIMessageManager.idl?rev=cd4e131df428&mark=218-222,226-226#172

Comment 19

[Mark Capella [:capella]]

Attachment: bug1130646_The_Patch.diff

The actual correction, using the pre-req (above)

Comment 20

[Mark Capella [:capella]]

Attachment: bug1130646_Warn_On_Msg_CC.diff

Maybe this patch for future (?)

Comment 21

[Brian Nicholson (:bnicholson)]

Sorry for the delayed review on this. I have some pending comments, and I'll try to finish by tomorrow.

Comment 22

[Brian Nicholson (:bnicholson)]

Comment on attachment 8659057 [details] [diff] [review]
bug1130646_The_Patch.diff

Review of attachment 8659057 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoApp.java
@@ +2382,5 @@
> +                    tabs.closeTab(tab, parent);
> +                    return;
> +                }
> +
> +                moveTaskToBack(true);

GeckoAppShell.sendRequestToGecko executes its callbacks on the Gecko thread. I don't think everything in this block is thread-safe, so we should wrap onDefault in a Runnable dispatched on the UI thread.

::: mobile/android/chrome/content/browser.js
@@ +4604,5 @@
> +
> +    Messaging.addListener(() => {
> +      // If the current (selected) tab has backPress listener, return its results, else false.
> +      let listener = this._backPressListeners[BrowserApp.selectedTab.id];
> +      return { handled: (listener ? listener() : false) };

So we still only support one listener per tab? Based on the above comments, I was thinking we should iterate over a set of listeners so that setting another listener doesn't clobber any existing ones.

At first I thought that's why you were adding the viewId property. If we support only one back press listener per tab, why did you need the unique identifier (and not just use the tabId)?

@@ +4622,5 @@
> +
> +  /**
> +   * Set the BackPressInfo for this tabId / ReaderView Id pair.
> +   */
> +  setBackPressInfo: function(tabId, viewId, listener) {

We should rename these and update the comments to something much more generic; e.g., addBackPressListener()/removeBackPressListener(). I think the "non-hacky" part of this bug is making something that isn't specific to reader view :)

::: toolkit/components/reader/AboutReader.jsm
@@ +185,5 @@
> +      return this._viewId;
> +    }
> +
> +    return this._viewId = Cc["@mozilla.org/uuid-generator;1"].
> +      getService(Ci.nsIUUIDGenerator).generateUUID().toString();

You can remove _viewId and replace this with "this.viewId = ..." as margaret mentioned. Setting this.viewId will replace the getter, so all subsequent accesses to the viewId property will simply return the value rather than calling this function. In short, you're just setting up a lazy getter.

Comment 23

[Brian Nicholson (:bnicholson)]

Comment on attachment 8659248 [details] [diff] [review]
bug1130646_Warn_On_Msg_CC.diff

Don't think we want this...seems like too much cruft for such a specific/relatively minor issue.

Comment 24

[Mark Capella [:capella]]

Attachment: bug1130646_The_Patch.diff

re: |GeckoAppShell.sendRequestToGecko executes its callbacks on the Gecko thread|
>>> Awesome! Thanks.

re: |We should rename these addBackPressListener()/removeBackPressListener()|
>>> Done.

re: |You can remove _viewId and replace this with "this.viewId = ..." as margaret mentioned.|
>>> I've previously used a pattern like: [0], but the assignment here was throwing for me, with: 
    TypeError: setting a property that has only a getter.
    This is apparently due to the |new AboutReader()| prototype. I've attached a new solution.
    Correct me if I'm still off in left-field.

re: |why did you need the unique identifier (and not just use the tabId)?|
>>> The main case is to associate a tabId to a viewId at the point we open the font popup. This is
    because when the user taps the 'X' close on the tab, the unload is caught in AboutReader.jsm
    and by the team it gets to Android via "Reader:DropdownClosed" in Reader.js, the tab is
    destroyed, and the tab selection has been moved, so cleanup of the tab listener is incorrect.

re: |So we still only support one listener per tab? Based on the above comments,
     I was thinking we should iterate over a set of listeners so that setting another
     listener doesn't clobber any existing ones.|
>>> Maybe I've missed earlier discussions.
    We currently assume one listener per tab. Navigation will close any open font popup and
    listeners are removed. As we assign unique ID's to each ReaderView, and tab.id's don't seem to be reused,
    I can't see how to generate an association that collides.


[0] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=1be077af56e3#96

Comment 25

[Brian Nicholson (:bnicholson)]

Comment on attachment 8663677 [details] [diff] [review]
bug1130646_The_Patch.diff

Review of attachment 8663677 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mark Capella [:capella] from comment #24)
> re: |You can remove _viewId and replace this with "this.viewId = ..." as
> margaret mentioned.|
> >>> I've previously used a pattern like: [0], but the assignment here was throwing for me, with: 
>     TypeError: setting a property that has only a getter.
>     This is apparently due to the |new AboutReader()| prototype.

Hm, do you have a code snippet you tried? I'm curious what's going on here since I think we do this in many JSMs, including object prototypes. This little blurb works for me:
  let P = function() {};
  P.prototype = { get view() { delete this.view; return this.view = 123; } };
  let p = new P();
  p.view;

(In reply to Mark Capella [:capella] from comment #24)
> >>> Maybe I've missed earlier discussions.
>     We currently assume one listener per tab.

Right, I don't think we should make that assumption (see previous comments about making this a generic, "non-hacky" API). We should support back press listeners in general for anything that wants to register them -- no need to make this reader-specific.

Comment 26

[Mark Capella [:capella]]

I believe your snippet fails appropriately with the addition of "use strict";

Regarding: |back press listeners in general for anything that wants to register them|
That does expand the scope as I understood it then. I've considered future uses of the wire-frame as I've gone along, and failing to see an immediate need, and not having been involved in previous agreements, was assuming we'd move this specific-need solution, (ReaderView), and settle for this as a springboard for any later expansion of the chain.

Comment 27

[Brian Nicholson (:bnicholson)]

Comment on attachment 8659054 [details] [diff] [review]
bug1130646_Allow_For_ListenAfterClose_flag.diff

Review of attachment 8659054 [details] [diff] [review]:
-----------------------------------------------------------------

Apologies again for the lengthy review delay. Last week was our iOS work week, so didn't have the opportunity to context switch for Android reviews!

Comment 28

[Brian Nicholson (:bnicholson)]

Comment on attachment 8663677 [details] [diff] [review]
bug1130646_The_Patch.diff

Review of attachment 8663677 [details] [diff] [review]:
-----------------------------------------------------------------

Nice find digging into the listenWhenClosed flag! Crazy that you needed so much depth in the inner workings of the browser to fix something that looks so deceptively simple.

::: mobile/android/chrome/content/browser.js
@@ +4634,5 @@
> +
> +  /**
> +   * Remove the backPressListener for this ReaderView Id.
> +   */
> +  removeBackPressListener: function(viewId) {

Hm, so if we aren't going to make this a true generic listener-based approach, we should rethink these functions being on BrowserEventHandler. On the surface, they seem like a nice way for consumers to hook into the back button; in practice, this is a limited, specific implementation that shouldn't be public (and would break things if we/add-ons tried to use it elsewhere).

If we're not going to support listeners for anything other than our reader view, can we move these out of BrowserEventHandler and into Reader.js? It would be nice to have this be a generic API in the future, but I'd prefer to avoid confusion in the meantime.

Comment 29

[Mark Capella [:capella]]

Attachment: bug1130646_The_Patch.diff

re: |in practice, this is a limited, specific implementation that shouldn't be public|

Agreed! I moved the listeners to Reader.js as attached.

re: |Crazy that you needed so much depth in the inner workings of the browser|

Not at all! That was the most interesting bit of the whole bug! I do like working with Java, but it tends to be pretty boring UI stuff. The Gecko/Core interactions are where all the action is.

Carrying over r+, and pushing to try ... 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dd8cb1352bc

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8b02d7bec20f
https://hg.mozilla.org/integration/fx-team/rev/0893fb109341

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b02d7bec20f
https://hg.mozilla.org/mozilla-central/rev/0893fb109341