Improve clipping and positioning documentation #2892

mrobinson · 2018-07-13T10:15:38Z

The new documentation gives a more complete and up-to-date picture of
WebRender's clipping and positioning architecture.

This change is

leeoniya · 2018-07-13T13:36:35Z

is anything mentioned these issues still relevant for Ideas for the Future?

#2551

...when we separate the concept of
reference frames and stacking contexts.

#2736

mrobinson · 2018-07-13T14:58:46Z

@leeoniya Stacking contexts and reference frames are now two separate concepts after #2756. The idea of applying clips to stacking contexts was implemented in #2600. We are still working out exactly how flexible Gecko requires the display list to be.

staktrace · 2018-07-13T17:50:00Z

Thanks for writing this documentation! I read through it and it clarified a few things for me. I agree that differentiating between the three kinds of things in the API would be good and make things more robust. I was looking at the gecko side code and it seems like every clip node that we create with a parent has what is effectively a spatial node as the parent (i.e. it's the WrClipId corresponding to a scroll frame, a reference frame, or a sticky node) with one exception: here we create a clip node with a mask, and then use that clip code as the parent for other clip nodes that might get defined on items nested inside the mask.

If we can do this in a different way (e.g. including the mask in the clip chain of the nested items) then that would clean this up and we should be able to separate WrClipId from a WrSpatialNodeId on the gecko side. That would be a good first step towards a better API, I think.

kvark

Nice writeup, thank you! I got a few questions/concerns.

kvark · 2018-07-13T18:48:43Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+# Original Design
+
+To understand the current design for clipping and positioning (transformations
+and scrolling) in WebRender if can be useful to have a little background about


kvark · 2018-07-13T18:48:43Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+    intersecting content from outside the scroll area.
+ 3. Items in the same scrolling root, clipped by different clips one or more of
+    which are defined outside the scrolling root itself.
+ 4. Items which are clipped by some clips in the hierarchy, but not others ie


kvark · 2018-07-13T18:48:43Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+    elements. Every display list item has an assigned `ClipChain` which
+    specifies what `ClipNodes` are applied to that item.
+
+The `SpatialNode` of the clips applied to each item are completely independent of


SpatialNodes

I changed this to make the inflection of the noun match the subject.

kvark · 2018-07-13T18:48:43Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+
+One holdover from the previous design is that both `ClipNode` and `SpatialNodes`
+have a parent node, which is either a `SpatialNode` or a `ClipNode`.  From this
+node WebRender can determine both a parent `ClipNode` and a parent `SpatialNode`


This is confusing me quite a bit. Why do we want to support the ClipNode being a parent, ever?

Nowadays there is no reason this has to be the case, but consider that in the past the only way to combine clip nodes was to have them be part of the ancestry in the ClipScrollTree. With the ClipChain API and the fact that an item can have both a positioning node and clip node assigned, it isn't strictly necessary. Once both Gecko and Servo are using the ClipChain API for all clips and never using ClipNodes as positioning nodes, we can fix this for good.

Thank you for explanation! @staktrace do you know if there is an issue to file/track for this on Gecko side?

@kvark I don't believe there is; please feel free to file one

Thanks! Filed as https://bugzilla.mozilla.org/show_bug.cgi?id=1476044

kvark · 2018-07-13T18:48:43Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+# Clipping and Positioning in the Display List
+
+Each non-structural WebRender display list item has
+ * A `ClipId` of a `SpatialNode` or `ClipNode` for positioning


a clip id of a clip node for positioning - this rings a bell for me

kvark · 2018-07-13T18:48:43Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+to the evolutionary nature of the design. The general trend is that the display
+list gradually moves toward the internal representation. The most important of
+these incongruities is that while `ClipNodes`, sticky frames, and scroll frames
+are defined and simply return a ClipId, reference frames return a ClipId, but


nit: codify ClipId

kvark · 2018-07-13T18:48:43Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+## Converting `ClipId` to global `ClipScrollTree` indices
+
+WebRender must access `ClipNodes` and `SpatialNodes` quite a bit when building
+scenes and frames, so it tries to convert `CiipIds`, which are already


kvark · 2018-07-13T18:48:43Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+
+WebRender must access `ClipNodes` and `SpatialNodes` quite a bit when building
+scenes and frames, so it tries to convert `CiipIds`, which are already
+per-pipeline indices, to global scene-wide indices.  Internally this a


a verb is missing? ("this a")

kvark · 2018-07-13T18:48:43Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+scene-wide `ClipScrollTree`.
+
+Nodes are added to their respective arrays sequentially as the display list is
+processed during "flattening." When encountering an iframe, the


let's not include the "." in quote marks

So weirdly this is the style in American English, which I admit is bizarre. Since the rest of the document is written in American English though, maybe it makes sense to be consistent here.

gw3583 · 2018-07-16T02:27:41Z

This will be great to have updated, thanks!

mrobinson

Thanks for the review! I've updated the PR fixing the issues you mentioned and leaving comments in others.

mrobinson · 2018-07-16T06:57:44Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+# Original Design
+
+To understand the current design for clipping and positioning (transformations
+and scrolling) in WebRender if can be useful to have a little background about


mrobinson · 2018-07-16T06:57:44Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+    elements. Every display list item has an assigned `ClipChain` which
+    specifies what `ClipNodes` are applied to that item.
+
+The `SpatialNode` of the clips applied to each item are completely independent of


I changed this to make the inflection of the noun match the subject.

mrobinson · 2018-07-16T06:57:44Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+
+One holdover from the previous design is that both `ClipNode` and `SpatialNodes`
+have a parent node, which is either a `SpatialNode` or a `ClipNode`.  From this
+node WebRender can determine both a parent `ClipNode` and a parent `SpatialNode`


Nowadays there is no reason this has to be the case, but consider that in the past the only way to combine clip nodes was to have them be part of the ancestry in the ClipScrollTree. With the ClipChain API and the fact that an item can have both a positioning node and clip node assigned, it isn't strictly necessary. Once both Gecko and Servo are using the ClipChain API for all clips and never using ClipNodes as positioning nodes, we can fix this for good.

mrobinson · 2018-07-16T06:57:44Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+to the evolutionary nature of the design. The general trend is that the display
+list gradually moves toward the internal representation. The most important of
+these incongruities is that while `ClipNodes`, sticky frames, and scroll frames
+are defined and simply return a ClipId, reference frames return a ClipId, but


mrobinson · 2018-07-16T06:57:44Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+## Converting `ClipId` to global `ClipScrollTree` indices
+
+WebRender must access `ClipNodes` and `SpatialNodes` quite a bit when building
+scenes and frames, so it tries to convert `CiipIds`, which are already


mrobinson · 2018-07-16T06:57:44Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+
+WebRender must access `ClipNodes` and `SpatialNodes` quite a bit when building
+scenes and frames, so it tries to convert `CiipIds`, which are already
+per-pipeline indices, to global scene-wide indices.  Internally this a


mrobinson · 2018-07-16T06:57:44Z

webrender/doc/CLIPPING_AND_POSITIONING.md

+scene-wide `ClipScrollTree`.
+
+Nodes are added to their respective arrays sequentially as the display list is
+processed during "flattening." When encountering an iframe, the


So weirdly this is the style in American English, which I admit is bizarre. Since the rest of the document is written in American English though, maybe it makes sense to be consistent here.


        Improve clipping and positioning documentation

The new documentation gives a more complete and up-to-date picture of WebRender's clipping and positioning architecture.

kvark · 2018-07-16T16:53:55Z

@bors-servo r+

bors-servo · 2018-07-16T16:53:56Z

📌 Commit 8ad0431 has been approved by kvark

bors-servo · 2018-07-16T16:53:59Z

⌛ Testing commit 8ad0431 with merge 1505a50...


        Auto merge of #2892 - mrobinson:improve-doc, r=kvark

Improve clipping and positioning documentation The new documentation gives a more complete and up-to-date picture of WebRender's clipping and positioning architecture.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2892)

bors-servo · 2018-07-17T03:35:22Z

💥 Test timed out

gw3583 · 2018-07-17T03:54:49Z

@bors-servo retry

bors-servo · 2018-07-17T03:54:52Z

⌛ Testing commit 8ad0431 with merge de0ef91...


        Auto merge of #2892 - mrobinson:improve-doc, r=kvark

Improve clipping and positioning documentation The new documentation gives a more complete and up-to-date picture of WebRender's clipping and positioning architecture.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2892)

bors-servo · 2018-07-17T05:11:01Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing de0ef91 to master...

kvark requested changes Jul 13, 2018

View changes

mrobinson reviewed Jul 16, 2018

View changes

Improve clipping and positioning documentation

Loading status checks…

8ad0431

The new documentation gives a more complete and up-to-date picture of WebRender's clipping and positioning architecture.

mrobinson force-pushed the mrobinson:improve-doc branch from 8592ad5 to 8ad0431 Jul 16, 2018

kvark approved these changes Jul 16, 2018

View changes

bors-servo merged commit 8ad0431 into servo:master Jul 17, 2018
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

mrobinson deleted the mrobinson:improve-doc branch Jul 17, 2018

servo / webrender

Join GitHub today

Improve clipping and positioning documentation #2892

Improve clipping and positioning documentation #2892

Conversation

mrobinson commented Jul 13, 2018 • edited by larsbergstrom

leeoniya commented Jul 13, 2018

mrobinson commented Jul 13, 2018

staktrace commented Jul 13, 2018

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gw3583 commented Jul 16, 2018

mrobinson left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kvark commented Jul 16, 2018

bors-servo commented Jul 16, 2018

bors-servo commented Jul 16, 2018

bors-servo commented Jul 17, 2018

gw3583 commented Jul 17, 2018

bors-servo commented Jul 17, 2018

bors-servo commented Jul 17, 2018

mrobinson commented Jul 13, 2018 •

edited by larsbergstrom