Closed Bug 480962 Opened 11 years ago Closed 10 years ago

zoom level is reset when clicking on a download link

Categories

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

defect
Not set

Tracking

(fennec1.0+)

VERIFIED FIXED
Tracking Status
fennec 1.0+ ---

People

(Reporter: jmaher, Assigned: vingtetun)

References

Details

Attachments

(3 files, 2 obsolete files)

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
Attached patch patch v1 (obsolete) — Splinter Review
By this patch, Fennec refreshes the canvas after page-loaded only when:

 * location is changed.
 * some data are transfered.
Attachment #383098 - Flags: review?
Attachment #383098 - Flags: review? → review?(mark.finkle)
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?
Attachment #383098 - Flags: review?(mark.finkle) → review-
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.)
ProgressController isn't a singleton - but in general, I agree.
Attached patch patch v1.1Splinter Review
Simple update.

 * _needToRefreshView => _canSkipRefresh
 * the initial value of _canSkipRefresh is set in the constructor
Attachment #383098 - Attachment is obsolete: true
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?
Attachment #383552 - Flags: review?(tglek)
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.
Attachment #383552 - Flags: review?(tglek) → review+
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.
s/the "not found" page/the "Server not found" page/
(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)
@mfinkle: checkin-needed ?
Comment on attachment 383552 [details] [diff] [review]
alternative patch

Not quite ready yet. It doesn't handle "back/forward" cases.
Attachment #383552 - Flags: review+
we have a patch ready?  Could make the user experience a lot better by getting this in.
tracking-fennec: --- → ?
Patches aren't quite ready and now tile cache has bit rot these patches anyway
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.
Flags: in-litmus?
tracking-fennec: ? → 1.0+
Duplicate of this bug: 512903
Comment on attachment 401811 [details] [diff] [review]
Patch

I'm wondering if It's not better to move the networkStop code into documentStop instead?
Attachment #401811 - Flags: review?(mark.finkle)
(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
Assignee: nobody → 21
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 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.
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.
Attached patch Patch v0.2Splinter Review
* 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
Attachment #401811 - Attachment is obsolete: true
Attachment #408644 - Flags: review?(mark.finkle)
Attachment #401811 - Flags: review?(mark.finkle)
Attachment #408644 - Flags: review?(mark.finkle) → review?(froystig)
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
yeah, I have noticed this has been working as expected lately.  Would be nice to mark this as verified.
Go ahead and mark it as verified if you think it should be, Joel.
Status: RESOLVED → VERIFIED
Added to litmus
https://litmus.mozilla.org/show_test.cgi?id=9844
Flags: in-litmus? → in-litmus+
Component: General → Panning/Zooming
You need to log in before you can comment on or make changes to this bug.