Closed
Bug 915918
Opened 11 years ago
Closed 11 years ago
If a different tab is selected in the background while in editing mode, URL being entered will open in that new tab
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(firefox26 affected, firefox27 affected, fennec+)
RESOLVED
FIXED
Firefox 28
People
(Reporter: Margaret, Assigned: mcomella)
References
Details
(Keywords: reproducible)
Attachments
(3 files, 5 obsolete files)
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 |
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
Updated•11 years ago
|
tracking-fennec: --- → ?
Updated•11 years ago
|
tracking-fennec: ? → 26+
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Updated•11 years ago
|
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 2•11 years ago
|
||
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)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #815177 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 4•11 years ago
|
||
Patch 02 is for, in my opinion, better code clarity - it's non-obvious to me that "openUrl" will exit editing mode.
Assignee | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
...And now I can't repro. I wonder if I just wasn't using the correct build. Carry on.
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
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-
Assignee | ||
Comment 9•11 years ago
|
||
(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)
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #816875 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #815177 -
Attachment is obsolete: true
Attachment #816876 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #816875 -
Attachment description: 01.5: Refactor hideBrowserSearch/HomePager into onStopEditing → 01.5a: Refactor hideBrowserSearch/HomePager into onStopEditing
Comment 12•11 years ago
|
||
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 13•11 years ago
|
||
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 14•11 years ago
|
||
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+
Updated•11 years ago
|
tracking-fennec: ? → +
Assignee | ||
Comment 16•11 years ago
|
||
(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)
Assignee | ||
Updated•11 years ago
|
Attachment #816874 -
Attachment is obsolete: true
Assignee | ||
Comment 17•11 years ago
|
||
(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)
Assignee | ||
Updated•11 years ago
|
Attachment #816875 -
Attachment is obsolete: true
Assignee | ||
Comment 18•11 years ago
|
||
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 19•11 years ago
|
||
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 20•11 years ago
|
||
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+
Assignee | ||
Comment 21•11 years ago
|
||
> 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)
Assignee | ||
Updated•11 years ago
|
Attachment #819965 -
Attachment is obsolete: true
Comment 22•11 years ago
|
||
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+
Assignee | ||
Comment 23•11 years ago
|
||
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
Assignee | ||
Comment 24•11 years ago
|
||
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
Assignee | ||
Comment 25•11 years ago
|
||
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•11 years ago
|
||
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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•