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

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: antlam, Assigned: domivinc)

Tracking

unspecified
Firefox 42
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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!
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.
Assignee: nobody → domivinc
I tried to cover all the different cases: new tab, selection of another existing tab.
Attachment #8635743 - Flags: review?(michael.l.comella)
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?
Attachment #8635743 - Flags: review?(michael.l.comella) → review-
All changes done, patch of bug 1184762 updated.
Attachment #8635743 - Attachment is obsolete: true
Attachment #8637140 - Flags: review?(michael.l.comella)
Depends on: 1184762
Attachment #8637140 - Flags: review?(michael.l.comella) → review+
Keywords: checkin-needed
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!
Flags: needinfo?(domivinc)
Keywords: checkin-needed
Same patch re-based.
Flags: needinfo?(domivinc)
Keywords: checkin-needed
Attachment #8637140 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ff9265c2d8a5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.