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

Ensure that viewport dimensions are updated when expanding/collapsing iframe #9897

Merged
merged 2 commits into from Jun 27, 2017

Conversation

alanorozco
Copy link
Member

@alanorozco alanorozco commented Jun 13, 2017

Uses known information about the frame collapsed state. Invalidates cached rect if the viewport changes.

Copy link
Contributor

@lannka lannka left a comment

Choose a reason for hiding this comment

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

Did I read correctly: the CachedValueObservable and CANCEL_FULL_OVERLAY_FRAME_REMEASURE were introduced for an optimization, which is to not remeasure inside the inabox runtime for every scroll or resize when it's in overlay mode.

If so, would the following work:

  • we store the latest position change locally, and skip remeasureAll if it's in overlay mode
  • when CANCEL_FULL_OVERLAY_FRAME_RESPONSE is returned, we use the latest viewport position to do a remeasureAll.

@alanorozco
Copy link
Member Author

alanorozco commented Jun 21, 2017

@lannka

Just doing that would lead to an incorrect state if the viewport changes while the overlay is open. CANCEL_FULL_OVERLAY_FRAME_REMEASURE is not an optimization, it actually updates the state in case it's wrong. (Box rect changes when expanded/collapsed. The collapsed measurement might be incorrect if the viewport changes when the lightbox is expanded).

@lannka
Copy link
Contributor

lannka commented Jun 21, 2017

Any viewport change of the host page should be sent to the iframe, even when overlay is open. Why there will be incorrect state?

@alanorozco
Copy link
Member Author

alanorozco commented Jun 21, 2017

Because the collapsed box rect hasn't been recalculated after the viewport change. We know what the box rect looks like when expanded, but not what it will look like when being collapsed again.

@lannka
Copy link
Contributor

lannka commented Jun 21, 2017

That part makes sense. But I still don't understand why host has to respond 2 separate responses, and upon receiving those responses, client remeasure twice.

Is that an optimization because most of the time the box won't change (since we freeze the scroll), so the 2nd response is considered as a sanity check adjustment ?

@alanorozco
Copy link
Member Author

alanorozco commented Jun 21, 2017

Yep. If we always remeasure we're always going to skip a frame because recalculation requires a vsync pass. It's not just optimization for optimization sake, it would be bad UX otherwise since elements could jump or the change would be laggy.

@alanorozco alanorozco force-pushed the lightbox-x branch 5 times, most recently from 8ebba43 to 1a8c7a1 Compare June 23, 2017 02:04
@alanorozco
Copy link
Member Author

alanorozco commented Jun 23, 2017

@lannka PTAL

Moved the collapsed rect cache logic to host per off-thread discussion. Refactored it so it would be simpler. Even the tests are simpler using this approach!

@alanorozco
Copy link
Member Author

@lannka ping

@lannka
Copy link
Contributor

lannka commented Jun 27, 2017

Awesome! Thanks for the change.

@alanorozco alanorozco merged commit f2b1528 into ampproject:master Jun 27, 2017
@alanorozco alanorozco deleted the lightbox-x branch June 27, 2017 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants