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

Editorial: formalize screenshot waiting algorithm #538

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jugglinmike
Copy link
Contributor

@jugglinmike jugglinmike commented Sep 12, 2023

Alternatively, we might define an algorithm for HTML to explicitly invoke during Update the rendering. There's certainly precedence for this, as HTML already signals three other specifications near the moment we're interested in here in BiDi.

By integrating with requestAnimationFrame, this approach precludes coordination with HTML, but it's not without its downsides, e.g.

  • calling out to ECMAScript might be distracting for some readers
  • the scheduling is observable to authors' code in that it increments the target's animation frame callback identifier
  • authors' rAF callbacks may execute after this algorithm resumes

...and with that, I've all but talked myself out of this patch :P


Preview | Diff

@whimboo whimboo added enhancement New feature or request module-browsingContext Browsing Context module labels Sep 13, 2023
Copy link
Contributor

@jrandolf-2 jrandolf-2 left a comment

Choose a reason for hiding this comment

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

I think this sounds reasonable. Would like some confirmation from others in Firefox before we give our LGTM.

index.bs Outdated
<div algorithm>
To <dfn>await the next animation frame</dfn> given |window|:

1. Let |render id| be the string representation of a
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do opaque implementation-specific string. That way implementations aren't bound by UUID.

Copy link
Member

Choose a reason for hiding this comment

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

I note in passing that render id isn't observable either way, so making it a UUID in the spec or not doesn't actually constrain implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as it doesn't matter either way, I'm happy to take @jrandolf's suggestion since that seems less distracting (though I used HTML's wording: "new unique opaque string").

Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

I believe the correct way to do this would be with a specific step in https://html.spec.whatwg.org/#update-the-rendering (c.f. mark paint timing) rather than invoking DOM APIs.

index.bs Outdated
<div algorithm>
To <dfn>await the next animation frame</dfn> given |window|:

1. Let |render id| be the string representation of a
Copy link
Member

Choose a reason for hiding this comment

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

I note in passing that render id isn't observable either way, so making it a UUID in the spec or not doesn't actually constrain implementations.

@jugglinmike
Copy link
Contributor Author

I believe the correct way to do this would be with a specific step in https://html.spec.whatwg.org/#update-the-rendering (c.f. mark paint timing) rather than invoking DOM APIs.

Great minds think alike--that's exactly what I was getting at in the description of this pull request! I've added a commit to use that approach, instead. I'm expecting folks will want to workshop the design some more, so I'm going to hold off on submitting anything to HTML. But to help explain this patch, here's how it could be integrated in its current form:

      <li><p>For each <span>fully active</span> <code>Document</code> <var>doc</var> in
      <var>docs</var> run <span>process top layer removals</span> given <var>doc</var>.</p></li>

+     <li><p>For each <span>fully active</span> <code>Document</code> <var>doc</var> in
+     <var>docs</var> run <span>resume suspended renderings</span> given <var>doc</var>'s
+     <span>browsing context</span>. <ref>WEBDRIVERBIDI</ref></p></li>
     </ol>
    </li>

@jugglinmike
Copy link
Contributor Author

I just realized that BiDi has a formal mechanism for tracking patches to external specs. I've pushed up a commit to include the change I described in my previous comment, and I've resolved merge conflicts from recent changes in the main branch.

What do you think, @jgraham?

Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

This basically seems oK< but I really would prefer the patch in HTML if possible. We don't have great luck with HTML considering other specifications when refactoring, and having local patches makes it worse.


<div algorithm="extension to update the rendering in HTML">

1. For each [=fully active=] <code>Document</code> |doc| in <var
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the term "suspended renderings"; it sounds too much like we're preventing paints from happening, which isn't true, we're just awaiting an animation frame.

Also, on the HTML side pass in the document, not the browsing context. We want to move away from the use of browsing context, so it's easier to do the book keeping entirely on the WebDriver side.

@@ -1971,6 +1971,11 @@ implicitly set when the context is created. For browsing contexts with an
associated WebDriver [=window handle=] the [=/browsing context id=] must be the
same as the [=window handle=].

Each [=/browsing context=] has an associated <dfn>list of suspended rendering
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 add properties directly on the remote end (or session) rather than on the browsing context. In this case a map between browsing context id and animation frame callback ids would work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request module-browsingContext Browsing Context module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants