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

Formalize the restriction transformation #13

Merged
merged 16 commits into from
Jul 17, 2023

Conversation

eladalon1983
Copy link
Contributor

@eladalon1983 eladalon1983 commented Jun 15, 2023

This PR formalizes what restrictTo() does a bit better, but does not yet go all the way. (Hence some TODOs.)


Preview | Diff

@eladalon1983 eladalon1983 requested a review from markafoltz June 15, 2023 16:38
Copy link
Collaborator

@markafoltz markafoltz left a comment

Choose a reason for hiding this comment

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

Some initial comments to align this with parts of view transitions, which has some of the same requirements. And some other thoughts which are tangential to this specific PR.

What are you thinking should happen if an element violates some of the conditions that make it capturable? It seems like with region capture, only the capturing application gets an indirect signal that the capture is failing (i.e. no new frames are produced).

From an ergonomics point of view, it may be worth having some signal back to the captured application as to why capture is not working. We could probably wire up some debug messages in devtools, but a more explicit API in script could also be an option.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@eladalon1983
Copy link
Contributor Author

Some initial comments to align this with parts of view transitions

At the moment, I see view transitions as an implementation artifact of Chrome, and not a requirement we should consciously add to the spec. I think that if we phrase the spec correctly, theoretical implementation may choose to render twice, or to force all other stacking contexts to become transparent, or to use any other approach - so long as the result is identical.

From an ergonomics point of view, it may be worth having some signal back to the captured application as to why capture is not working.

The captured application is not necessarily even aware that it's being captured. Consider:

  • TLD (top-level document) embeds ED (embedded document).
  • ED sends a CropTarget.
  • <5 minutes later meme>
  • TLD calls gDM(preferCurrentTab: true) or even getViewportMedia().
  • TLD uses the CropTarget it previously got.

I think it's up to the capturer to inform the capturee, if necessary. I think the browser should not tell the captured application that it's being captured.

@eladalon1983
Copy link
Contributor Author

@baylesj, could you please also take a look?

@eladalon1983
Copy link
Contributor Author

I've made significant changes to this PR, so PTAL, @mfoltzgoogle and @baylesj.

index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
@baylesj
Copy link

baylesj commented Jul 14, 2023

@baylesj, could you please also take a look?

Overall looks good to me. Is there a straightforward way to view this as rendered and styled HTML to check that part of it?

@eladalon1983
Copy link
Contributor Author

@baylesj, could you please also take a look?

Overall looks good to me. Is there a straightforward way to view this as rendered and styled HTML to check that part of it?

image

Check out these two links.

index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
@khushalsagar
Copy link

Discussed offline, we'll file a csswg issue to resolve the question on implicit vs explicit rendering constraints.

@eladalon1983 eladalon1983 merged commit a30cc82 into screen-share:main Jul 17, 2023
@eladalon1983
Copy link
Contributor Author

Thanks for the review everyone!

Mark, apologies for not waiting for an official LGTM, but I think all of your comments have been addressed, and I'd like to send an i2p for this earlier. If you have any more open issues, I'd be happy to deal with them in follow-up PRs.

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.

4 participants