510124 - Provide new RenderDocument flags for separate painting fixed and not fixed layout frames

Status: NEW

(Core :: Layout, defect)

Description

[Oleg Romashin (:romaxa)]

Attachment: New flags for RenderDocument, and usage of these flags in nsViewPortFrame

For browser rendering implementation with virtual layout surface (canvas for fennec) existing one problem with fixed positioned frames.
Problem appear when we scrolling virtual surface and fixed frames are scrolled too...
Repainting of fixed frames for each scroll event make scrolling speed dependent from content, without synchronous repainting those frames jumping or following to the scrolled virtual surface (depends how often we are updating scroll port position).

There is 2 possible ways to fix this problem:
1) Hide fixed frames before scrolling, and restore it back when scrolling finished.
2) Use option 1) + paint and remember fixed frames temporary layer, and during scrolling paint that layer on top of main surface (which does not contain painted fixed frames).

Here is the working patch which allow to paint layout:
1) Fixed frames only 
2) Without fixed frames
3) both - default

Still not sure about right names for flags and member variables...

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: New PresShell API for Invalidating fixed frames only

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This patch doesn't match the approach described in comment #0.

Comment 3

[Oleg Romashin (:romaxa)]

(In reply to comment #2)
> This patch doesn't match the approach described in comment #0.

Why it doesn't match? 
First patch providing flag for separate rendering fixed frames and layout without fixed frames... Before scrolling we will paint layout without fixed frames, and after scrolling return fixed frames back by requesting for paint in normal mode.

Second patch will help to repaint only fixed frames regions, to avoid extra painting and make it cheaper.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oops, I didn't noticed you'd added two patches.

For the first patch, you don't need to add flags to nsViewportFrame. In nsDisplayListBuilder I think it's clearer if the two flags have the same sense --- i.e., they should both indicate frames to paint, or both indicate frames to ignore. In this case I think it's better to make them indicate frames to ignore, so call them mIgnoreFixedFrames and mIgnoreNonFixedFrames.

I don't know why you need the patch in comment #1. Why is it useful to invalidate fixed-pos frames?

Comment 5

[Oleg Romashin (:romaxa)]

> I don't know why you need the patch in comment #1. Why is it useful to
> invalidate fixed-pos frames?

It is just for fast re-painting of fixed frames area.

On N900, when we start scrolling I'm requesting to repaint facebook toolbar area only, and when scrolling is finished I repaint that area again.

This is important because before scrolling started we have to repaint that area as possible faster, otherwise toolbar moving together with cached offscreen area.

Comment 6

[Oleg Romashin (:romaxa)]

Comment on attachment 394181 [details] [diff] [review]
New PresShell API for Invalidating fixed frames only

Probably it would be flexible also to have possibility to get region of fixed frames position...

Comment 7

[Oleg Romashin (:romaxa)]

> For the first patch, you don't need to add flags to nsViewportFrame. In

I need to know value of that flags in "ViewportFrame::GetChildList", how I can get nsDisplayListBuilder in that function?

> ignore. In this case I think it's better to make them indicate frames to
> ignore, so call them mIgnoreFixedFrames and mIgnoreNonFixedFrames.

Ye I can do that

Comment 8

[Oleg Romashin (:romaxa)]

Attachment: Updated patch

Updated according to comment

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  NS_IMETHOD InvalidateFixedFrames(void) = 0;

Would it make more sense to not invalidate, just return the region occupied by the fixed frames? Or something like that? InvalidateFixedFrames is a bit weird since it does a NOTIFY_ONLY invalidate, so it's not actually invalidating anything, just firing a MozAfterPaint event.

+  if (!ignoreFixedFrames)
+    aBuilder->MarkFramesForDisplayList(this, mFixedContainer.GetChildList(), aDirtyRect);

{}

+  nsIFrame* kid = ignoreNonFixedFrames ?
+      mFixedContainer.GetChildList().FirstChild() :
+      mFrames.FirstChild();

This will actually cause subtle problems. Normally we paint fixed-pos frames through their placeholders. This guarantees that stuff like
  <div style="opacity:0.5">
     <div style="position:fixed; ..."></div>
  </div>
works properly. By painting the fixed-pos frames directly in this case, you'd break those pages.

I'm not sure what to do here. Possibly you need to go through the display list after it's been built and remove all leaf display items associated with frames that aren't inside one of the fixed-pos frames of this nsViewportFrame.

Comment 10

[Oleg Romashin (:romaxa)]

What do you mean about problem with this testcase?:
<body bgcolor=blue>
<div style="opacity:0.5">
     <div style="position:fixed;"><h1>XYZ</h1></div>
     <h1>ABC</h1>
</div>
<h1>123</h1>
<body>

Now it painting in this way:
Only Fixed frames: - transparent "XYZ"
Only Normal frames: - transparent "ABC", not opaque "123"

Is it wrong behavior or what?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Only Fixed frames: - transparent "XYZ"

How does that work? It seems to me that the text should not be transparent, with your patch. Can you dump the display list and paste it here?

Comment 12

[Oleg Romashin (:romaxa)]

Yep, you right, I was wrong and did some mistake.
How can I detect "items associated with frames that aren't inside" - ?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Every leaf display item has an associated frame --- GetUnderlyingFrame(). Call it, and then scan the ancestor chain of that frame to see if it's under a fixed-pos frame.

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: New API for getting fixed frames area

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You need to rev IID on nsIPresShell.

I don't think this should return the screenrect. Can't it just return coordinates relative to the presshell, i.e. the union of the frame rects?

And I think it should return nsIntRegion, not an nsIRegion. Does that work for you?

Comment 16

[Oleg Romashin (:romaxa)]

> You need to rev IID on nsIPresShell.
Ok,
> 
> I don't think this should return the screenrect. Can't it just return
> coordinates relative to the presshell, i.e. the union of the frame rects?
Which function should I call to get that union?

It would be nice to have the same coordinates as in MozAfterPaint event.

For painting these rects I'm using the same function which is painting MozAfterPaint events


> 
> And I think it should return nsIntRegion, not an nsIRegion. Does that work for
> you?
I think I can use nsIntRegion, no problem.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #16)
> > I don't think this should return the screenrect. Can't it just return
> > coordinates relative to the presshell, i.e. the union of the frame rects?
> Which function should I call to get that union?

Just call GetRect instead of GetScreenRect. Or call GetRect and then convert to CSS pixels.

> It would be nice to have the same coordinates as in MozAfterPaint event.

That's in CSS pixels relative to the presshell, i.e. not screen coordinates.

Comment 18

[Oleg Romashin (:romaxa)]

(In reply to comment #13)
> Every leaf display item has an associated frame --- GetUnderlyingFrame(). Call
> it, and then scan the ancestor chain of that frame to see if it's under a
> fixed-pos frame.

In this case I should get normal list with all frames at first... and then run recursive function (similar to ComputeVisibility) and recreate DisplayList by keeping fixed frames and frames which are containing fixed-pos frames...

But this I think a bit expensive...

Another way is to hack BuildDisplayList functions and don't add frames which are not containing fixed frames.

This should be a cheaper solution...

Comment 19

[Oleg Romashin (:romaxa)]

Attachment: Remove nonFixedFrames implementation

Comment 20

[Oleg Romashin (:romaxa)]

> > you?
> I think I can use nsIntRegion, no problem.

Hmm... 1.9.2 does not have nsIntRegion... :(.
I will create one version for trunk, based on nsIntRegion...
And another for 1.9.2 branch based on nsRegion.

Comment 21

[Oleg Romashin (:romaxa)]

Oh, and I not able to use nsRegion outside of layout... 
I'm getting this:
 undefined symbol: _ZN8nsRegion4InitEv
....

Comment 22

[Oleg Romashin (:romaxa)]

Attachment: Updated IID, GetScreenRect -> GetRect + conversion

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  PRBool IsIgnoreFixedFrames(void) { return mIgnoreFixedFrames; }
+  PRBool IsIgnoreNonFixedFrames(void) { return mIgnoreNonFixedFrames; }

IsIgnoring

We don't need IsIgnoreNonFixedFrames. PresShell::RenderDocument can just test its aFlags directly.

+  return Count() > 0 ? PR_TRUE : PR_FALSE;

return Count() > 0;

+   * Find and keep only fixed frames and their parents in the list.
+   * @return TRUE if list is not empty
+   */
+  PRBool RemoveNonFixedItems(nsDisplayListBuilder* aBuilder, nsIFrame *aFixed);

Need to document what aFixed means. Actually I suspect what you want to do here is pass in the nsViewportFrame whose fixed-pos frames you want to keep. Then for each GetUnderlyingFrame, find its ancestor whose parent is that nsViewportFrame, and check if that ancestor is the viewport frame's normal child (GetChildList(nsnull)).

+  if (!aBuilder->IsIgnoreFixedFrames()) {
+    aBuilder->MarkFramesForDisplayList(this, mFixedContainer.GetChildList(),
+                                       aDirtyRect);
+  }

This is actually incorrect. It will skip fixed-pos frames in IFRAMEs. You actually want to only ignore fixed frames of the top-level document you're rendering.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  NS_IMETHOD GetFixedFramesArea(nsIRegion **aOutRegion) = 0;

Need some documentation.

+    outRegion->Union(nsPresContext::AppUnitsToIntCSSPixels(rect.x),
+                     nsPresContext::AppUnitsToIntCSSPixels(rect.y),
+                     nsPresContext::AppUnitsToIntCSSPixels(rect.width),
+                     nsPresContext::AppUnitsToIntCSSPixels(rect.height));

You probably want some specific rounding here, but I'm not sure what. What do you want to return when the edges of the fixed-pos frame aren't aligned with CSS pixels? Note that due to zooming they might be aligned to device pixels but not CSS pixels.

Also, do you want to include the overflow areas of fixed-pos content?

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 412595 [details] [diff] [review]
Remove nonFixedFrames implementation

see comments #23 and #24

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 412615 [details] [diff] [review]
Updated IID, GetScreenRect -> GetRect + conversion

see comments #23 and #24

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.