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 2 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
45 changes: 29 additions & 16 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,22 @@ <h3><dfn>CropTarget</dfn> Definition</h3>
};
</pre>
<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:
<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>
Copy link

Choose a reason for hiding this comment

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

Can you suggest a link that we can use for the term "weak reference"?

Copy link
Member

Choose a reason for hiding this comment

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

Those are two terms: "weak" and "reference".

The precedent here is HTML which uses the word "weak" 12 times without defining it, and uses the word "reference" 176 times.

This PR seems consistent with precedent. The term "weak" is used in TAG design principles related to garbage collection, so it seems clear that a "reference" is implicitly strong (affects GC) unless it is preceeded by the word "weak". cc @annevk

<p>Create <var>cropTarget</var>.<dfn data-dfn-for="CropTarget">[[\Element]]</dfn> initialized to <var>weakRef</var>.</p>
<div class="note">
<p><var>cropTarget</var> is keeping a weak reference to the element it is representing.
In other words, <var>cropTarget</var> will not prevent garbage collection of its element.</p>
jan-ivar marked this conversation as resolved.
Show resolved Hide resolved
</div>
</li>
</ol>
</p>
</section>
<section id="producecroptarget-method">
<h3>MediaDevices.produceCropTarget</h3>
Expand All @@ -168,33 +184,27 @@ <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).

Calling {{MediaDevices/produceCropTarget}} on an {{HTMLElement}} of a
supported type associates that element 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>
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
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
Copy link
Member

Choose a reason for hiding this comment

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

nit: FYI I've done s/P/p for all of the doc in response to review feedback.

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 @@ -234,8 +244,11 @@ <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,
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,
Copy link
Member

@eladalon1983 eladalon1983 Mar 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this the core of the PR, according to the context set by the attached issue. I'd move everything else to separate PRs and discuss those changes separately.

I'd gladly merge this PR if:

  1. We leave just this paragraph in the PR (lines 236 and onwards).
  2. We remove reference to <var>cropTarget</var>.[[\Element]] and replaced it with Let <var>element</var> be the {{HTMLElement}} for which cropTarget was produced. Then...

(Btw, HTMLElement rather than Element, until we settle issue #13. I'm OK with Element personally, but let's wait for @alvestrand to elaborate his thinking on this topic.)

Copy link
Member

Choose a reason for hiding this comment

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

Let's make progress the regular way. I just spent a lot of time commenting on each thread here to make progress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Goal of this PR is: introduce the slot, use it in one place to define more precisely cropping. We can split the PR in two.
In the interest of time, I would tend to merge this PR, merge the serialisation PR on top of this one reusing the weak reference slot.

Copy link
Member

@eladalon1983 eladalon1983 Mar 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Let's make progress the regular way. I just spent a lot of time commenting on each thread here to make progress.

I don't understand this message.

introduce the slot

My concern with the slot is that I am not sure how it maps onto specifying implementation. I am not keen on leaving anything inside the CropTarget that could leak to a malicious application that's managed to take over the render process. So no indication of which Document it's running in, etc. Using an internal slot seems to suggest this.

the user agent MUST execute the following algorithm:
</p>
<ol>
Expand Down