1184912 - Zoomed view stays open if I didn't close it and I switch apps

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

STR:

Trigger zoomed view
Leave it open
Switch apps
Open a link with Firefox
Zoomed view is still open from last time

I hope it's just me!

Comment 1

[Dominique Vincent [:domivinc]]

The zoomed view is stopped when the event "Content:LocationChange" is received.
It looks like we never received this event in your user case.
I'm going to fix this issue.

Comment 2

[Dominique Vincent [:domivinc]]

Attachment: patch-19072015 2-Bug_1184912___Close_zoomed_view_on_tab_change__r_mcomella.patch

I tried to cover all the different cases: new tab, selection of another existing tab.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8635743 [details] [diff] [review]
patch-19072015 2-Bug_1184912___Close_zoomed_view_on_tab_change__r_mcomella.patch

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

::: mobile/android/base/BrowserApp.java
@@ +2057,5 @@
>      }
>  
>      @Override
>      public void addTab() {
> +        stopZoomDisplayIfVisible();

We should only need onTabChanged -> case SELECTED because when a tab is added to the foreground, it is selected, when a tab is added to the background (e.g. popup via page content script), it shouldn't be selected and we don't want to close the zoomed view, or when the current tab is removed, it is selected.

@@ +2063,5 @@
>      }
>  
>      @Override
>      public void addPrivateTab() {
> +        stopZoomDisplayIfVisible();

As above, we shouldn't need this.

@@ +2289,5 @@
>          if (oldTab != null) {
>              oldTab.setIsEditing(false);
>          }
>  
> +        stopZoomDisplayIfVisible();

This calls Tabs.selectTab, which calls notifyListeners on SELECTED so you should just be able to leave the reference in BrowserApp.onTabChanged.

::: mobile/android/base/ZoomedView.java
@@ +500,5 @@
>  
>          moveUsingGeckoPosition(leftFromGecko, topFromGecko);
>      }
>  
> +    public void stopZoomDisplayIfVisible() {

Why is this wrapper necessary? Can we just check if the view is visible in stopZoomDisplay directly? How does this interact with your work in bug 1184762 where we check for visibility in hideZoomedView?

Comment 4

[Dominique Vincent [:domivinc]]

Attachment: patch-20072015 2-Bug_1184912___Close_zoomed_view_on_tab_change__r_mcomella.patch

All changes done, patch of bug 1184762 updated.

Comment 5

[Dominique Vincent [:domivinc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=779a5bd0de2a

Comment 6

[Carsten Book [:Tomcat]]

Hi, this patch failed to apply:

renamed 1184912 -> patch-20072015_2-Bug_1184912___Close_zoomed_view_on_tab_change__r_mcomella.patch
applying patch-20072015_2-Bug_1184912___Close_zoomed_view_on_tab_change__r_mcomella.patch
patching file mobile/android/base/ZoomedView.java
Hunk #1 FAILED at 495
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/ZoomedView.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh patch-20072015_2-Bug_1184912___Close_zoomed_view_on_tab_change__r_mcomella.patch

can you take a look, thanks!

Comment 7

[Dominique Vincent [:domivinc]]

Attachment: patch-27072015 1-Bug_1184912___Close_zoomed_view_on_tab_change__r_mcomella.patch

Same patch re-based.

Comment 8

[Dominique Vincent [:domivinc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0b15b62a550

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ff9265c2d8a5

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ff9265c2d8a5