480962 - zoom level is reset when clicking on a download link

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

If I am browsing a webpage (zoomed to read easier) and there is a link which downloads a file, I successfully download the file (in a new tab), and the zoom level is reset to the original view for the webpage that hosted the link.

Here is an example of a page that has multiple links:
https://litmus.mozilla.org/show_test.cgi?id=7217

Comment 1

[YUKI "Piro" Hiroshi]

Attachment: patch v1

By this patch, Fennec refreshes the canvas after page-loaded only when:

 * location is changed.
 * some data are transfered.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 383098 [details] [diff] [review]
patch v1

I think I see what you are doing here, but the use of "!" in your tests caused confusion for me.

first:
Browser.canvasBrowser.endLoading(!this._needToRefreshView);

then:
if (!aSkipRefreshView) {

I think the basic plan is: Only call zoomToPage() & flushRegion() if a real webpage loads.

If we get a onLocationChange or STATE_TRANSFERRING, then we must have a real webpage. _networkStart can also be fired for downloads.

If this is the case, I'd really like to think about dropping our use of _networkStart to kick off a pageload. Maybe we should be using a _doumentTransferring() function to kick off a page load.

Thoughts?

Comment 3

[Ben Combee (:bcombee)]

Also, ProgressController._needToRefreshView should be set in the constructor, not as a property on the prototype object.

(Yes, I sometimes think we should just make our singletons singletons instead of using contructors, just to make this simpler.)

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

ProgressController isn't a singleton - but in general, I agree.

Comment 5

[YUKI "Piro" Hiroshi]

Attachment: patch v1.1

Simple update.

 * _needToRefreshView => _canSkipRefresh
 * the initial value of _canSkipRefresh is set in the constructor

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: alternative patch

This patch is a little different. It uses the STATE_TRANSFERRING flag to watch for a real document load, not just a download file network sniff. But it doesn't use separate flags. It just reorganizes the way we fire loading/loaded calls.

Feedback?

Comment 7

[(dormant account)]

Comment on attachment 383552 [details] [diff] [review]
alternative patch

I'm not familiar with moz events, but empirical evidence suggests that the prior events were incorrect, so I assume this is better.

Comment 8

[YUKI "Piro" Hiroshi]

Is the _documentTransferring() called from onLocationChange() ? When I've used "Go Back" and "Go Forward" buttons, I've sometimes seen that the state flags don't have the STATE_TRANSFERRING flag. It is not set in some cases, the "not found" page and so on.

Comment 9

[YUKI "Piro" Hiroshi]

s/the "not found" page/the "Server not found" page/

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #8)
> Is the _documentTransferring() called from onLocationChange() ? When I've used
> "Go Back" and "Go Forward" buttons, I've sometimes seen that the state flags
> don't have the STATE_TRANSFERRING flag. It is not set in some cases, the "not
> found" page and so on.

You are exactly right. I wondered why you needed a flag in onLocationChange.

I'll try to find a way to get it to work, without adding another boolean flag (we have too many of those now)

Comment 11

[Antonio Gomes (tonikitoo)]

@mfinkle: checkin-needed ?

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 383552 [details] [diff] [review]
alternative patch

Not quite ready yet. It doesn't handle "back/forward" cases.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

we have a patch ready?  Could make the user experience a lot better by getting this in.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Patches aren't quite ready and now tile cache has bit rot these patches anyway

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

stuart: this is still a problem with beta3, it actually shifts the screen. 

repro:
1) go to irs.gov
2) search for "fw4 pdf"
3) there is only 1 result
4) zoom in 
5) click on the 1 link
6) observer screen weirdness...outlined below


If maximum zoom (only about 20 characters on the whole n810 screen), the download popup displays and the screen is a white background, no more text is shown.  This is the case even after the download is complete.  Tapping the screen will unzoom it and make a full display


If partial zoom is active, then we shift the page to the upper left (I assume it is the viewport, but zoomed out.  The bottom and right areas are white background and stay that way until you click on the screen.

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 401811 [details] [diff] [review]
Patch

I'm wondering if It's not better to move the networkStop code into documentStop instead?

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #18)
> (From update of attachment 401811 [details] [diff] [review])
> I'm wondering if It's not better to move the networkStop code into documentStop
> instead?

Moving this stuff around is risky and fragile. We'll need to test a lot of things

Comment 20

[Aakash Desai [:aakashd]]

I think this is a problem with links in general as I filed bug 518911 about another issue with a link that directs to another webpage.

Comment 21

[Roy Frostig :froystig]

Comment on attachment 401811 [details] [diff] [review]
Patch

If I understand correctly, there are still two potential issues with this patch:

1)  onLocationChange() is called when the hashtag is changed on a URI.  So for instance, if the browser is pointed at the URI "http://foo.org/" and then this changes to "http://foo.org/#bar", the onLocationChange() function will be called and cause us to startLoading() when, as far as I can tell, we really don't want to, as doing so might lead to unfortunate results like invalidating the viewport and doing a new zoomToPage(), etc.

2)  onLocationChange() calls startLoading() with the idea that there is still a symmetric call to endLoading() done by _networkStop().  However, onLocationChange() does not always correspond to a network action, so we startLoading(), but the corresponding endLoading() never happens.  Significant examples of such non-network onLocationChange() calls are the hashtag changing (as in (1)), and some navigations caused by forward/back history navigation (when the page to which we are navigating forward/back is fully cached by the browser).  


In general, the following issue exists:

We want to have entry points into startLoading() and endLoading() correspond to the "start of navigating to a page" and "the end of navigating to a page", respectively.  Using _networkStart() alone as an entry point for the former is no good because some network actions do not correspond to navigation to a new page (e.g. downloading a file), and using onLocationChange() alone is no good either because there are some location-changing actions that do not correspond to navigation to a new page (e.g. hashtag change).  Also note that using a version of onLocationChange() modified so that it only compares |uri.split("#")[0]| with its current value in order to decide whether a new page is being loaded is not quite right.  This is because, though this does seem like a real page navigation, it isn't necessarily one that requires network action (e.g. forward/back navigation), and so the corresponding endLoading() is not guaranteed to be called.


I don't know if Gecko can offer some more "correct" such entry points, but if it did then we already have our fix.  Otherwise, I also don't know if we are guaranteed that onLocationChange() and _networkStart() are invoked in a known order when the browser indeed navigates to a page (i.e. that it isn't just a race condition between them when that happens), but if we are guaranteed some order of execution, then I can think of a pretty straightforward fix in either case that uses both functions to determine (a) whether this is a real page change, and (b) whether it is one that actually causes network action, or one that we need to call endLoading() for right away.


Please do call me out on any of the above if I made a wrong assumption at any point.

Comment 22

[Roy Frostig :froystig]

I should clarify:  (1) and (2) are actual problems that I saw happening in trying to test the patch.  The stuff in the rest of the comment is not really an issue as much as it is me trying to understand what the _networkStart()/onLocationChange() functions are meant to handle.

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

* Don't reset the zoom level/icon on click on a download link
* It should handle back/forwark since I don't use at all the STATE_TRANSFERRIN flag

Comment 24

[Aakash Desai [:aakashd]]

I believe this bug was fixed almost a month ago. Vivien, do you have any idea which bug might have done this? I'll move this to verified once we find a bug id.

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

yeah, I have noticed this has been working as expected lately.  Would be nice to mark this as verified.

Comment 26

[Aakash Desai [:aakashd]]

Go ahead and mark it as verified if you think it should be, Joel.

Comment 27

[Jaya]

Added to litmus
https://litmus.mozilla.org/show_test.cgi?id=9844