The default bug view has changed. See this FAQ.

Fix checkerboarding during loading

RESOLVED FIXED in mozilla9

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: stechz, Assigned: stechz)

Tracking

Trunk
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
There are a lot of times we see checkerboard during loading. One thing we could do is ignore the displayport (and thus checkerboarding) if the only thing on the page is a solid color.

Another thing we could do is add a way to remove the displayport, and do that on a location change in Fennec.
(Assignee)

Comment 1

6 years ago
After playing around, here is one of the biggest sources of checkerboarding during load:
1) something is tapped in the awesomescreen while the keyboard is up
2) browser comes up, keyboard begins to come down
3) document is loading and paints begin to suppress
4) once keyboard is down, screen resizes and browser resizes
5) document would repaint, but paints are suppressed

I'm not quite sure who is at fault here. *Something* should cause a repaint even though paints are suppressed.
Summary: Do not paint checkerboards if page is solid color → Fix checkerboarding during loading
(Assignee)

Comment 2

6 years ago
Created attachment 554571 [details] [diff] [review]
Hacky fix: don't paint checkerboards when painting has been suppressed
(Assignee)

Comment 3

6 years ago
So the above patch "fixes" the problem by not painting anything while painting has been suppressed in the content process. Like I said above, the right thing to do is to repaint when the window resizes instead of suppressing it.

Roc, do you know what should be in charge of causing a repaint in the content process? And how to do it? UpdateView will ignore the repaint due to presshell suppression.
I guess normally what happens in the single-process world is that when you resize a browser window, the whole window is invalid so invalidation is not needed. We ask the content document to paint at its new size and it paints the background color.

Is PuppetWidget::Resize getting called? If so, why isn't it invalidating the area that needs to be repainted?
(Assignee)

Comment 5

6 years ago
Resize is getting called several times, and I did see an invalidate in there. Hm.
(Assignee)

Comment 6

6 years ago
To me it looks like there are two problems:

1) http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6145
NS_FRAME_UPDATE_LAYER_TREE isn't set, so we early exit from painting on resize.

2) If we are before the first repaint and A) the new frame isn't present yet or B) the new frame is present. Either way, invalidating the resize difference between the old displayport and the new displayport doesn't really make sense.

I'll play around a little bit.
We create layers for the entire viewport, not just the displayed portion, right? So the viewport we used when the keyboard was up is not big enough?
(Assignee)

Comment 8

6 years ago
Created attachment 555116 [details] [diff] [review]
Fix checkerboarding during loading
(Assignee)

Comment 9

6 years ago
Comment on attachment 555116 [details] [diff] [review]
Fix checkerboarding during loading

This fixes the platform part of our checkerboarding issue by using the prescontext's visible area for the color layer bounds when no frame is present.

I also removed a faulty assertion from nsPresShell that assumes that the root view's dimensions will be the same as the visible area for the presshell, which is simply not true when setCSSViewport has been called. What was the assertion for? If it is for something specific I'd be happy to fix it.
Attachment #555116 - Flags: review?(roc)
(Assignee)

Comment 10

6 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> We create layers for the entire viewport, not just the displayed portion,
> right? So the viewport we used when the keyboard was up is not big enough?

We talked about this on IRC, but for context for those following at home: normally we do create layers for the entire viewport, but when there is no frame present we were only creating a layer that covered the invalidated area. This breaks us in the multiprocess case.
(Assignee)

Comment 11

6 years ago
You might notice I removed some whitespace: there were trailing whitespace characters and the newlines didn't seem necessary anyway, so I just removed them.
(Assignee)

Comment 12

6 years ago
Created attachment 555163 [details] [diff] [review]
WIP: Reduce checkerboarding during load
(Assignee)

Comment 13

6 years ago
The WIP does several things:
* remove display port method (probably not a necessary part of the patch)
* only reflow for viewport size once on first paint (we might have to change this? maybe mbrubeck has some cases where this won't work)
* call setWindowSize when keyboard goes away (removes permanent checkerboard issue)
* don't set displayport before first paint (helps for checkerboarding between first paint and viewport reflow, though I'm not exactly sure why).
Comment on attachment 555163 [details] [diff] [review]
WIP: Reduce checkerboarding during load

Review of attachment 555163 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget to rev UUID on nsIDOMWindowUtils.
Comment on attachment 555116 [details] [diff] [review]
Fix checkerboarding during loading

Review of attachment 555116 [details] [diff] [review]:
-----------------------------------------------------------------

lovely!
Attachment #555116 - Flags: review?(roc) → review+
(Assignee)

Comment 16

6 years ago
Created attachment 555235 [details] [diff] [review]
Reduce checkerboarding during load
(Assignee)

Updated

6 years ago
Assignee: nobody → ben
(Assignee)

Updated

6 years ago
Attachment #555163 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #554571 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #555235 - Flags: review?(mbrubeck)
Comment on attachment 555235 [details] [diff] [review]
Reduce checkerboarding during load

Review of attachment 555235 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/chrome/content/browser.js
@@ -1601,3 @@
>        aTab.updateThumbnail();
>  
> -      browser.messageManager.addMessageListener("MozScrolledAreaChanged", aTab.scrolledAreaChanged);

You can also remove |this.scrolledAreaChanged = this.scrolledAreaChanged.bind(this);| from the Tab constructor.

@@ +2963,5 @@
>  
> +    if (firstPaint) {
> +      // You only get one shot, do not miss your chance to reflow.
> +      this.updateViewportSize();
> +    }

I think you can just remove updateViewportSize() here now that you have removed useFallbackWidth.  See bug 604765 where they were both added.

@@ +3215,5 @@
>      Browser.styles["viewable-width"].width = newWidth + "px";
>      Browser.styles["viewable-width"].maxWidth = newWidth + "px";
>  
>      let startup = !oldHeight && !oldWidth;
> +    if (newHeight > oldHeight || newWidth != oldWidth) {

Please add a comment before the "if" statement, like "Don't update the viewport if the screen it is just shrinking vertically because a keyboard appeared."

Also, you can move "let startup" inside the "if" statement.
Attachment #555235 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 18

6 years ago
> You can also remove |this.scrolledAreaChanged =
> this.scrolledAreaChanged.bind(this);| from the Tab constructor.

Done.

> I think you can just remove updateViewportSize() here now that you have
> removed useFallbackWidth.  See bug 604765 where they were both added.

Actually, it's nice to do this after first paint immediately so that the page is sized appropriately during load. Most of the time you don't even notice the size change on my build. Could we keep it? :)

> Please add a comment before the "if" statement, like "Don't update the
> viewport if the screen it is just shrinking vertically because a keyboard
> appeared."
> 
> Also, you can move "let startup" inside the "if" statement.

Done.
http://hg.mozilla.org/mozilla-central/rev/7e98f39c07e9
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
(Assignee)

Comment 20

6 years ago
Frontend patch has not landed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Benjamin Stover (:stechz) from comment #18)
> > I think you can just remove updateViewportSize() here now that you have
> > removed useFallbackWidth.  See bug 604765 where they were both added.
> 
> Actually, it's nice to do this after first paint immediately so that the
> page is sized appropriately during load. Most of the time you don't even
> notice the size change on my build. Could we keep it? :)

Sure, we can keep this.
http://hg.mozilla.org/mozilla-central/rev/b1cab441835f
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 688047
You need to log in before you can comment on or make changes to this bug.