480958 - unable to scroll content when it dynamically is expanded

Status: VERIFIED FIXED

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

Description

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

In trying to use litmus with fennec, I am unable to use it because the list of test cases are in a show/hide div.  I get the whole list of test cases when I load the page, but when I click on a test case it expands and I cannot scroll to the bottom of the page anymore.

It appears that we calculate the total amount of scrolling/panning available and when it changes dynamically the user cannot access the content which gets pushed out of the original view.

Comment 1

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

Attachment: patch

This patch removes the "pageLoading" check around the code that does our lazy height/width updates. Therefore, the code will catch DHTML changes that resize the document.

Tested with http://people.mozilla.org/~mfinkle/expand.html

(also has a few minor code format tweaks)

Comment 2

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

Comment on attachment 381546 [details] [diff] [review]
patch

This doesn't handle enough cases

Comment 3

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

Attachment: patch2

This patch works in all the tests I tried, including jmaher's (http://people.mozilla.com/~jmaher/expand/). Click the numbered list in the main frame to expand test cases. The page grows as you expand tests, and panning works. I did see that redraws were not happening until I panned, but that is a different bug.

Stuart, you might not like this patch because I had to check the content area after each MozAfterPaint event, but I only do that if _not_ pageloading, so performace won't be as badly affected.

I also changed over to a closure-wrapper used to actually retrieve the content area. This saves us 3 calls through xpconnect for each call. Also, Browser.updateViewportBounds does test that the area is different before calling the expensive WidgetStack.setViewportBounds method.

I am hoping we don't see any change in performance. I'll try testing with Taras' Fennecmark.

Comment 4

[Ben Combee (:bcombee)]

Comment on attachment 381692 [details] [diff] [review]
patch2

I like the closure technique for keeping local XPConnect pointers around.  I'll have to remember that -- could be useful in panning/kinetic code too.

Comment 5

[Roy Frostig :froystig]

Attachment: Test dynamic content expansion in both x and y directions.

Here's an additional test I made before I knew this was a bug.

I also fixed this bug independently of the above patch while overhauling CanvasBrowser.js to use the tile cache, so it should be fixed if/when we land the tile cache stuff.

The test may be useful to keep around in general.

Comment 6

[Ben Combee (:bcombee)]

An example reported from the real world is the twitter.com statuses page.  When you hit the "More" button at the bottom, the page resizes to handle the additional tweets.

Comment 7

[Roy Frostig :froystig]

As of Tile repo merge, Stuart and I discussed fixing this bug by adding a new DOM event to the <xul:browser> which notifies of changes to the content window size.  This would behave much like MozAfterPaint, and be called something like MozAfterSizeChange.

For now, we simulate this event on the frontend by fake-firing it every 2-or-so seconds during page load.  See all the blocks of code marked with a comment "!!! --- RESIZE HACK BEGIN -----" and "END" in BrowserView.js (and a small one in browser.js) to see the current workaround.  Note that this workaround doesn't fix the bug, as content that expands the page dynamically after page load (once we finish fake-firing the custom event) will not cause BrowserView to logically expand its viewport.

Comment 8

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

(In reply to comment #7)
> As of Tile repo merge, Stuart and I discussed fixing this bug by adding a new
> DOM event to the <xul:browser> which notifies of changes to the content window
> size.  This would behave much like MozAfterPaint, and be called something like
> MozAfterSizeChange.

MozAfterResize is less of a mouthful

> "!!! --- RESIZE HACK BEGIN -----" and "END" in BrowserView.js (and a small one

Please include a reference to this bug number in the comment:

Comment 9

[Roy Frostig :froystig]

Attachment: Patch v1

Patch dependent on patch in bug 514732.

I wrote this a while ago (around when I made Patch v2 in bug 514732) and tested it side-by-side then.  Since then, this patch bitrotted a bit, so this is an untested refresh.

Comment 10

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

this is a blocking-fennec: 1.0b3+, will this get fixed before we ship 1.0?  I just want to make sure our queries account for this bug and other bug with an older blocking-fennec flag.

Comment 11

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

this page is also failing for me:
http://msdn.microsoft.com/en-us/library/ms682425%28VS.85%29.aspx

Also the payment page in amazon.com checkout has a problem.

Comment 12

[Roy Frostig :froystig]

(In reply to comment #11)

With or without both patches?

Comment 13

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

sorry, no patches, just a 1.9.2 20090924 nightly build on my n810

Comment 15

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

this is the case for verifying a security exception on https://verisign.com

Comment 16

[Benjamin Stover (:stechz)]

Comment on attachment 402528 [details] [diff] [review]
Patch v1

>   getBrowserDimensions: function getBrowserDimensions(browser) {

Is there a need for this fn anymore?  It can be expensive and we get it for free with the new event.  It's used for zoomToFit, but on maps.google.com for instance it returns old size values (I don't know why).  If zoomToFit just uses the viewport to determine its new size maps.google.com renders correctly.

>+    this._viewportChanged(dimsChanged, dimsChanged && !!causedByZoom);

Logically, !(!(p))=p, so why is this here?  If this is a clever conversion to boolean, it looks unnecessary.  Also, if the viewport dimensions didn't change, why is _viewportChanged called?  It seems odd the _viewportChanged takes the size changed parameter at all, come to think of it.

>+      if (!internalSet)
>+        this.zoomChanged = true;

I couldn't find where internalSet is ever true.  Why do we have this parameter?

>-      // !!! --- RESIZE HACK BEGIN -----
>-      // change to the real event type and perhaps refactor the handler function name
>-      this._browser.removeEventListener("FakeMozAfterSizeChange", this.handleMozAfterSizeChange, false);
>-      // !!! --- RESIZE HACK END -------

It is awesome to see this particular part of the diff :)

>+    if (x < 0) w += x;
>+    if (y < 0) h += y;

Could you explain this part?

Comment 17

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

Comment on attachment 402528 [details] [diff] [review]
Patch v1

Just some nits:

>+    let dimsChanged = (oldwidth != width || oldheight != height);

"sizeChanged" 

"dimsChanged" isn't readable enough (yes, we're fixing "rite" as well)

>+  setZoomLevel: function setZoomLevel(zl, internalSet) {

can we fix "zl" -> "zoomLevel" while we're here? and "internalSet" -> "internalChange", although "userChange" would be nicer and clearer, but will change the logic

>-    let browserChanged = (this._browser !== browser);
>+    let oldBrowser = this._browser;

Why do we need "oldBrowser" at all? this._browser is the oldBrowser

>-    // !!! --- RESIZE HACK END -------
>-    this.setViewportDimensions(this.browserToViewport(w), this.browserToViewport(h));
>+    let browser = this._browser;
>+    let [scrollX, scrollY] = BrowserView.Util.getContentScrollValues(browser);

is "browser" really needed?

>     if (this == Browser.selectedTab) {
>-      // !!! --- RESIZE HACK BEGIN -----
>-      bv.simulateMozAfterSizeChange();
>-      // !!! --- RESIZE HACK END -----
> 
>       let restoringPage = (this._state != null);

Remove the blank line. We don't need that starting the if block

Comment 18

[Roy Frostig :froystig]

(In reply to comment #16)
> (From update of attachment 402528 [details] [diff] [review])
> >   getBrowserDimensions: function getBrowserDimensions(browser) {
> 
> Is there a need for this fn anymore?  It can be expensive and we get it for
> free with the new event.  It's used for zoomToFit, but on maps.google.com for
> instance it returns old size values (I don't know why).  If zoomToFit just uses
> the viewport to determine its new size maps.google.com renders correctly.

No active need / use of this function any longer AFAIK.  Kept it around with a warning comment because I was being conservative without an extremely good reason.  It isn't actually entirely consistent with the event's values, so probably better to throw it out.

> 
> >+    this._viewportChanged(dimsChanged, dimsChanged && !!causedByZoom);
> 
> Logically, !(!(p))=p, so why is this here?  If this is a clever conversion to
> boolean, it looks unnecessary.

I prefer not to pass undefined when I mean false --- undefined tells me the variable doesn't exist, e.g. optional parameter not given.  The variable causedByZoom is an optional parameter, so it may be undefined.  JS doesn't convert things in a boolean chain to actual booleans, so |true && undefined| evaluates to |undefined|.

> Also, if the viewport dimensions didn't change,
> why is _viewportChanged called?  It seems odd the _viewportChanged takes the
> size changed parameter at all, come to think of it.

Something can change about the viewport that isn't it's size dimensions' values.  One example is that the visible rect may have changed.

> >+      if (!internalSet)
> >+        this.zoomChanged = true;
> 
> I couldn't find where internalSet is ever true.  Why do we have this parameter?

Mmmm, I think this is just residue from stuff I was doing in earlier phases of this patch.  Oops.

> 
> >-      // !!! --- RESIZE HACK BEGIN -----
> >-      // change to the real event type and perhaps refactor the handler function name
> >-      this._browser.removeEventListener("FakeMozAfterSizeChange", this.handleMozAfterSizeChange, false);
> >-      // !!! --- RESIZE HACK END -------
> 
> It is awesome to see this particular part of the diff :)

Aye. :)

> 
> >+    if (x < 0) w += x;
> >+    if (y < 0) h += y;
> 
> Could you explain this part?

This is due to how, in theory, the page could grow in the "negative" directions from the point that we consider the origin in BrowserView.  BrowserView (and I believe Firefox does this in a general sense as well, though I could be wrong) displays only the "positive" quadrant of the page, and this is an assumption under which the implementation of the TileCache operates.  These adjust width and height from the incoming event properties so that we ignore changes to width and height contributes by growth in quadrants other than x > 0 && y > 0.

Comment 19

[Roy Frostig :froystig]

(In reply to comment #17)
> >-    let browserChanged = (this._browser !== browser);
> >+    let oldBrowser = this._browser;
> 
> Why do we need "oldBrowser" at all? this._browser is the oldBrowser

We're using it numerous times in the following few lines.  Cache it in a local variable instead of repeatedly looking up the same property on |this|.


Will fix the rest when addressing Stover's comments.

Comment 20

[Roy Frostig :froystig]

Attachment: Patch v2

Address Ben Stover and Mark Finkle's comments.

Patch still untested.  I need to get a build going in order to test it, but someone else may want to join me in trying some of the many affected webpages.

Comment 21

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

This patch doesn't seem to work completely. When a page is expanded, the view is resized, but the tiles are not rendered correctly.

The test page I was using is the "Page Loading Error" page. Enter "https://mozilla.org" into the URLBar to make it load. Then use the expand/collapse sections to test.

Comment 22

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

Testing the attachment yields a correct resize and partially rendered tiles. It looks like the non-dirty tiles were not cleared when the page expanded, so I am seeing previous (firstrun) artifacts.

Comment 23

[Roy Frostig :froystig]

Good catch.  I'll take a look this coming Friday.

Comment 24

[Roy Frostig :froystig]

Attachment: Patch v3

Alright.  Here it is --- tested and everything.

A few long-standing bugs were revealed by having torn out some of the code as per the original purpose of this patch (as mentioned in previous comments, bye bye to "!!! RESIZE HACK"), but fixed along the way.  These are mainly things that were previously being hidden (read: magically taken care of) by the fact that BrowserView/TileManager were invalidating much more than was really needed whenever viewport dimensions changed.  As of this patch, we only invalidate the entire viewport's worth of tiles when it is necessary, i.e. on one of three occasions: (1) Zoom, (2) Navigate to new page, and (3) MozAfterPaint gives us a rect the size of the viewport.

For the interested, I recommend taking a look at the big comment in BrowserView.js documenting the workaround I put in to account for how, as it turns out, MozAfterPaint is not always sufficient for us in invalidating an entire page upon new page load.

(In somewhat short form: MozAfterPaint makes good guarantees about paint notifications inside what it thinks is the visible region, but weaker guarantees on paint notifications for things going on outside that region (list at https://developer.mozilla.org/en/Gecko-Specific_DOM_Events#Important_notes).  This turns out to leave holes in some of the new pages we transition to, as we don't get a set of paint invalidate notifications that cover the entire new page).

... And it's still Friday. :)

Comment 25

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

Comment on attachment 406840 [details] [diff] [review]
Patch v3

Looks good and works well.

The only problem I saw was with the certificate exception error page.

Go to https://mozilla.org and you will see the error page. It has two expandable sections at the bottom.

Pan down and you'll see some drawing problems. Expand/collapse the sections and you might see problems too.

This patch wells extremely well for all the web content cases I tested.

r+ and we can file a followup bug for the exception page problems.

Comment 26

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

isn't the ssl exceptions page chrome?

Comment 27

[Juha Seppänen]

Another URL to test with would https://verisign.com

Comment 28

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

thank you roy!

pushed:
https://hg.mozilla.org/mobile-browser/rev/d7623fd1ad70

Comment 29

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

ugh, I found a regression when loading planet.mozilla.org, panning a few times, clicking a link, then going "back" to planet.

I am now unable to pan to the bottom of planet.

Comment 30

[Axel Linder]

Regarding the regression I noticed that the panning restriction might have to do with the size of the site which has been visited previously. Dumping the size of the viewport with the following use case shows the effect described by Mark...


Browsing to planet.mozilla.org for the first time works beautifully...

[BrowserViewportState] {
	viewportRect=[0,0,800,94688.96875],

location from Browser: http://planet.mozinterns.net/
location from BV     : http://planet.mozinterns.net/


Switching to e.g. www.google.de (smaller site) shows a size to large for the normal google startpage...

[BrowserViewportState] {
	viewportRect=[0,0,799.9531999999999,80022.21355624999],

location from Browser: http://www.google.de/
location from BV     : http://www.google.de/


Switching back to planet.mozilla.org shows the size of the ordinary google site and now panning is blocked as described by Mark!

[BrowserViewportState] {
	viewportRect=[0,0,785.68,471.408],

location from Browser: http://planet.mozinterns.net/
location from BV     : http://planet.mozinterns.net/
 

Being on a larger site before mozilla.org I didn't experience this.

Probably this might be of help anyhow...

Comment 31

[Roy Frostig :froystig]

Yeah.  From my conversation to myself in IRC (with some added):

01:24 < froystig> the reason mfinkle was seeing the bug was because:
01:24 < froystig> 1) back/forward sometimes doesn't cause reflow on the page to which you are navigating back/forward to (if it was well-cached)
01:24 < froystig> 2) therefore the MozScrolledAreaChanged event doesn't fire, so BrowserView paints the page, but under the viewport dimensions and zoomLevel of the page from which we just did a back/forward.
01:27 < froystig> 3) therefore it would be wise to, as part of the BrowserViewportState in BrowserView, hold the values of all viewport dimensions and zoomlevels corresponding to every page in the navigation history of that browser (so that viewport sizes can be restored on back/forward navigations despite not having a new event come in (since we've seen the event already at least once))
01:30 < froystig> 4) therefore i need to know about when such a 'back' or 'forward' navigation occurs, so that i can restore the right set of values.  having a notification of such a back/forward come from the <browser> (with, say, an int corresponding to how many pages we went forward or back) would be useful.

Comment 32

[Benjamin Stover (:stechz)]

Keeping a history of all the viewport dimensions feels like a hack to me.  I would expect MozScrolledAreaChanged to be fired even on a cached page.  If that's a big project, let's cache the dimensions and file a new bug for a rainy day.

Caching the entire viewport would have the bonus of correctly panning/zooming to the correct position when pressing back/forward (see bug 496644).

Comment 33

[Roy Frostig :froystig]

Actually, I started writing a m-c patch earlier today that issues MozScrolledAreaChanged when a cached page is restored from history, but forgot to mention it here.  I think I'm close to done, so hopefully that takes care of everything in this bug.


Even though that will fix this bug, Ben brings up a point in how it could be a nice feature to cache viewport data of pages in navigation history so they can be restored along with everything else on the page.  We can, in general, cache viewport data corresponding to pages in the bfcache ("back/forward cache") by setting a "OnPageShow" and "OnPageHide" listeners on the <browser> given to the BrowserView, which would react when the |persistent| property of the listeners' event is set |true| (see http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/events/nsIDOMPageTransitionEvent.idl).  There is currently a catch: there is no mechanism by which we can get notified of bfcache page evictions, so we don't know when to evict from our own corresponding cache.  Hence we grow an arbitrarily large data structure over time.  I think that if and when that is fixed, we can have the functionality Ben mentioned in Comment #32.  Perhaps good material for a followup bug.

Comment 34

[Roy Frostig :froystig]

Attachment: Patch v4

OK.  So this one fixes the issues with pages that are restored from the bfcache, which was the issue that caused the regression seen above.  It doesn't do much on its own, but rather depends heavily on (and reacts correctly to) attachment 408255 [details] [diff] [review] that I posted in bug 514732.

Comment 35

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.5, updated to apply

Comment 36

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 409779 [details] [diff] [review]
patch v.5, updated to apply

carrying mfinkle's review

Comment 37

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mobile-browser/rev/3d8f7d7b2fc7

Comment 38

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b2pre) Gecko/20091104 Firefox/3.6b2pre Fennec/1.0b5

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091104 Firefox/3.7a1pre Fennec/1.0b5