915918 - If a different tab is selected in the background while in editing mode, URL being entered will open in that new tab

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

Once bug 906041 is fixed, this will be more noticable on Nightly, but it's an issue that has always existed, albeit an edge case that nobody ever reported.

STR:
1) Navigate to http://people.mozilla.com/~bnicholson/test/target.html
2) Within 5 seconds, tap the URL bar to open editing mode (you'll see a doorhanger for enabling popups the first time you do this; enable them)
3) Enter a URL
4) Notice your URL loads in a new different tab, and that target.html is still open in the other tab

Comment 1

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

^ Sorry, needinfo was a mistake.

Comment 2

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

Attachment: 01a: Select previously selected tab upon editing mode exit

This solution feels a bit hacky to me - I would have preferred to have an onSelectTabListener that called canSelectTab each time selectTab is called (returning if it returns false). However, the tab is initially created from JS, which does not know about editing mode on the Java side, and so there's no way to stop that creation without some ugly message passing, particularly because JS should not know about editing mode.

I also took the liberty to add an onEditingModeExit() method, since it also seemed strange to me that there are three entirely different ways of leaving editing mode.

Also, it's worth noting that I felt the addTab code has become a bit crufty over time (e.g. bug 924712). It's also weird that both Java and JS can open a tab and tell the other it happened, in a non-obvious way ("Tab:Load" and "Tab:Added" respectively). I wonder if this code path shouldn't get a refactoring (where, more cleanly, JS can always tell Java to initiate new tabs, or vice versa). What do you think?

Comment 3

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

Attachment: 02a: openUrl -> openUrlAndExitEditingMode

Comment 4

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

Patch 02 is for, in my opinion, better code clarity - it's non-obvious to me that "openUrl" will exit editing mode.

Comment 5

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

Found a bug: if the browser search screen is visible and the app is exited via the "Home" button, the browser search screen never hides itself.

I'll look into this.

Comment 6

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

...And now I can't repro. I wonder if I just wasn't using the correct build.

Carry on.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 815176 [details] [diff] [review]
01a: Select previously selected tab upon editing mode exit

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

Looks good overall, but I think you should try to hook on the existing callback instead of creating a new one. If hooking into onStopEditing is not possible for some reason, I'd avoid the 'on' prefix on the method name (as this is not really a callback) and move the hideHomePager() into it.

::: mobile/android/base/BrowserApp.java
@@ +175,5 @@
>  
>          return mSiteIdentityPopup;
>      }
>  
> +    public Integer selectedTabDuringEditingModeEntryID = null;

nit: All class properties should be declared at the top.

targetTabForEditingMode?

@@ +1400,5 @@
>              throw new IllegalArgumentException("Cannot handle null URLs in enterEditingMode");
>          }
>  
> +        final Tab selectedTab = Tabs.getInstance().getSelectedTab();
> +        selectedTabDuringEditingModeEntryID = selectedTab != null ? selectedTab.getId() : null;

nit: add parens around the ternary conditional.

@@ +1516,5 @@
>      }
>  
> +    private void onEditingModeExit() {
> +        setSelectedTabToSelectedTabDuringEditingModeEntry();
> +        hideBrowserSearch();

Factoring out hideBrowserSearch() and setSelectedTabToSelectedTabDuringEditingModeEntry() together seems a bit arbitrary. There's already an onStopEditing() callback which you could use to set the target tab (and maybe call hideBrowserSearch() and hideHomePager() too?)

@@ +1522,5 @@
> +
> +    /**
> +     * Selects the tab that was selected when editing mode was entered.
> +     *
> +     * XXX: A background tab may be selected while editing mode is active (e.g. popups), causing

Why XXX? Is that something that needs to be addressed later?

@@ +1526,5 @@
> +     * XXX: A background tab may be selected while editing mode is active (e.g. popups), causing
> +     * the new url to load in the newly selected tab. Call this method on editing mode exit
> +     * to mitigate this.
> +     */
> +    private void setSelectedTabToSelectedTabDuringEditingModeEntry() {

selectTabFromEditingMode?

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 815177 [details] [diff] [review]
02a: openUrl -> openUrlAndExitEditingMode

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

::: mobile/android/base/BrowserApp.java
@@ +1293,5 @@
>  
>          return true;
>      }
>  
> +    private void openUrlAndExitEditingMode(String url) {

I'd prefer something simpler like openUrlAndStopEditing()

Comment 9

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

Attachment: 01b: Select previously selected tab upon editing mode exit

(In reply to Lucas Rocha (:lucasr) from comment #7)
> Looks good overall, but I think you should try to hook on the existing
> callback instead of creating a new one.

I was unaware of it (but am glad it exists! :) I refactored to do that.

> Why XXX? Is that something that needs to be addressed later?

I usually use it for things that are somewhat hackish (and perhaps need to be later replaced) but you made me realize how ambiguous it is. It isn't entirely relevant here so I opted to remove it.

I also moved the hideBrowserSearch/hideHomePager refactor into patch 1.5 (to be uploaded).

Comment 10

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

Attachment: 01.5a: Refactor hideBrowserSearch/HomePager into onStopEditing

Comment 11

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

Attachment: 02b: openUrl -> openUrlAndStopEditing

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 816874 [details] [diff] [review]
01b: Select previously selected tab upon editing mode exit

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

Looks good but I need some more context before giving r+

::: mobile/android/base/BrowserApp.java
@@ +166,5 @@
>  
>      private BrowserHealthReporter mBrowserHealthReporter;
>  
> +    // The tab to be selected on editing mode exit.
> +    public Integer targetTabForEditingMode = null;

Should be private and renamed to mTargetTabForEditingMode.

@@ +482,5 @@
>          });
>  
>          mBrowserToolbar.setOnStopEditingListener(new BrowserToolbar.OnStopEditingListener() {
>              public void onStopEditing() {
> +                selectTargetTabForEditingMode();

nit: add empty line here.

@@ +1308,5 @@
>          if (tabs.isSelectedTabId(tabId)) {
>              hideHomePager();
>          } else {
> +            // This tab should be selected on editing mode exit.
> +            targetTabForEditingMode = tabId;

I think I need more context about this change here. What kind of situation are you covering for here? At first sight, this seems to assume a specific order of events to work (i.e. this bit sets targetTabForEditingMode and then onStopEditing will does its thing) which seems a bit fragile.

@@ +1423,5 @@
>              throw new IllegalArgumentException("Cannot handle null URLs in enterEditingMode");
>          }
>  
> +        final Tab selectedTab = Tabs.getInstance().getSelectedTab();
> +        targetTabForEditingMode = (selectedTab != null) ? selectedTab.getId() : null;

nit: I tend to prefer parens around the whole ternary expression.

@@ +1548,5 @@
> +     */
> +    private void selectTargetTabForEditingMode() {
> +        if (targetTabForEditingMode != null) {
> +            Tabs.getInstance().selectTab(targetTabForEditingMode);
> +        }

nit: add empty line here.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 816875 [details] [diff] [review]
01.5a: Refactor hideBrowserSearch/HomePager into onStopEditing

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

Looks good, just needs some clarification.

::: mobile/android/base/BrowserApp.java
@@ +484,5 @@
>          mBrowserToolbar.setOnStopEditingListener(new BrowserToolbar.OnStopEditingListener() {
>              public void onStopEditing() {
>                  selectTargetTabForEditingMode();
> +                hideHomePager();
> +                hideBrowserSearch();

nit: add empty line here.

@@ -1305,5 @@
>          }
>  
> -        // If this tab is already selected, just hide the home pager.
> -        if (tabs.isSelectedTabId(tabId)) {
> -            hideHomePager();

The hideHomePager() was only called if tabId was the selected tab here and, with your change, it will be called unconditionally. Is that intended? Does it work on all cases where a 'switch to tab' entry is tapped in about:home (including from 'tabs from last time')?

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 816876 [details] [diff] [review]
02b: openUrl -> openUrlAndStopEditing

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

Nice.

Comment 15

[Lucas Rocha (:lucasr)]

Not critical for 26, tracking+.

Comment 16

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

Attachment: 01c: Select previously selected tab upon editing mode exit.

(In reply to Lucas Rocha (:lucasr) from comment #12)
> At first sight, this seems to assume a specific
> order of events to work (i.e. this bit sets targetTabForEditingMode and then
> onStopEditing will does its thing) which seems a bit fragile.

Yeah, I agree it feels fragile so I think I came up with a better solution
(in addition to fixing the nits).

Comment 17

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

Attachment: Part 1.5b: Refactor hiding of BrowserSearch and HomePager into onStopEditing.

(In reply to Lucas Rocha (:lucasr) from comment #13)
Rebased and fixed nits.

> The hideHomePager() was only called if tabId was the selected tab here and,
> with your change, it will be called unconditionally. Is that intended?

Yes. When cancelEdit (and thus hideHomePager) are called, we'll hide the
HomePager (though this actually occurs first in a call to Tabs.selectTab).

If we don't want to (i.e. we're on about:home), hideHomePager already accounts
for this case and returns without hiding the HomePager [1].

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=f1b97193d162#1586

However, it could be a bit fragile to rely on this unspecified behavior of
hideHomePager and I'm starting to lean towards calling hideHomePager explicitly
where we need it (like before my changes). Conceptually, when we exit editing
mode, we're not always going to hide the home pager (i.e. we're on about:home)
so this seems to be the better approach.

What do you think?

> Does it work on all cases where a 'switch to tab' entry is tapped in about:home
> (including from 'tabs from last time')?

The one time this code path will be run (I believe) is when about:home is
bookmarked and that bookmark is selected, which I've hand-tested and it works
properly. Other similar cases (e.g. history, tabs from last time, etc.) do not
show about:home in their listings and thus this code path cannot be activated.

Comment 18

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

Comment on attachment 819965 [details] [diff] [review]
Part 1.5b: Refactor hiding of BrowserSearch and HomePager into onStopEditing.

This should be just feedback on the selectTab fix until I get your response in comment 17.

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 819900 [details] [diff] [review]
01c: Select previously selected tab upon editing mode exit.

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

Looks good.

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 819965 [details] [diff] [review]
Part 1.5b: Refactor hiding of BrowserSearch and HomePager into onStopEditing.

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

I like how this enforces consistent behaviour in all cases. On the other hand, I wonder if there are any implicit assumptions regarding the order of these calls in some situations? Make sure this is not the case.

Comment 21

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

Attachment: Part 1.5: Refactor hiding of BrowserSearch and HomePager into onStopEditing.

> I wonder if there are any implicit assumptions regarding the order of these
> calls in some situations?

Good call - dismissEditingMode had an ordering issue with toggling the
HomePager visibility (it's confusing band-aid fix so see bug 911830 for a
discussion on that). I swapped the order and updated the band-aid comment to
clarify it. However, I don't think the band-aid is necessary (see bug 915825
comment 1) so the swap might become a non-issue in the end.

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 821328 [details] [diff] [review]
Part 1.5: Refactor hiding of BrowserSearch and HomePager into onStopEditing.

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

Yep.

Comment 23

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

https://hg.mozilla.org/integration/fx-team/rev/906461e491d9
https://hg.mozilla.org/integration/fx-team/rev/d93a9e884f13
https://hg.mozilla.org/integration/fx-team/rev/c9bdbfd50135

Comment 24

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

Backout because, as lucasr mentioned, I shouldn't be pushing before merge day.

https://hg.mozilla.org/integration/fx-team/rev/8585177af350
https://hg.mozilla.org/integration/fx-team/rev/8c766adb98f4
https://hg.mozilla.org/integration/fx-team/rev/85ed494c5b02

Comment 25

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

Into 28:

remote:   https://hg.mozilla.org/integration/fx-team/rev/28cea8ff6352
remote:   https://hg.mozilla.org/integration/fx-team/rev/7c9a315f345f
remote:   https://hg.mozilla.org/integration/fx-team/rev/b28a7bf0930d

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28cea8ff6352
https://hg.mozilla.org/mozilla-central/rev/7c9a315f345f
https://hg.mozilla.org/mozilla-central/rev/b28a7bf0930d