483425 - bandaid visual jarring when clicking on link

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: hack to paint white

Right now when we click on a link, there's a panTo(0, -60) which shows the urlbar, but as a consequence it also moves the canvas to the top, forcing a drawWindow.

Here's a quick bandaid patch that just has us paint white until we've received the first MozAfterPaint for the new page.  This also potentially makes page loads faster, because we don't have to pay the cost of doing the full repaint as the first thing after clicking a link that's not at the top of some page.

Ideally, on clicking a URL we'd pop the urlbar into place without moving the visible document until we actually had something to render for the new one, at which point we'd pan back to the top and unfreeze the urlbar.  I tried doing this, but it's much trickier than this.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated

It works better if you qrefresh.

Comment 2

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

Comment on attachment 367416 [details] [diff] [review]
updated

>diff --git a/chrome/content/CanvasBrowser.js b/chrome/content/CanvasBrowser.js
>   flushRegion: function flushRegion(viewingBoundsOnly) {
>+    if (this._pageWaitingForFirstPaint) {
>+      // how did we get here?
>+      let ctx = this._canvas.getContext("2d");
>+      this.clearRegion();
>+      ctx.fillStyle = "white";
>+      ctx.fillRect(0, 0, this._canvas.width, this._canvas.height);
>+      return;
>+    }
>+

We use the same lines of code in CanvasBrowser.startLoading - can you make a simple helper function?

Comment 3

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

Comment on attachment 367416 [details] [diff] [review]
updated

>diff --git a/chrome/content/CanvasBrowser.js b/chrome/content/CanvasBrowser.js

>   flushRegion: function flushRegion(viewingBoundsOnly) {
>+    if (this._pageWaitingForFirstPaint) {
>+      // how did we get here?
>+      let ctx = this._canvas.getContext("2d");
>+      this.clearRegion();
>+      ctx.fillStyle = "white";
>+      ctx.fillRect(0, 0, this._canvas.width, this._canvas.height);
>+      return;
>+    }
>+

Thinking more about this, shouldn't the canvas be white already? we should hit .startLoading first, which paints the canvas white. This patch sets ._pageWaitingForFirstPaint = true in .startLoading which should mean .flushRegion is just painting white on white.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep, the issue is that whatever it paints is actually just contributing to the problem.  What happens is:

in browser.js networkStart():
- calls canvasBrowser.startLoading()
  - page is painted white
  * user at this point sees the page turn white, since an invalidate happens for that paint
- calls BrowserUI.update(TOOLBARSTATE_LOADING)
  - sets UIMODE_URLVIEW (which hides the urlbar??)
  - calls panTo(0, -60) to pan to the start and make the urlbar visible
    - panTo pans to the top, and has to redraw the canvas, so calls its redraw handler
      - redraw handler eventually gets into CanvasBrowser and calls flushRegion
        - flushRegion calls drawWindow to repaint the top left part of the old page, because we haven't received a new document yet (STATE_DOCUMENT & START, I think)
        * user at this point sees the top left of the page they were just on get rendered
- new page finally starts loading, MozAfterPaint is issued
  * user sees content from new page

So to the user, right now it looks like this:

* user clicks on link on page A to go to page B
* page flashes white
* the top of page A is drawn (looks visually like a scroll to the top)
* after a second or two the initial parts of page B are drawn (due to the loading paint delay)

The easy fix here changes this to:

* user clicks on link on page A to go to page B
* page turns white, urlbar visible
* new page paints

The checkerboard could work, but it would have to be painted directly onto the canvas (since the canvas is not transparent).  However, as is the checkerboard gives me a headache -- the checkers are too small and too contrasty :)

The better fix would have this become:

* user clicks on link on page A to go to page B
* urlbar drops down with throbber, but page doesn't move or change; possibly the link that was just clicked stays highlighted with the nice outline
* when new page has content ready to paint, it just shows up there from the top of the new page, though internally we'd scroll back to the top and unfreeze the urlbar

Comment 5

[(dormant account)]

Attachment: my approach

Vlad, that still does a pointless pan which is quite cpu-hungry, esp if you are at a bottom of a page.

My patch is a WiP, as this should be done in a transaction to save a few viewporthandler calls(atm one is called for panning another for zoom).

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep, that would work, except that with that the URL bar isn't visible while you're loading so the user has no indication that a load is taking place.  That's probably bad (getting the urlbar visible was the reason that the pan was there originally).

Comment 7

[(dormant account)]

Attachment: closer to being done

I don't think we need to do fillRect at all. As far as I can tell pageBounds is always the size of bounds in viewportHandler is always constant and it always corresponds to canvas size.

The page-change behavior is much nicer now. You click on  link, it gets highlighted, then the page changes to the  new page.

There is still the occasional "false" mozafterpaint event that is obvious when one zooms out on the firstrun page, then uses awesomebar to go to another page. Not sure what's causing that to fire. I'm guessing that it has something to do with a focus-change, but i'm not sure.

Comment 8

[(dormant account)]

> There is still the occasional "false" mozafterpaint event that is obvious when
> one zooms out on the firstrun page, then uses awesomebar to go to another page.
> Not sure what's causing that to fire. I'm guessing that it has something to do
> with a focus-change, but i'm not sure.

I'm guessing we could work-around this by preventing zoomtopaging during page load UNTIL a few bytes have been received, but that seems like the wrong way to fix this.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #7)
> The page-change behavior is much nicer now. You click on  link, it gets
> highlighted, then the page changes to the  new page.

Where's the URL bar during this process?

Comment 10

[(dormant account)]

(In reply to comment #9)
> (In reply to comment #7)
> > The page-change behavior is much nicer now. You click on  link, it gets
> > highlighted, then the page changes to the  new page.
> 
> Where's the URL bar during this process?

Thanks, forgot to mention that I didn't do that part and was hoping we'd work out a better behavior for the urlbar.
With the patch it either shows the urlbar if it's already is shown or doesn't if it isn't.

Comment 11

[(dormant account)]

I propose that we "freeze" the urlbar onto screen with _showToolbar during pageloads.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep, I think that's what we should do as well; freezing it into place is easy, I just wasn't sure when we should unfreeze it.

Comment 13

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

(In reply to comment #11)
> I propose that we "freeze" the urlbar onto screen with _showToolbar during
> pageloads.

[more or less notes for me]

That was the original intent of the | ws.panTo(0, -60, true); | in browser-ui.js

The patch pans to 0,0 - which itself won't show the urlbar. Freezing it into view will float it over the content, obscuring it. We could freeze in startLoading and unfreeze in endLoading?

(In reply to comment #10)
> With the patch it either shows the urlbar if it's already is shown or doesn't
> if it isn't.

Really? the ws.PanTo(0, 0) would seem to always move the urlbar out of view. Do you mean that if the urlbar is frozen its visible, unfrozen it's not visible?

Comment 14

[(dormant account)]

yes to both... The unfreeze logic should probably be same as in panhandler(factor it out). Ie if the sidebars are showing, don't hide the navbar post-loading,etc

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Right, force it visible on load start and then undo that in end.  I have a patch that turns the urlbar visibility thing into a counter instead of a boolean, so that you can show it multiple times and it'll hide only on the last hide.. something like that should probably be used.

I'm not sure that CanvasBrowser's startLoading/endLoading are the right place though, since CanvasBrowser knows nothing about UI.

Comment 16

[(dormant account)]

> I'm not sure that CanvasBrowser's startLoading/endLoading are the right place
> though, since CanvasBrowser knows nothing about UI.

browser-ui is the place to do it, it knows when urlback switches from loading to loaded view.

Comment 17

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

Comment on attachment 368135 [details] [diff] [review]
closer to being done

We need to file a followup bug on show/hiding the urlbar.

Comment 18

[(dormant account)]

http://hg.mozilla.org/mobile-browser/rev/ce51a8749abb