Closed Bug 1038354 Opened 5 years ago Closed 5 years ago

Switch to tab toast doesn't work in editing mode

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 35
Tracking Status
firefox32 --- unaffected
firefox33 --- wontfix
firefox34 --- verified
firefox35 --- verified
fennec 33+ ---

People

(Reporter: bnicholson, Assigned: nalexander)

References

Details

(Keywords: regression, reproducible)

Attachments

(3 files, 2 obsolete files)

STR:
1) Open a private browsing tab
2) Click the URL bar
3) Long press a top site thumbnail
4) Choose Open in New Tab or Open in Private Tab
5) Click the "Switch" button on the toast

After these STR, nothing happens. We should switch to the opened tab.
tracking-fennec: ? → 33+
This is not actually a private-browsing interaction at all!  It is revealed there because when you open a fresh private browsing tab, the only thing to do is to focus the URL bar.  In a regular tab, you can interact with the home fragment before focusing the URL bar.  (That works.)

However, if you focus the URL bar, and then interact with the home fragment (from a regular or private tab), the tab switch fails.  There's some home fragment state to consider here.  Adding lucasr and margaret for their home fragment knowledge.

That is, the new STR are Brian's original, but it's not important if the tab is private.
Summary: Switch to tab toast doesn't work in private browsing → Switch to tab toast doesn't work in editing mode
So, this quick hack fixes the problem.  We could codify this by
extracting an interface (like OnUrlOpen...) and saving the BrowserApp
in HomeFragment.onViewCreated or similar.  I'm not sure what is the
right way to dismiss edit mode.
Attachment #8458333 - Flags: feedback?(margaret.leibovic)
Attachment #8458333 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 8458333 [details] [diff] [review]
Exit editing mode before quick-switching to tab.

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

Looks fine but the direct coupling between BrowserApp and HomeFragment is a bit unfortunate.

::: mobile/android/base/BrowserApp.java
@@ +1573,5 @@
> +        // mode exit) in lieu of the tab we are about to select.
> +        mTargetTabForEditingMode = null;
> +
> +        final Tabs tabs = Tabs.getInstance();
> +        tabs.selectTab(tabId);

nit: Tabs.getInstance().selectTab(tabId) to be more concise?

@@ +1575,5 @@
> +
> +        final Tabs tabs = Tabs.getInstance();
> +        tabs.selectTab(tabId);
> +
> +        mBrowserToolbar.cancelEdit();

I'd probably cancel edit mode before selecting the new tab as we use the edit mode state to handle about:home visibility when a new tab is selected, see:

http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#239

::: mobile/android/base/home/HomeFragment.java
@@ +206,5 @@
>                      R.drawable.switch_button_icon,
>                      new ButtonToast.ToastListener() {
>                          @Override
>                          public void onButtonClicked() {
> +                            ((BrowserApp) getActivity()).switchToTab(newTabId);

This is directly coupling HomeFragment with BrowserApp which is not ideal. But it seems some previous change has already coupled HomeFragment with GeckoApp :-/ I would probably have added some interface for handling this action (i.e. create the toast and handling the button click) outside HomeFragment. We can talk about this in a follow-up if you prefer.

Strictly speaking, the type cast above should actually use BrowserApp instead of GeckoApp because HomeFragment is a BrowserApp-specific component.
Attachment #8458333 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(In reply to Lucas Rocha (:lucasr) from comment #3)
> Comment on attachment 8458333 [details] [diff] [review]
> Exit editing mode before quick-switching to tab.
> 
> Review of attachment 8458333 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine but the direct coupling between BrowserApp and HomeFragment is a
> bit unfortunate.
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +1573,5 @@
> > +        // mode exit) in lieu of the tab we are about to select.
> > +        mTargetTabForEditingMode = null;
> > +
> > +        final Tabs tabs = Tabs.getInstance();
> > +        tabs.selectTab(tabId);
> 
> nit: Tabs.getInstance().selectTab(tabId) to be more concise?
> 
> @@ +1575,5 @@
> > +
> > +        final Tabs tabs = Tabs.getInstance();
> > +        tabs.selectTab(tabId);
> > +
> > +        mBrowserToolbar.cancelEdit();
> 
> I'd probably cancel edit mode before selecting the new tab as we use the
> edit mode state to handle about:home visibility when a new tab is selected,
> see:
> 
> http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> java#239

In this case we should update the maybeSwitchToTab logic as well:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1599

If we are adding a new helper method for switching tabs, we should probably just use it inside the maybeSwitchToTab implementation.

> ::: mobile/android/base/home/HomeFragment.java
> @@ +206,5 @@
> >                      R.drawable.switch_button_icon,
> >                      new ButtonToast.ToastListener() {
> >                          @Override
> >                          public void onButtonClicked() {
> > +                            ((BrowserApp) getActivity()).switchToTab(newTabId);
> 
> This is directly coupling HomeFragment with BrowserApp which is not ideal.
> But it seems some previous change has already coupled HomeFragment with
> GeckoApp :-/ I would probably have added some interface for handling this
> action (i.e. create the toast and handling the button click) outside
> HomeFragment. We can talk about this in a follow-up if you prefer.
> 
> Strictly speaking, the type cast above should actually use BrowserApp
> instead of GeckoApp because HomeFragment is a BrowserApp-specific component.

I agree with lucasr that this coupling of HomeFragment and BrowserApp isn't great. It looks like the only existing GeckoApp dependency comes from using the button toast, and it would be nice to see a better interface for accessing that (rather than directly getting the widget from GeckoApp), but we could address that in a follow-up.
Attachment #8458333 - Flags: feedback?(margaret.leibovic) → feedback+
This moves the relevant functionality into an interface (implemented
by BrowserApp), which helps reduce the coupling between the
HomeFragments and the rest of the App lifecycle.

I agonized about adding a new interface versus extending an existing
one, and finally decided that I am not the right person to extract the
unified interface (at least, not right now).

Finally, I will be PTO until mid-August.  If one of you can shepherd
this through landing, and then possibly uplift to Aurora, that would
be much appreciated.
Attachment #8458333 - Attachment is obsolete: true
Attachment #8458922 - Attachment is obsolete: true
Attachment #8460406 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8460406 [details] [diff] [review]
Exit editing mode before quick-switching to tab. r=lucasr

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

I wonder if we could do this by tracking background tabs being added directly in Tabs instead of tying this code specifically to HomePager?
Attachment #8460406 - Flags: review?(lucasr.at.mozilla)
Lucas, do you want to help wrap up this patch, since Nick isn't going to be back until August 11?

I can also take it on, but you seem to have ideas about a different way to approach this :)
Flags: needinfo?(lucasr.at.mozilla)
Ok, I'll have a look at it next week.
Assignee: nobody → lucasr.at.mozilla
Flags: needinfo?(lucasr.at.mozilla)
Status: NEW → ASSIGNED
Oh dear, this slipped up to Beta.  I'll try to move on this.
Duplicate of this bug: 1064310
Assignee: lucasr.at.mozilla → nalexander
Keywords: reproducible
Poke from triage.
Flags: needinfo?(nalexander)
I'm not exactly sure what the issue is regarding this landing, nor *all* the coupling issues, but I do want to note that editing mode does make a lot of assumptions that state will not change while it's open (likely including selected tab state). I will likely de-couple this in bug 1058902, but note that this is in no rush to land or be uplifted. That being said, after it lands, I'll have some more knowledge with which to work on this bug.
OK, I'll take this and see if I can get something moving.
Flags: needinfo?(nalexander)
Here's the plan: land the rebased original patch on Nightly, r=lucasr.  (No need for re-review, in my opinion.)  I've tested this locally, but we'll get a QA pass.

The Nightly patch applies, with offsets, to Aurora.  Beta requires some love, since refactorings landed; but I have it locally updated and will attach.  I built and locally tested Aurora and Beta, but we'll want QA here too.
Approval Request Comment
[Feature/regressing bug #]: I think it's always been there.

[User impact if declined]: poor user experience when using a nice feature on the Home Panel.  Open in new [private] tab -> click switch icon will do nothing.

[Describe test coverage new/current, TBPL]: tested locally.

[Risks and why]: late in the cycle, stalled patch being landed late.  (Due to PTO and some pretty minor futzing with approach that wasn't worthwhile.)

[String/UUID change made/needed]: none.
Attachment #8498658 - Flags: review+
Attachment #8498658 - Flags: approval-mozilla-aurora?
Approval Request Comment
[Feature/regressing bug #]: I think it's always been there.

[User impact if declined]: poor user experience when using a nice feature on the Home Panel.  Open in new [private] tab -> click switch icon will do nothing.

[Describe test coverage new/current, TBPL]: tested locally.

[Risks and why]: late in the cycle, stalled patch being landed late.  (Due to PTO and some pretty minor futzing with approach that wasn't worthwhile.)  Some possibility of completely screwing the logic and throwing an exception or similar in weird edge cases that I can't predict.

[String/UUID change made/needed]: none.
Attachment #8498660 - Flags: review+
Attachment #8498660 - Flags: approval-mozilla-beta?
Comment on attachment 8498660 [details] [diff] [review]
1038354.beta.patch

We are so late in the cycle (beta 9 go to build is today and no mobile version), the patch is big and touches important files, we have this bug for a while, I think this should not be uplifted in beta but aurora could take it!
Attachment #8498660 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Keywords: qawanted
Tested on latest fx-team build:
* Issue does not reproduce anymore
* Performed different scenarios and Switch to tab functionality is working fine
https://hg.mozilla.org/mozilla-central/rev/e5c4d68db526
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
(In reply to Mihai Pop from comment #20)
> Tested on latest fx-team build:
> * Issue does not reproduce anymore
> * Performed different scenarios and Switch to tab functionality is working
> fine

Thanks for such qa turn-around.  It's always appreciated.
(In reply to Nick Alexander :nalexander from comment #22)
> (In reply to Mihai Pop from comment #20)
> > Tested on latest fx-team build:
> > * Issue does not reproduce anymore
> > * Performed different scenarios and Switch to tab functionality is working
> > fine
> 
> Thanks for such qa turn-around.  It's always appreciated.

Argh, *quick* qa turn-around.
Verified as fixed in build 35.0a1 (2014-10-03);
Device: Nexus 7 (Android 4.4.4).
Keywords: qawanted
Ping for the Aurora patch
Flags: needinfo?(sledru)
Flags: needinfo?(lmandel)
Comment on attachment 8498658 [details] [diff] [review]
1038354.aurora.patch

Aurora+
Attachment #8498658 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(sledru)
Flags: needinfo?(lmandel)
Verified as fixed on
Build: Firefox for Android 34 Beta 1
Device: Asus Transformer Pad TF300T (Android 4.2.1)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.