Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Owned resources may render to left or right of viewport #5583

Merged
merged 2 commits into from
Oct 13, 2016

Conversation

jridgewell
Copy link
Contributor

This allows owned resources to render when outside the viewport on the
x-axis (only). This works because owned resources will never be
prerendered until the owner explicitly allows it. So, the owner knows
better than Resources does at this point.

Fixes #5549.

This allows owned resources to render when outside the viewport on the
x-axis (only). This works because owned resources will never be
prerendered until the owner explicitly allows it. So, the owner knows
better than Resources does at this point.

Fixes ampproject#5549.
Copy link
Contributor

@dvoytenko dvoytenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Could you please also add a test? (I'm approving right away since I'm online spotingly).

Copy link
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on what Dima said.

@jridgewell
Copy link
Contributor Author

For reference, this was introduced in #4081. Working on tests.

Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe don't #5549 yet though, Carousel still needs to honor renderOutsideViewport value instead of just preloading +/- 1.

Also why should we get this far at all when an owner explicitly says prerender this child? Should that not skip all the visibility checks anyway?

@jridgewell
Copy link
Contributor Author

jridgewell commented Oct 13, 2016

Also why should we get this far at all when an owner explicitly says prerender this child?

  1. https://github.com/ampproject/amphtml/blob/f28e116/src/service/resources-impl.js#L550-L555
  2. https://github.com/ampproject/amphtml/blob/f28e116/src/service/resources-impl.js#L1393-L1410
  3. https://github.com/ampproject/amphtml/blob/f28e116/src/service/resources-impl.js#L1353-L1383
  4. https://github.com/ampproject/amphtml/blob/f28e116/src/service/resource.js#L505-L563

Carousel still needs to honor renderOutsideViewport value instead of just preloading +/- 1.

Carousel knows better than Resources does. Resources intentionally skips anything owned when doing it's own preloading. So any of Carousel's slides won't be preloaded by Resources.

But Carousel only tries to preload the next (or previous) slide, so no issue with trusting the preload call when the are owned.

@aghassemi
Copy link
Contributor

aghassemi commented Oct 13, 2016

@jridgewell The owner check can't just be on the horizontal case though. If a vertical element (e.g. say sidebar) takes ownership of its children and calls schedulePreload on an item that is not visible, it may still fail to preload if the distance < viewportBox.height * multipler / scrollPenalty; returns false.

@dvoytenko
Copy link
Contributor

@aghassemi This is a temporary fix. We can't yet manage the composite elements properly.

@jridgewell jridgewell merged commit 415385e into ampproject:master Oct 13, 2016
@cramforce
Copy link
Member

@zhouyx Could we merge this into canary?

@zhouyx
Copy link
Contributor

zhouyx commented Oct 14, 2016

👌

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
)

* Owned resources may render to left or right of viewport

This allows owned resources to render when outside the viewport on the
x-axis (only). This works because owned resources will never be
prerendered until the owner explicitly allows it. So, the owner knows
better than Resources does at this point.

Fixes ampproject#5549.

* Add tests
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
)

* Owned resources may render to left or right of viewport

This allows owned resources to render when outside the viewport on the
x-axis (only). This works because owned resources will never be
prerendered until the owner explicitly allows it. So, the owner knows
better than Resources does at this point.

Fixes ampproject#5549.

* Add tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants