Calculate current absolute position of fixed-position elements #5624

jridgewell · 2016-10-14T22:23:13Z

Currently, Resources caches the absolute position (layoutBox's top and bottom). This works perfectly for the majority of elements, since they never move absolutely.

But fixed-position elements are different. They are positioned relative to the viewport, and the viewport moves. So, fixed-positions "move" absolutely. This change makes it so that we cache the relative positions of fixed-positions (to the viewport). When we later ask for the element's layoutBox, we'll calculate the "absolute" position by adding in the current scrollTop and scrollLeft. This is exactly the same as what I'm doing for ad's layoutBox.

Part 2 (and hopefully the final) of #5149.
/cc @jasti.

camelburrito · 2016-10-14T23:29:34Z

src/base-element.js

@@ -172,8 +172,7 @@ export class BaseElement {
   * @return {!./layout-rect.LayoutRectDef}
   */
  getLayoutBox() {
-    return this.element.getResources().getResourceForElement(
-        this.element).getLayoutBox();
+    return this.element.getLayoutBox();


is this a DOM api? i think this should be from resources-impl - like how it was before?

It's defined in custom-element.js. It does exactly this.

camelburrito · 2016-10-17T19:20:37Z

LGTM - @dvoytenko do you want to take a pass?

dvoytenko · 2016-10-17T20:48:07Z

Yes. Looking.

dvoytenko · 2016-10-17T21:03:43Z

src/custom-element.js

@@ -444,9 +444,6 @@ function createBaseCustomElementClass(win) {
      this.layout_ = Layout.NODISPLAY;

      /** @private {number} */
-      this.layoutWidth_ = -1;


What's going to happen to this var? I still see it used throughout in this class.

You're looking at an old diff.

No, I think I'm looking at the right one. This appears to be a9808200539a76eb965b6c6f7d0aba79cce8a4e1 commit...

Whoops. Re-added.

dvoytenko · 2016-10-17T21:04:27Z

src/custom-element.js

@@ -707,6 +704,7 @@ function createBaseCustomElementClass(win) {
        this.implementation_.layoutWidth_ = this.layoutWidth_;
      }
      // TODO(malteubl): Forward for stubbed elements.
+      // TODO(jridgewell): We should pass the layoutBox down.


It's passed. Right?

this.implementation_.onLayoutMeasure();. We should pass it to #onLayoutMeasure, because a lot of them immediately call #getLayoutBox.

Ok, that'd be fine, of course. But we'd keep getLayoutBox() method itself? I made more notes on this below as well. Ideally this would be only size. However, in the meantime, what this API promises is to return "the last measured/known layout box".

But we'd keep getLayoutBox() method itself?

Yup, we need that else where.

Ideally this would be only size.

Yah, just passing { width, height } would satisfy most all of them. Anyone else could call #getPosition or whatever we end up calling it.

dvoytenko · 2016-10-17T21:18:39Z

src/service/resource.js

+      return this.layoutBox_;
+    }
+    const viewport = this.resources_.getViewport();
+    return moveLayoutRect(this.layoutBox_, viewport.getScrollLeft(),


Hmm. This is a bit troublesome. The definition of the getLayoutBox is that it doesn't change based on the scroll state. We are de-facto changing it here. I'm not sure what downstream issues we maybe expecting from this. The truth is, of course, that fixed element's layout box doesn't change but defined within another (fixed) layer. I think a better option for us would be:

Modify overlaps method to incorporate the knowledge of isFixed. We can even rename it as overlapsViewport. IMO, this is better b/c this is a more direct API that's only used for the purposes of scheduling.

At this time, propagating getLayoutBox is rather dangerous. I don't know who uses it and where. I think it's best to replace this call with getLayoutSize() or getLayoutWidth/Height pair. I believe most of clients only care about size and not absolute position.

The definition of the getLayoutBox is that it doesn't change based on the scroll state.

I've taken it to mean "give me the absolute position from the origin (0, 0)", which is how everything (until we introduced #isFixed) treated it. This just fixes the value fixed-position's return.

Modify overlaps method to incorporate the knowledge of isFixed.

The overlaps stuff is totally superficial to the PR. I need to fix #getLayoutBox for Flying Carpet, which depends on it returning an absolute position.

I don't know who uses it and where. I think it's best to replace this call with getLayoutSize() or getLayoutWidth/Height pair.

I'd love to see this. Should we wait for the Layers work?

Yeah, it should definitely wait for layers work. Or, alternatively we can do a refactoring pass to clarify this API and remove exposing left/top to clients that don't need it.

I've taken it to mean "give me the absolute position from the origin (0, 0)", which is how everything (until we introduced #isFixed) treated it. This just fixes the value fixed-position's return.

This might be a mis-understanding, but this is not how I read this code. I definitely understand that the current implementation is wrong, but not yet clear that this fixes it.

What does "origin (0, 0)" mean: document origin or viewport origin? From what I see, (assuming we measured the fixed element once at first) if I call this method twice at different scroll positions (e.g. scrollTop=100 and 200) this method will now return different results. I'm not sure if that'd improve things.

Or, alternatively we can do a refactoring pass to clarify this API and remove exposing left/top to clients that don't need it.

That works.

What does "origin (0, 0)" mean: document origin or viewport origin?

The top left corner of the document, not the viewport.

if I call this method twice at different scroll positions (e.g. scrollTop=100 and 200) this method will now return different results.

Exactly. Because the fixed-position has "moved" relative to the origin. Because fixed-position's are fixed relative to the viewport (usually), and the viewport moves down the document, so fixed-positions move.

I see. Alright.

dvoytenko · 2016-10-17T21:56:14Z

src/custom-element.js

@@ -444,9 +444,6 @@ function createBaseCustomElementClass(win) {
      this.layout_ = Layout.NODISPLAY;

      /** @private {number} */
-      this.layoutWidth_ = -1;


No, I think I'm looking at the right one. This appears to be a9808200539a76eb965b6c6f7d0aba79cce8a4e1 commit...

dvoytenko · 2016-10-17T21:57:21Z

src/custom-element.js

@@ -707,6 +704,7 @@ function createBaseCustomElementClass(win) {
        this.implementation_.layoutWidth_ = this.layoutWidth_;
      }
      // TODO(malteubl): Forward for stubbed elements.
+      // TODO(jridgewell): We should pass the layoutBox down.


Ok, that'd be fine, of course. But we'd keep getLayoutBox() method itself? I made more notes on this below as well. Ideally this would be only size. However, in the meantime, what this API promises is to return "the last measured/known layout box".

This makes the Resource’s `layoutBox` aware of fixed-position behavior. We now store the _relative_ position to the viewport for fixed-positions. When the element requests it’s current `layoutBox`, we’ll return the absolute position from (0, 0) based on the current `scrollTop` and `scrollLeft`.

jridgewell · 2016-10-18T22:07:39Z

@dvoytenko: Can you take one last look, added two more commits for testing.

dvoytenko · 2016-10-19T18:25:08Z

LGTM

…oject#5624) * tmp * Make layoutBox rects aware of fixed position movement This makes the Resource’s `layoutBox` aware of fixed-position behavior. We now store the _relative_ position to the viewport for fixed-positions. When the element requests it’s current `layoutBox`, we’ll return the absolute position from (0, 0) based on the current `scrollTop` and `scrollLeft`. * Fix out of order assignment * Re-add #layoutWidth. Accidental deletion. * Fix const assignments * Fix calcTaskScore_ tests for fixeds * Fix setting initialLayoutBox_

jridgewell assigned dvoytenko and camelburrito Oct 14, 2016

camelburrito reviewed Oct 14, 2016

View reviewed changes

dvoytenko suggested changes Oct 17, 2016

View reviewed changes

erwinmombay mentioned this pull request Oct 17, 2016

Meeting Notes 2016-10-17 #5645

Closed

dvoytenko suggested changes Oct 17, 2016

View reviewed changes

dvoytenko approved these changes Oct 17, 2016

View reviewed changes

camelburrito approved these changes Oct 18, 2016

View reviewed changes

jridgewell force-pushed the fixed-position-intersection-observer-part-2 branch from 2a2132c to 09c2437 Compare October 18, 2016 18:43

jridgewell added 5 commits October 18, 2016 14:43

tmp

50f19a7

Fix out of order assignment

9c88bf9

Re-add #layoutWidth. Accidental deletion.

e26fcdd

Fix const assignments

d8b2b48

jridgewell force-pushed the fixed-position-intersection-observer-part-2 branch from 09c2437 to d8b2b48 Compare October 18, 2016 18:43

jridgewell added 2 commits October 18, 2016 16:22

Fix calcTaskScore_ tests for fixeds

3214e82

Fix setting initialLayoutBox_

3869f24

jridgewell force-pushed the fixed-position-intersection-observer-part-2 branch from a252937 to 3869f24 Compare October 18, 2016 21:02

jridgewell merged commit feb5bfc into ampproject:master Oct 19, 2016

jridgewell deleted the fixed-position-intersection-observer-part-2 branch October 19, 2016 19:33

This was referenced Oct 19, 2016

Add Flying Carpet to FixedLayer #5701

Merged

IntersectionObserver: Fixed Position Elements return incorrect Results #5149

Closed

dvoytenko mentioned this pull request Oct 20, 2016

Ensure that an element is measured when resize is scheduled #5708

Merged

erwinmombay mentioned this pull request Oct 24, 2016

Meeting Notes 2016-10-24 #5788

Closed

jridgewell mentioned this pull request Nov 9, 2016

intersection observer polyfill #6100

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calculate current absolute position of fixed-position elements #5624

Calculate current absolute position of fixed-position elements #5624

jridgewell commented Oct 14, 2016 •

edited

camelburrito Oct 14, 2016

jridgewell Oct 17, 2016 •

edited

camelburrito commented Oct 17, 2016

dvoytenko commented Oct 17, 2016

dvoytenko Oct 17, 2016

jridgewell Oct 17, 2016

dvoytenko Oct 17, 2016

jridgewell Oct 17, 2016

dvoytenko Oct 17, 2016

jridgewell Oct 17, 2016

dvoytenko Oct 17, 2016

jridgewell Oct 17, 2016

dvoytenko Oct 17, 2016

jridgewell Oct 17, 2016

dvoytenko Oct 17, 2016

jridgewell Oct 17, 2016

dvoytenko Oct 17, 2016

dvoytenko Oct 17, 2016

dvoytenko Oct 17, 2016

jridgewell commented Oct 18, 2016

dvoytenko commented Oct 19, 2016

Calculate current absolute position of fixed-position elements #5624

Calculate current absolute position of fixed-position elements #5624

Conversation

jridgewell commented Oct 14, 2016 • edited

Choose a reason for hiding this comment

jridgewell Oct 17, 2016 • edited

Choose a reason for hiding this comment

camelburrito commented Oct 17, 2016

dvoytenko commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell commented Oct 18, 2016

dvoytenko commented Oct 19, 2016

jridgewell commented Oct 14, 2016 •

edited

jridgewell Oct 17, 2016 •

edited