Closed Bug 906041 Opened 11 years ago Closed 11 years ago

Newly opened pages replace about:home while editing mode is active

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

ARM
Android
defect

Tracking

(firefox26+ fixed, firefox27 verified, fennec26+)

VERIFIED FIXED
Firefox 27
Tracking Status
firefox26 + fixed
firefox27 --- verified
fennec 26+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

(Keywords: regression, reproducible)

Attachments

(2 files, 2 obsolete files)

STR:
1) Visit some page
2) Swipe close fennec from app list
3) Open fennec
4) Quickly tap on urlbar

When you do this, the restored page is shown while in editing mode, not about:home. If you exit editing mode and re-enter it, we do show about:home.

Testing on a Nexus 4.
This seems more like the session-restored page overrides the about:home UI if you start editing mode before gecko is up and running.
>This seems more like the session-restored page overrides the about:home UI if you start editing mode before gecko is up and running.

Seems likely - about:home shows up momentarily for me before the page content is drawn on top.
I'll try to investigate this.
Actually, I wonder if Brian might want to take this (or offer an opinion about where to start looking), since he's more familiar with all this session restore logic.
Flags: needinfo?(bnicholson)
Assuming this is just caused by Gecko creating a new tab while editing mode is open, I wonder if this is about more than just session restore. For example, what happens if a page decides to do window.open() with a target of "_blank" while editing mode is active?
Flags: needinfo?(bnicholson)
tracking-fennec: --- → ?
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → 26+
(In reply to Brian Nicholson (:bnicholson) from comment #5)
> Assuming this is just caused by Gecko creating a new tab while editing mode
> is open, I wonder if this is about more than just session restore. For
> example, what happens if a page decides to do window.open() with a target of
> "_blank" while editing mode is active?

I tested this out, and this bug is in fact reproducible using window.open().

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)
Summary: [fig] about:home isn't shown if you enter editing mode while page is restoring during startup → Newly opened pages replace about:home while editing mode is active
(In reply to Brian Nicholson (:bnicholson) from comment #6)
> (In reply to Brian Nicholson (:bnicholson) from comment #5)
> > Assuming this is just caused by Gecko creating a new tab while editing mode
> > is open, I wonder if this is about more than just session restore. For
> > example, what happens if a page decides to do window.open() with a target of
> > "_blank" while editing mode is active?
> 
> I tested this out, and this bug is in fact reproducible using window.open().
> 
> 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)

Thanks for the testcase. This also exposes a weird case where we show doorhanger notifications if we're in editing mode. I suppose we should find a way to avoid doing that as well.
Yeah, any kind of popup UI could get in the way here, including dialogs too. I suppose that was one of the advantages of AwesomeBar being its own activity: the underlying browser UI couldn't interfere with it.
Also simple STR:
1. Tap the URL bar
2. Switch to another app
3. Open a link from the other app
So, there are two different cases here. One is where the current page you're on changes location (this is what I initially experienced during a session restore), and the other is where the current tab changes (what bnicholson's testcase does).

I'm having a hard time coming up with what the expected behavior should be in either of these cases. Even with the old AwesomeBar activity, there were some interaction issues if you tried entering a new URL while a page was loading. In fact, using bnicholson's testcase on Aurora, if you load that page, open the awesomescreen, and then try to load a URL, that URL is loaded in the new tab (not the one you were on when you opened the awesomescreen).

I'm thinking that the fix for this bug should be to just ensure that the HomePager is always visible while the user is in editing mode, since that will at least restore parity with the old AwesomeBar activity behavior.

However, to fix the issue of opening a link from another app, I think we'd want to exit editing mode whenever Fennec goes into the background.

Does anyone have any other thoughts on this?
I'd expect a couple of invariants while you're in editing mode:
1. You never change the target tab for the current editing mode
2. Events not triggered by the user do not dismiss editing mode

This means:

* If a new tab is opened while you're in editing mode (e.g. window.open() call), simply add the tab but do not auto-select it i.e. open the new tab in background. The target tab for the URL you entered should be the same if you commit it.
* If a new page is loaded in the current tab while you're in editing mode, don't auto-dismiss editing mode.

The case of opening a link from another app *is* triggered by the user though, so I'd expect it to auto-dismiss editing mode and auto-select any new tabs created for the external link.
(In reply to Lucas Rocha (:lucasr) from comment #12)
> I'd expect a couple of invariants while you're in editing mode:
> 1. You never change the target tab for the current editing mode
> 2. Events not triggered by the user do not dismiss editing mode
> 
> This means:
> 
> * If a new tab is opened while you're in editing mode (e.g. window.open()
> call), simply add the tab but do not auto-select it i.e. open the new tab in
> background. The target tab for the URL you entered should be the same if you
> commit it.
> * If a new page is loaded in the current tab while you're in editing mode,
> don't auto-dismiss editing mode.
> 
> The case of opening a link from another app *is* triggered by the user
> though, so I'd expect it to auto-dismiss editing mode and auto-select any
> new tabs created for the external link.

I agree with this design.
(In reply to Lucas Rocha (:lucasr) from comment #12)

> * If a new tab is opened while you're in editing mode (e.g. window.open()
> call), simply add the tab but do not auto-select it i.e. open the new tab in
> background. The target tab for the URL you entered should be the same if you
> commit it.

The difficult thing here is that this tab opening and selection happens on the Gecko side, so it would be hard to prevent switching tabs while in editing mode. Perhaps we could do something like switch the tab back if we see we're in editing mode, or keep track of which tab we're in when we open editing mode, then return to that.

I'm actually not as concerned about this case, though, because this bug existed with our AweseomeBar activity as well, so it's not a regression. I can file a follow-up for this issue, but it's enough of an edge case that I don't want to block this bug on it.
Blocks: 915918
The main issue with this bug is that we're calling hideHomePager() whenever we get a location change event, which we shouldn't do if we're in editing mode. This patch also makes us ignore the rare case that the actual about:home page loads while the user is in editing mode, since we also shouldn't call showHomePager() while in editing mode.

I filed bug 915918 as a follow-up about the case where a different tab is selected.
Attachment #804091 - Flags: review?(lucasr.at.mozilla)
This part handles dismissing editing mode if we're opening a URL from an external app.
Attachment #804092 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 804091 [details] [diff] [review]
(Part 1) Don't change the visibility of the home pager if we're in editing mode

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

Looks, just needs the suggested refactoring.

::: mobile/android/base/BrowserApp.java
@@ +208,1 @@
>                          }

This code has become a bit too chunky for a switch. Factor out this code into a separate method like updateHomePagerForTab() or something.
Attachment #804091 - Flags: review?(lucasr.at.mozilla) → feedback+
Comment on attachment 804092 [details] [diff] [review]
(Part 2) Dismiss editing mode if the user is loading a URL from an external app

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

Nice.

::: mobile/android/base/BrowserApp.java
@@ +2158,5 @@
>          }
>  
> +        // Dismiss editing mode if the user is loading a URL from an external app.
> +        if (Intent.ACTION_VIEW.equals(action)) {
> +            dismissEditingMode();

Just wondering: is there any part of dismissEditingMode() that relies on mInitialized being true? It's probably worth double-checking that.
Attachment #804092 - Flags: review?(lucasr.at.mozilla) → review+
Good idea to factor this out into a separate method.
Attachment #804091 - Attachment is obsolete: true
Attachment #804636 - Flags: review?(lucasr.at.mozilla)
(In reply to Lucas Rocha (:lucasr) from comment #18)
> Comment on attachment 804092 [details] [diff] [review]
> (Part 2) Dismiss editing mode if the user is loading a URL from an external
> app
> 
> Review of attachment 804092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice.
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +2158,5 @@
> >          }
> >  
> > +        // Dismiss editing mode if the user is loading a URL from an external app.
> > +        if (Intent.ACTION_VIEW.equals(action)) {
> > +            dismissEditingMode();
> 
> Just wondering: is there any part of dismissEditingMode() that relies on
> mInitialized being true? It's probably worth double-checking that.

Good question... dismissEditingMode returns early if mBrowserToolbar.isEditing() is false, which I would think would be the case if mInitialized is false. However, it might be a good idea to just bail early in onNewIntent if mInitialized is false, since we wouldn't be in editing mode anyway in that case.
I'm not sure if what I'm seeing is this bug, but I think I can reproduce by 

i) Visiting a site
ii) Enter Guest Mode
iii) Exit Guest Mode
iv) Tap the url-bar
v) Begin to type, and then hit back

http://cl.ly/image/16002A18230p
http://cl.ly/image/0m15070R2Q2X
Updating this patch to bail early if mInitialized is false. I saw that we added that check in bug 800238 to avoid an NPE if the app is handling the new intent during startup.
Attachment #804092 - Attachment is obsolete: true
Attachment #804782 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 804636 [details] [diff] [review]
(Part 1 v2) Don't change the visibility of the home pager if we're in editing mode

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

Excellent.
Attachment #804636 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 804782 [details] [diff] [review]
(Part 2 v2) Dismiss editing mode if the user is loading a URL from an external app

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

Great.
Attachment #804782 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 804636 [details] [diff] [review]
(Part 1 v2) Don't change the visibility of the home pager if we're in editing mode

[Approval Request Comment]
Bug caused by (feature/regressing bug #): fig merge (bug 862793)
User impact if declined: home page can be hidden in editing mode if we get location change events
Testing completed (on m-c, etc.): just landed on fx-team
Risk to taking this patch (and alternatives if risky): low-risk, small logic tweak
String or IDL/UUID changes made by this patch: none
Attachment #804636 - Flags: approval-mozilla-aurora?
Comment on attachment 804782 [details] [diff] [review]
(Part 2 v2) Dismiss editing mode if the user is loading a URL from an external app

[Approval Request Comment]
Bug caused by (feature/regressing bug #): fig merge (bug 862793)
User impact if declined: user will remain in editing mode if trying to open a link from an external app
Testing completed (on m-c, etc.): just landed on fx-team
Risk to taking this patch (and alternatives if risky): low-risk, isolated logic change
String or IDL/UUID changes made by this patch: none
Attachment #804782 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a61effc0c5c5
https://hg.mozilla.org/mozilla-central/rev/68a3dd6385c0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Verified against the steps I ran into this with
Status: RESOLVED → VERIFIED
Attachment #804782 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #804636 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.