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.

Sign up for GitHub

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

Refer to getBoundingClientRect as a defintion of the actual crop area #32

Merged
merged 3 commits into from
Mar 31, 2022
Merged

Refer to getBoundingClientRect as a defintion of the actual crop area #32

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a6f0b06
Make CropTarget have an internal slot to keep a reference to its elem…
youennf Mar 23, 2022
a788c0e
Update index.html
youennf Mar 28, 2022
1acb49b
Merge branch 'main' into introduce-element-as-internal-slot
eladalon1983 Mar 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 45 additions & 27 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,13 +152,35 @@ <h3><dfn>CropTarget</dfn> Definition</h3>
// Intentionally empty; just an opaque identifier.
};
</pre>
<dl data-link-for="CropTarget" data-dfn-for="CropTarget"></dl>
<div class="note">
<p>
There is no consensus yet on the name for {{CropTarget}}. This is under discussion in
<a href="https://github.com/w3c/mediacapture-region/issues/18">issue #18</a>.
</p>
</div>
<dl data-link-for="CropTarget" data-dfn-for="CropTarget"></dl>
<p>
To <dfn data-export>create a CropTarget</dfn> with <var>element</var> as input, run the
following steps:
</p>
<ol>
<li>
<p>Let <var>cropTarget</var> be a new object of type {{CropTarget}}.</p>
</li>
<li>
<p>Let <var>weakRef</var> be a weak reference to <var>element</var>.</p>
<p>
[=Create a CropTarget|Create=] <var>cropTarget</var>.[[\Element]] initialized to
<var>weakRef</var>.
</p>
<div class="note">
<p>
<var>cropTarget</var> keeps a weak reference to the element it represents. In other
words, <var>cropTarget</var> will not prevent garbage collection of its element.
</p>
</div>
</li>
</ol>
<p>
{{CropTarget}} objects are serializable. The [=serialization steps=], given
<var>value</var>, <var>serialized</var>, and a boolean <var>forStorage</var>, are:
Expand Down Expand Up @@ -199,36 +221,26 @@ <h3>MediaDevices.produceCropTarget</h3>
</dt>
<dd>
<p>
The first call of {{MediaDevices/produceCropTarget}} on an {{HTMLElement}} of a
supported type, permanently associates that element with a {{CropTarget}}. Subsequent
calls return the same {{CropTarget}}. This {{CropTarget}} may be used as input to
Comment on lines -203 to -204
Copy link
Member

Choose a reason for hiding this comment

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

It seems useful to specify that two CropTargets produced from the same Element would be treated as equal. This ensures that if things only happen when changing crop-targets, then they won't happen if changing between equivalent targets. For example:

  const ct_a1 = produceCropTarget(document.getElementById('id_a'));
  const ct_a2 = produceCropTarget(document.getElementById('id_a'));
  const ct_b = produceCropTarget(document.getElementById('id_b'));
  mst.cropTo(ct_a1);
  mst.cropTo(ct_a2); // No-op.
  mst.cropTo(ct_b);  // Yes-op.

Copy link
Member

Choose a reason for hiding this comment

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

(lost this comment somehow)

"Treated as equal" is an optimization that should be taken based on underlying realities in the cropTo algorithm, orthogonally to any API decision here. Instead, what seems at stake here is merely:

const ct_a1 = produceCropTarget(document.getElementById('id_a'));
const ct_a2 = produceCropTarget(document.getElementById('id_a'));
console.log(ct_1 === ct_2); // true or false?

I do agree though this can be left for a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We no longer need to specify these rules as they are connected to the same element.
By having a weak reference slot, this kind of ambiguities disappear.

Copy link
Member

Choose a reason for hiding this comment

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

I agree "we no longer need" it, if we ever did, but that's different from whether this PR needs it. I don't have a strong opinion so whatever gets this merged faster.

Whether ct_1 === ct_2 is true or false is something we need to specify at least down the road though, and seems possibly tied to #11 (e.g. element.cropTarget would be compatible with true not false).

{{BrowserCaptureMediaStreamTrack/cropTo}}.
Calling {{MediaDevices/produceCropTarget}} on an {{HTMLElement}} of a supported type
associates that {{HTMLElement}} with a {{CropTarget}}. This {{CropTarget}} may be used
as input to {{BrowserCaptureMediaStreamTrack/cropTo}}. We define a
<dfn>valid CropTarget</dfn> as one returned by a previous call to
{{MediaDevices/produceCropTarget()}} in the current [=browsing context=].
</p>
<p>
We define a <dfn>valid CropTarget</dfn> as one returned by a previous call to
{{MediaDevices/produceCropTarget()}} in the current top-level document [=browsing
context=] or in one of its descendant [=browsing context=].
</p>
<p>
When {{MediaDevices/produceCropTarget}} is first called on a given <var>element</var>,
the user agent MUST produce a new unique {{CropTarget}} and permanently associate it
with <var>element</var>. The user agent MUST return a {{Promise}} <var>p</var>. The
user agent MUST resolve <var>p</var> only after it has finished all the necessary
internal propagation of state associated with the new {{CropTarget}}, at which point
the user agent MUST be ready to receive the new {{CropTarget}} as a valid parameter to
When {{MediaDevices/produceCropTarget}} is called on a given <var>element</var>, the
user agent [=create a CropTarget|creates a CropTarget=] with <var>element</var> as
input. The user agent MUST return a {{Promise}} <var>p</var>. The user agent MUST
resolve <var>p</var> only after it has finished all the necessary internal propagation
of state associated with the new {{CropTarget}}, at which point the user agent MUST be
ready to receive the new {{CropTarget}} as a valid parameter to
{{BrowserCaptureMediaStreamTrack/cropTo}}.
</p>
<p>
Subsequent calls to {{MediaDevices/produceCropTarget}} MUST return a new {{Promise}}
that behaves the same way <var>p</var> does. Specifically, the new {{Promise}} MUST
resolve after <var>p</var>, and MUST resolve to the same {{CropTarget}} as
<var>p</var>.
</p>
<p>
When cloning an {{HTMLElement}} on which {{MediaDevices/produceCropTarget}} was
previously called, the clone MUST NOT be associated with any {{CropTarget}}. If
previously called, the clone is not associated with any {{CropTarget}}. If
{{MediaDevices/produceCropTarget}} is later called on the clone, a new {{CropTarget}}
MUST be assigned to it.
will be assigned to it.
Comment on lines 240 to +243
Copy link
Member

Choose a reason for hiding this comment

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

This PR seems to tackle a few more issues than it set out to tackle. Can we discuss these changes separately? I'm not necessarily opposed, mind you.

Copy link
Member

Choose a reason for hiding this comment

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

To me, this seems related to @youennf's change above to produce a new cropTarget every time, which makes this normative prose redundant. We could remove it entirely as well, since a cloned element is clearly a new element, so I don't see why we need to say anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I made it non normative since it is no longer needed. I would be for removing it entirely.

Copy link
Member

Choose a reason for hiding this comment

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

@youennf's change above to produce a new cropTarget every time

I did not understand this change as requiring a new CropTarget each time, but rather as permitting it.

Copy link
Member

Choose a reason for hiding this comment

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

@youennf's change above to produce a new cropTarget every time

I did not understand this change as requiring a new CropTarget each time, but rather as permitting it.

</p>
</dd>
</dl>
Expand Down Expand Up @@ -289,9 +301,15 @@ <h3>BrowserCaptureMediaStreamTrack</h3>
<dd>
<p>
Calls to this method instruct the user agent to start/stop cropping a [=self-capture
video track=] to the contours of the element referenced by
<var>cropTarget</var>. Whenever {{BrowserCaptureMediaStreamTrack/cropTo}} is invoked,
the user agent MUST execute the following algorithm:
video track=] to the
<a href="https://drafts.csswg.org/cssom-view/#dom-element-getboundingclientrect">
bounding client rectangle</a
>
of <var>cropTarget</var>.[[\Element]]. Since the track is restricted to the visible
viewport of the [=display-surface=], the captured area will be the intersection of the
visible viewport and the element bounding client rectangle. Whenever
{{BrowserCaptureMediaStreamTrack/cropTo}} is invoked, the user agent MUST execute the
following algorithm:
</p>
<ol>
<li>
Expand Down