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

[css-view-transitions-1] Replace 'snapshot viewport' with 'snapshot root' #8324

Merged
merged 3 commits into from
Jan 18, 2023

Conversation

bokand
Copy link
Contributor

@bokand bokand commented Jan 18, 2023

[css-view-transitions-1] Replace 'snapshot viewport' with 'snapshot root'

"Viewport" is already a heavily overloaded term and this rect isn't really a "viewport" in most senses so rename it to "snapshot root" to avoid confusion.

Also some minor improvements around its origin and how transforms are computed.

See also: #8070, #7859

This includes the scrollbars, but does not include the URL bar, as web content never appears in that area.
</figcaption>
</figure>

This means the snapshot canvas size is likely to be consistent for the [=document element=]'s [=captured element/old image=] and [=captured element/new element=].

The <dfn>snapshot viewport origin</dfn> refers to the start of the block and inline axes of the [=snapshot viewport=].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used only in reference to transforms which AFAIK are always based on x,y running positive starting from the top-left.

@@ -1210,7 +1210,7 @@ urlPrefix: https://html.spec.whatwg.org/multipage/rendering.html; type: dfn;

1. Set |height| to the current height of |capturedElement|'s [=new element=]'s [=border box=].

1. Set |transform| to a transform that maps the |capturedElement|'s [=new element=]'s [=border box=] from document origin to its quad from the [=snapshot viewport origin=].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit confused by what was meant here so maybe I'm just misunderstanding but "its quad from the snapshot viewport origin" is what we're trying to compute here so it seems a bit circular.

I also think the "border box from document origin" is wrong if we were to load the page scrolled (since the snapshot viewport is effectively fixed but the document origin is pushed up by the scroll)...instead we want to go from the snapshot rect origin to "element's border box relative to the snapshot rect origin".

@bokand bokand requested a review from jakearchibald January 18, 2023 04:06
@bokand
Copy link
Contributor Author

bokand commented Jan 18, 2023

@vmpstr @khushalsagar as FYI

@bokand bokand changed the title [CSS-VIEW-TRANSITIONS] Replace 'snapshot viewport' with 'snapshot root' [css-view-transitions-1] Replace 'snapshot viewport' with 'snapshot root' Jan 18, 2023
@@ -1210,7 +1210,7 @@ urlPrefix: https://html.spec.whatwg.org/multipage/rendering.html; type: dfn;

1. Set |height| to the current height of |capturedElement|'s [=new element=]'s [=border box=].

1. Set |transform| to a transform that maps the |capturedElement|'s [=new element=]'s [=border box=] from document origin to its quad from the [=snapshot viewport origin=].
1. Set |transform| to a transform that maps the [=snapshot root origin=] to the top-left corner of the |capturedElement|'s [=new element=]'s [=border box=].
Copy link
Contributor

@jakearchibald jakearchibald Jan 18, 2023

Choose a reason for hiding this comment

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

I'm trying to think of clearer ways to word this. What about:

  1. Set |transform| to a transform that would place |capturedElement|'s [=new element=]'s [=border box=] in its current visual position, from the [=snapshot root origin=].

Although I'm happy to go with what you have here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Settled on:

  1. Set |transform| to a transform that would map |capturedElement|'s [=new element=]'s [=border box=] from the [=snapshot root origin=] to its current visual position.

Also updated the equivalent prose in the setup phase to match.

Copy link
Contributor

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

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

LGTM. One comment about a line I struggled to get my head around, but that might just be me.

@bokand bokand merged commit 537adc8 into w3c:main Jan 18, 2023
@bokand
Copy link
Contributor Author

bokand commented Jan 18, 2023

Thanks for the review!

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

Successfully merging this pull request may close these issues.

2 participants