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

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Margaret, Assigned: mcomella)

Tracking

({reproducible})

Trunk
Firefox 28
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 affected, firefox27 affected, fennec+)

Details

Attachments

(3 attachments, 5 obsolete attachments)

2.90 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
4.54 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
4.47 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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
tracking-fennec: --- → ?
tracking-fennec: ? → 26+
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Flags: needinfo?(ibarlow)
^ Sorry, needinfo was a mistake.
Flags: needinfo?(ibarlow)
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?
Attachment #815176 - Flags: review?(lucasr.at.mozilla)
Attachment #815177 - Flags: review?(lucasr.at.mozilla)
Patch 02 is for, in my opinion, better code clarity - it's non-obvious to me that "openUrl" will exit editing mode.
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.
...And now I can't repro. I wonder if I just wasn't using the correct build.

Carry on.
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?
Attachment #815176 - Flags: review?(lucasr.at.mozilla) → review-
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()
Attachment #815177 - Flags: review?(lucasr.at.mozilla) → review-
(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).
Attachment #815176 - Attachment is obsolete: true
Attachment #816874 - Flags: review?(lucasr.at.mozilla)
Attachment #815177 - Attachment is obsolete: true
Attachment #816876 - Flags: review?(lucasr.at.mozilla)
Attachment #816875 - Attachment description: 01.5: Refactor hideBrowserSearch/HomePager into onStopEditing → 01.5a: Refactor hideBrowserSearch/HomePager into onStopEditing
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.
Attachment #816874 - Flags: review?(lucasr.at.mozilla) → feedback+
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')?
Attachment #816875 - Flags: review?(lucasr.at.mozilla) → feedback+
Comment on attachment 816876 [details] [diff] [review]
02b: openUrl -> openUrlAndStopEditing

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

Nice.
Attachment #816876 - Flags: review?(lucasr.at.mozilla) → review+
Not critical for 26, tracking+.
tracking-fennec: 26+ → ?
tracking-fennec: ? → +
(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).
Attachment #819900 - Flags: review?(lucasr.at.mozilla)
Attachment #816874 - Attachment is obsolete: true
(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.
Attachment #819965 - Flags: review?(lucasr.at.mozilla)
Attachment #816875 - Attachment is obsolete: true
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.
Attachment #819965 - Flags: review?(lucasr.at.mozilla) → feedback?(lucasr.at.mozilla)
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.
Attachment #819900 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #819965 - Flags: feedback?(lucasr.at.mozilla) → feedback+
> 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.
Attachment #821328 - Flags: review?(lucasr.at.mozilla)
Attachment #819965 - Attachment is obsolete: true
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.
Attachment #821328 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/28cea8ff6352
https://hg.mozilla.org/mozilla-central/rev/7c9a315f345f
https://hg.mozilla.org/mozilla-central/rev/b28a7bf0930d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Depends on: 935604
You need to log in before you can comment on or make changes to this bug.