Display fixed position objects on every page of paged media.

Display fixed position objects on every page of paged media.

"For paged media, boxes with fixed positions are repeated on every page."
- https://www.w3.org/TR/2011/REC-CSS2-20110607/visuren.html#fixed-positioning

BUG=303728
Committed: https://crrev.com/a325c95dc0b41ce76f398d7db73c108dc9293702
Cr-Commit-Position: refs/heads/master@{#385513}

[rhogan, 2016-01-11 18:15:05]

robhogan@gmail.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2016-01-11 19:54:51]

I think the tests should be reftests. That should be possible if you use forced
page breaks.

https://codereview.chromium.org/1575983002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/1575983002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BlockPainter.cpp:75: // For paged media,
boxes with fixed positions are repeated on every page.
I think this approach is wrong. This would be in the wrong paint order, and I
suspect that they would even be painted doubly on the first page, since they
already got painted on the first page prior to your changes, and with this code
they'll be painted again.

And aren't you going to miss fixedpos objects that live inside other fixedpos
objects. Those aren't in the positioned descendants list of the root element,
are they?

[rhogan, 2016-01-14 12:10:44]

https://codereview.chromium.org/1575983002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/1575983002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BlockPainter.cpp:75: // For paged media,
boxes with fixed positions are repeated on every page.
On 2016/01/11 at 19:54:51, mstensho wrote:
> I think this approach is wrong. This would be in the wrong paint order..
OK I don't have enough of a clue about painting to know what the right thing to
do is here. I can't seem to paint the same object multiple times if I try to do
it from within the object's BlockPainter::paint() so I think I need to paint
from the HTML object like I'm currently doing but in some way that better
preserves paint order.

>.. and I suspect that they would even be painted doubly on the first page,
since they already got painted on the first page prior to your changes, and with
this code they'll be painted again.
No, I do actually skip the first page so there's no double painting.

> And aren't you going to miss fixedpos objects that live inside other fixedpos
objects. Those aren't in the positioned descendants list of the root element,
are they?
They are aren't they?

[mstensho (USE GERRIT), 2016-02-11 12:09:39]

Description was changed from

==========
Display fixed position objects on every page of paged media.

BUG=303728
==========

to

==========
Display fixed position objects on every page of paged media.

BUG=303728
==========

[rhogan, 2016-02-20 09:33:36]

robhogan@gmail.com changed reviewers:
+ chrishtr@chromium.org

[rhogan, 2016-02-20 09:33:37]

Hi Chris - could you add your two cents here? I don't know what the right way to
paint is here.

[rhogan, 2016-02-24 19:06:04]

robhogan@gmail.com changed reviewers:
+ wangxianzhu@chromium.org

[rhogan, 2016-02-24 19:06:04]

wangxianzhu - could you help with the painting question here? Am I doing this
right? Is there a better way?

[Xianzhu, 2016-02-24 19:22:51]

https://codereview.chromium.org/1575983002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/1575983002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BlockPainter.cpp:75: // For paged media,
boxes with fixed positions are repeated on every page.
On 2016/01/14 12:10:44, rhogan wrote:
> On 2016/01/11 at 19:54:51, mstensho wrote:
> > I think this approach is wrong. This would be in the wrong paint order..
> OK I don't have enough of a clue about painting to know what the right thing
to
> do is here. I can't seem to paint the same object multiple times if I try to
do
> it from within the object's BlockPainter::paint() so I think I need to paint
> from the HTML object like I'm currently doing but in some way that better
> preserves paint order.

I also think the current patch is incorrect about painting order. You don't need
to paint the same object multiple times by yourself. This is handled by
PaintLayerPainter or WebLocalFrameImpl. I think you just need to adjust the
paint offset of the fixedpos object when it's painted.

> 
> >.. and I suspect that they would even be painted doubly on the first page,
> since they already got painted on the first page prior to your changes, and
with
> this code they'll be painted again.
> No, I do actually skip the first page so there's no double painting.
> 
> > And aren't you going to miss fixedpos objects that live inside other
fixedpos
> objects. Those aren't in the positioned descendants list of the root element,
> are they?
> They are aren't they?

I confirm that the following code can find all viewport contained fixedpos
objects.

[rhogan, 2016-02-25 20:33:04]

https://codereview.chromium.org/1575983002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/1575983002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BlockPainter.cpp:75: // For paged media,
boxes with fixed positions are repeated on every page.
On 2016/02/24 at 19:22:51, Xianzhu wrote:
> I also think the current patch is incorrect about painting order. You don't
need to paint the same object multiple times by yourself. This is handled by
PaintLayerPainter or WebLocalFrameImpl. I think you just need to adjust the
paint offset of the fixedpos object when it's painted.

When I do that it doesn't paint at all. 

--- a/third_party/WebKit/Source/core/layout/LayoutBlock.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBlock.cpp
@@ -1274,6 +1274,19 @@ void LayoutBlock::paintChildren(const PaintInfo&
paintInfo, const LayoutPoint& p
 void LayoutBlock::paintObject(const PaintInfo& paintInfo, const LayoutPoint&
paintOffset) const
 {
     BlockPainter(*this).paintObject(paintInfo, paintOffset);
+    if (!view()->document().printing() || style()->position() != FixedPosition)
+        return;
+    LayoutView* view = this->view();
+    HTMLElement* body = document().body();
+    LayoutObject* bodyObject = isHTMLBodyElement(body) ? body->layoutObject() :
0;
+    if (!bodyObject)
+        return;
+    int pages = toLayoutBlock(bodyObject)->logicalHeight() /
view->pageLogicalHeight();
+    LayoutPoint adjustedPaintOffset = paintOffset;
+    for (int i = 0; i < pages; i++) {
+        adjustedPaintOffset += LayoutPoint(0, view->pageLogicalHeight());
+        BlockPainter(*this).paintObject(paintInfo, adjustedPaintOffset);
+    }
 }
 

Am I missing your point? Where should I be adjusting the offset?

[Xianzhu, 2016-02-25 21:33:05]

On 2016/02/25 20:33:04, rhogan wrote:
>
https://codereview.chromium.org/1575983002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right):
> 
>
https://codereview.chromium.org/1575983002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/BlockPainter.cpp:75: // For paged media,
> boxes with fixed positions are repeated on every page.
> On 2016/02/24 at 19:22:51, Xianzhu wrote:
> > I also think the current patch is incorrect about painting order. You don't
> need to paint the same object multiple times by yourself. This is handled by
> PaintLayerPainter or WebLocalFrameImpl. I think you just need to adjust the
> paint offset of the fixedpos object when it's painted.
> 
> When I do that it doesn't paint at all. 
> 
> --- a/third_party/WebKit/Source/core/layout/LayoutBlock.cpp
> +++ b/third_party/WebKit/Source/core/layout/LayoutBlock.cpp
> @@ -1274,6 +1274,19 @@ void LayoutBlock::paintChildren(const PaintInfo&
> paintInfo, const LayoutPoint& p
>  void LayoutBlock::paintObject(const PaintInfo& paintInfo, const LayoutPoint&
> paintOffset) const
>  {
>      BlockPainter(*this).paintObject(paintInfo, paintOffset);
> +    if (!view()->document().printing() || style()->position() !=
FixedPosition)
> +        return;
> +    LayoutView* view = this->view();
> +    HTMLElement* body = document().body();
> +    LayoutObject* bodyObject = isHTMLBodyElement(body) ? body->layoutObject()
:
> 0;
> +    if (!bodyObject)
> +        return;
> +    int pages = toLayoutBlock(bodyObject)->logicalHeight() /
> view->pageLogicalHeight();
> +    LayoutPoint adjustedPaintOffset = paintOffset;
> +    for (int i = 0; i < pages; i++) {
> +        adjustedPaintOffset += LayoutPoint(0, view->pageLogicalHeight());
> +        BlockPainter(*this).paintObject(paintInfo, adjustedPaintOffset);
> +    }
>  }
>  
> 
> Am I missing your point? Where should I be adjusting the offset?

Sorry what I suggested was incorrect. The fixedpos element has its own layer, so
we should adjust the offset of the layer instead of the object (which is too
late because the layer has been rejected as its bounds is out of the current
page). If we are painting multiple pages, we should also add the layer into
multiple fragments. I think this could be done in
PaintLayer::collectFragments().

[rhogan, 2016-03-19 14:01:15]

Description was changed from

==========
Display fixed position objects on every page of paged media.

BUG=303728
==========

to

==========
Display fixed position objects on every page of paged media.

"For paged media, boxes with fixed positions are repeated on every page." 
- https://www.w3.org/TR/2011/REC-CSS2-20110607/visuren.html#fixed-positioning

BUG=303728
==========

[rhogan, 2016-03-21 18:41:51]

On 2016/02/25 at 21:33:05, wangxianzhu wrote:

> Sorry what I suggested was incorrect. The fixedpos element has its own layer,
so we should adjust the offset of the layer instead of the object (which is too
late because the layer has been rejected as its bounds is out of the current
page). If we are painting multiple pages, we should also add the layer into
multiple fragments. I think this could be done in
PaintLayer::collectFragments().

I had another go on foot of your suggestion. Can you take another look?

[Xianzhu, 2016-03-21 19:37:18]

I think we should also add tests for fixed-position element under sub-frame,
transformed element or css clip.

https://codereview.chromium.org/1575983002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/printing/absolute-positioned-headers-and-footers.html
(right):

https://codereview.chromium.org/1575983002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/printing/absolute-positioned-headers-and-footers.html:1:
<!DOCTYPE html>
Nit: I suggest to rename the file to
"absolute-position-headers-and-footers.html" (without "ed" after "position"), to
keep consistent with other names. Also other files.

https://codereview.chromium.org/1575983002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/printing/fixed-positioned-but-static-headers-and-footers.html
(right):

https://codereview.chromium.org/1575983002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/printing/fixed-positioned-but-static-headers-and-footers.html:18:
crbug.com/303728: Header Line is statically positioned and will be repeated on
every page.
Nit: This is a bit confusing (especially compared to
absolute-positioned-headers-and-footers.html) making me think the header line is
static-position not fixed-position. How about "Fixed-position header line is
..."?

https://codereview.chromium.org/1575983002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/printing/fixed-positioned-headers-and-footers.html
(right):

https://codereview.chromium.org/1575983002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/printing/fixed-positioned-headers-and-footers.html:20:
crbug.com/303728: Header Line should be repeated on every page.
Nit: How about "Fixed-position header line ..."?

https://codereview.chromium.org/1575983002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/1575983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:459: bool
isFixedPosObjectInPagedMedia = object->style()->position() == FixedPosition &&
view->pageLogicalHeight();
We should exclude the case that the view is not the container of the fixed-pos
element (e.g. if the fixed-pos element is under a transformed element).

We may need to add another condition here: ... && view ==
object->containerForFixedPosition().

https://codereview.chromium.org/1575983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:463: // "For paged
media, boxes with fixed positions are repeated on every page."
Nit: include the link of source of the above statement.

https://codereview.chromium.org/1575983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:501: if (parentLayer
&& !isFixedPosObjectInPagedMedia) {
I think we should still clip (especially css clip) fixed-pos elements.

For example:
<div style="clip: rect(0, 100px, 5000px, 0)>
  <div style="position: fixed; ..."></div>
</div>

I think in this case, whether the fixed-pos element appears in all pages depends
on how big the clip is.

[rhogan, 2016-03-25 17:41:52]

On 2016/03/21 at 19:37:18, wangxianzhu wrote:
> For example:
> <div style="clip: rect(0, 100px, 5000px, 0)>
>   <div style="position: fixed; ..."></div>
> </div>
> 
> I think in this case, whether the fixed-pos element appears in all pages
depends on how big the clip is.


Thanks wangxianzhu - applied your comments and added some tests to cover them.
Can you take another look?

[Xianzhu, 2016-03-27 23:22:39]

On 2016/03/25 17:41:52, rhogan wrote:
> On 2016/03/21 at 19:37:18, wangxianzhu wrote:
> > For example:
> > <div style="clip: rect(0, 100px, 5000px, 0)>
> >   <div style="position: fixed; ..."></div>
> > </div>
> > 
> > I think in this case, whether the fixed-pos element appears in all pages
> depends on how big the clip is.
> 
> 
> Thanks wangxianzhu - applied your comments and added some tests to cover them.
> Can you take another look?

rhogan, have you published your comments? I can't see them.

[rhogan, 2016-03-28 10:45:51]

On 2016/03/27 at 23:22:39, wangxianzhu wrote:
> On 2016/03/25 17:41:52, rhogan wrote:
> > On 2016/03/21 at 19:37:18, wangxianzhu wrote:
> > > For example:
> > > <div style="clip: rect(0, 100px, 5000px, 0)>
> > >   <div style="position: fixed; ..."></div>
> > > </div>
> > > 
> > > I think in this case, whether the fixed-pos element appears in all pages
> > depends on how big the clip is.
> > 
> > 
> > Thanks wangxianzhu - applied your comments and added some tests to cover
them.
> > Can you take another look?
> 
> rhogan, have you published your comments? I can't see them.

I didn't have any - I took on board your comments and updated the CL! :)

[Xianzhu, 2016-03-28 17:01:34]

https://codereview.chromium.org/1575983002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/printing/fixed-positioned-headers-and-footers-clipped.html
(right):

https://codereview.chromium.org/1575983002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/printing/fixed-positioned-headers-and-footers-clipped.html:27:
crbug.com/303728: Clipped header Line should be repeated on every page.
I think for this case, we should display the header line in the first page only
because of the clipping.

The spec says "boxes with fixed positions are repeated on every page", but
doesn't mention other constraints which I think we still need to apply. My
understanding of the spec is that the fixed-position box is displayed as if the
document is scrolled at the beginning of each page.

[rhogan, 2016-03-30 19:49:05]

On 2016/03/28 at 17:01:34, wangxianzhu wrote:
>
https://codereview.chromium.org/1575983002/diff/60001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/printing/fixed-positioned-headers-and-footers-clipped.html
(right):
> 
>
https://codereview.chromium.org/1575983002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/printing/fixed-positioned-headers-and-footers-clipped.html:27:
crbug.com/303728: Clipped header Line should be repeated on every page.
> I think for this case, we should display the header line in the first page
only because of the clipping.

Can you explain the logic behind this to me? If the box is clipped like in my
test, how does it follow that it should display on the first page only?

> The spec says "boxes with fixed positions are repeated on every page", but
doesn't mention other constraints which I think we still need to apply. My
understanding of the spec is that the fixed-position box is displayed as if the
document is scrolled at the beginning of each page.

I don't see the connection here - I suspect a gap in my knowledge of the
relationship between clipping and scrolling?

FF renders the test by clipping the box on the first page. It then displays the
box at the top of all subsequent pages but doesn't clip any of them. Is that
what you're after?

[Xianzhu, 2016-03-30 20:39:28]

On 2016/03/30 19:49:05, rhogan wrote:
> On 2016/03/28 at 17:01:34, wangxianzhu wrote:
> >
>
https://codereview.chromium.org/1575983002/diff/60001/third_party/WebKit/Layo...
> > File
>
third_party/WebKit/LayoutTests/printing/fixed-positioned-headers-and-footers-clipped.html
> (right):
> > 
> >
>
https://codereview.chromium.org/1575983002/diff/60001/third_party/WebKit/Layo...
> >
>
third_party/WebKit/LayoutTests/printing/fixed-positioned-headers-and-footers-clipped.html:27:
> crbug.com/303728: Clipped header Line should be repeated on every page.
> > I think for this case, we should display the header line in the first page
> only because of the clipping.
> 
> Can you explain the logic behind this to me? If the box is clipped like in my
> test, how does it follow that it should display on the first page only?
> 
> > The spec says "boxes with fixed positions are repeated on every page", but
> doesn't mention other constraints which I think we still need to apply. My
> understanding of the spec is that the fixed-position box is displayed as if
the
> document is scrolled at the beginning of each page.
> 
> I don't see the connection here - I suspect a gap in my knowledge of the
> relationship between clipping and scrolling?
> 
> FF renders the test by clipping the box on the first page. It then displays
the
> box at the top of all subsequent pages but doesn't clip any of them. Is that
> what you're after?

Css-clip is applied on the whole subtree including out-of-flow-positioned
descendants. For the printing with css-clip case, the clip is on an
absolute-position element which will scroll along the page. Whether the
fixed-position object is visible when the viewport is scrolled depends on
whether the clipping area is visible in the viewport. In the test, the
fixed-position object is visible only in (8, 0, 400x50) area in absolute
coordinates, so it should be visible in only the first page (in which the
clipped area is visible).

If you open the page in browser and scroll it, you'll see that the
fixed-position disappears when the clipping area scrolls out of the viewport.

Another case that fixed-position might be invisible in some pages is like the
following (for which you might want to add another test case):

  <div style="position: fixed; background-color: yellow; width: 100px; height:
100px; top: 0; left: 0">Fixed</div>
  <div style="position: absolute; left: 0px; top: 1000px; width: 200px; height:
2000px; background-color: red; z-index: 2">Absolute</div>
  <div style="height: 5000px"></div>

Suppose the page height is 500px, the fixed-position element will be covered by
the absolute element thus invisible in page 3 and 4, and visible in other pages.

I think Firefox implements the clip case wrong for printing. It does implement
the above case correctly.

[rhogan, 2016-04-04 19:18:04]

On 2016/03/30 at 20:39:28, wangxianzhu wrote:
> 
> I think Firefox implements the clip case wrong for printing. It does implement
the above case correctly.

Thanks wangxianzhu - have updated the CL per your comments.

[Xianzhu, 2016-04-04 19:21:00]

lgtm

[rhogan, 2016-04-04 19:24:59]

On 2016/04/04 at 19:21:00, wangxianzhu wrote:
> lgtm

Awesome, thanks. Looking forward to landing this!

[rhogan, 2016-04-06 17:36:37]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2016-04-06 17:36:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/1575983002/#ps160001
(title: "Updated")

[commit-bot: I haz the power, 2016-04-06 17:36:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575983002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575983002/160001

[commit-bot: I haz the power, 2016-04-06 18:53:13]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-06 18:55:33]

Description was changed from

==========
Display fixed position objects on every page of paged media.

"For paged media, boxes with fixed positions are repeated on every page." 
- https://www.w3.org/TR/2011/REC-CSS2-20110607/visuren.html#fixed-positioning

BUG=303728
==========

to

==========
Display fixed position objects on every page of paged media.

"For paged media, boxes with fixed positions are repeated on every page."
- https://www.w3.org/TR/2011/REC-CSS2-20110607/visuren.html#fixed-positioning

BUG=303728

Committed: https://crrev.com/a325c95dc0b41ce76f398d7db73c108dc9293702
Cr-Commit-Position: refs/heads/master@{#385513}
==========

[commit-bot: I haz the power, 2016-04-06 18:55:34]

Patchset 9 (id:??) landed as
https://crrev.com/a325c95dc0b41ce76f398d7db73c108dc9293702
Cr-Commit-Position: refs/heads/master@{#385513}

[loyso (OOO), 2016-04-07 01:08:05]

This CL causes virtual/threaded/printing tests to fail on WebKit Win10 even
after re-baselining of tests.