Add support for "userContexts" argument to "browsingContext.setViewport" command. #876

lutien · 2025-02-11T14:19:05Z

A proposal to add "userContexts" argument to "browsingContext.setViewport" command.

Closes #851

lutien · 2025-02-11T15:27:01Z

index.bs

+
+<div algorithm="set viewport when a top-level traversable is created">
+
+When the [=set up a window environment settings object=]


It's probably a wrong place to hook on, but since WebDriver BiDi navigable created should happen when everything is set up, and likely we have to set up viewport even before preload scripts run we need something else here, and will have to add something to html spec. So this is more of placeholder for now.

Do we actually need to do this once per document, or only really once per navigable? It seems like ideally we run this once per top-level traversable, since we seem to skip over everything else, and then you can assert that the input navigable is a top-level traversable, and skip a bunch of checks below.

I think for now it would be better to have a sketch of under what conditions we want to run these steps (maybe "after creating a document in a new top-level traversable, before running the 'Run WebDriver BiDi preload scripts' steps") rather than being more specific in a way that's misleading.

That sounds good to me, it's basically what I wanted to specify, but didn't know how :)
Updated ✅

…rt" command.

jgraham · 2025-02-17T15:21:55Z

index.bs

@@ -2966,6 +2966,13 @@ between [=navigables=] and device pixel ratio overrides. It is initially empty.
 Note: this map is not cleared when the final session ends i.e. device pixel
 ratio overrides outlive any WebDriver session.

+A [=remote end=] has a <dfn>viewport overrides map</dfn> which is a weak map


I might make all the structs named rather than use anonymous ones i.e. something like

A <dfn for="viewport-configuration">viewport dimensions</dfn> is a [=struct=] with an [=struct/item=] named <dfn for="viewport-configuration">height</dfn> which is an integer and a [=struct/item=] named <dfn for="viewport-dimensions">width</dfn> which is an integer. A <dfn>viewport configuration</dfn> is a [=struct=] with an [=struct/item=] named <dfn for="viewport-configuration">viewport</dfn> which is a [=viewport-configuration/viewport dimensions=] or null and an [=struct/item=] named <dfn for="viewport-configuration">devicePixelRatio</dfn> which is a float or null. A [=remote end=] ahs a <dfn>viewport overrides map</dfn> which is a weak map between [=user contexts=] and [=viewport configuration=].

right, that's look nicer, updated

jgraham · 2025-02-17T16:01:14Z

index.bs

+<div algorithm>
+To <dfn>set device pixel ratio override</dfn> given |navigable| and |device pixel ratio|:
+
+1. Let |navigable| be the [=/navigable=] whose [=navigable/active document=] is


Is there a case where this isn't a no-op?

I guess now it should be handled in the command algorithm?

to me it read as Let |navigable| be |navigable|

jgraham · 2025-02-17T16:19:04Z

index.bs

+
+<div algorithm="set viewport when a top-level traversable is created">
+
+When the [=set up a window environment settings object=]


Do we actually need to do this once per document, or only really once per navigable? It seems like ideally we run this once per top-level traversable, since we seem to skip over everything else, and then you can assert that the input navigable is a top-level traversable, and skip a bunch of checks below.

I think for now it would be better to have a sketch of under what conditions we want to run these steps (maybe "after creating a document in a new top-level traversable, before running the 'Run WebDriver BiDi preload scripts' steps") rather than being more specific in a way that's misleading.

jgraham · 2025-02-17T16:23:37Z

index.bs

-      viewport=] to be the <code>width</code> of |viewport| in CSS pixels and
-      set the height of the |navigable|'s [=layout viewport=] to be the
-      <code>height</code> of |viewport| in CSS pixels.
+   1. If the implementation is unable to adjust the |navigable|'s [=layout viewport=]


We have this here; what happens if it fails when we try to adjust it in the user context case?

Yeah, that's a good question. I guess the implementation should know if they are able to adjust the layout to the requested settings without relation to the specific navigable. I moved this check outside and remove mention of the navigable. Let me know what do you think.

jgraham · 2025-02-17T16:27:40Z

index.bs


-   1. For the |navigable| and all [=descendant navigables=]:
+      1. If |user context| is null, return [=error=] with [=error code=] [=no such user context=].


This failure inside the loop means that we'd partially apply the command to any user contexts before the one that doesn't exist; we should probably fail if any don't exist.

Good point, made the validation loop first.

lutien · 2025-02-18T12:16:39Z

The build fails, but it doesn't look that it comes from my changes. Locally, I can reproduce it on main branch as well.

lutien · 2025-02-18T12:46:29Z

Alright, I figured out what is wrong: speced/bikeshed-boilerplate#135.

index.bs

juliandescottes · 2025-02-18T18:46:32Z

index.bs

+1. If |command parameters| [=map/contains=] "<code>userContexts</code>"
+   and |command parameters| [=map/contains=] "<code>context</code>",
+   return [=error=] with [=error code=] [=invalid argument=].


We should probably also raise an error if both parameters are missing? It will effectively be a noop and won't crash or anything, but the user should know about it. Otherwise they might assume it just applied setViewport to all navigables?

Yeah, that's true, added ✅

juliandescottes · 2025-02-18T18:52:19Z

index.bs

+
+      1. [=Set viewport=] with |navigable| and |viewport|.
+
+1. Run the [[cssom-view-1#resizing-viewports]] steps.


(not related to your PR, but those steps seem to require a document to run against?)

Yeah, looks like it, updated ✅

OrKoN · 2025-02-20T19:45:57Z

index.bs

+
+1. If [=viewport overrides map=] [=map/contains=] |user context|:
+
+   1. If |viewport overrides map|[|user context|] [=map/contains=] the <code>viewport</code> field:


I think this should be [=viewport overrides map=] (here and below)

OrKoN · 2025-02-20T19:46:23Z

index.bs

+
+1. If [=viewport overrides map=] [=map/contains=] |user context|:
+
+   1. If |viewport overrides map|[|user context|] [=map/contains=] the <code>viewport</code> field:


I think [=map/contains=] cannot be used here since |viewport overrides map|[|user context|] is a struct

OrKoN · 2025-02-20T19:51:23Z

index.bs

@@ -4648,71 +4659,154 @@ The <dfn export for=commands>browsingContext.setViewport</dfn> command modifies
   </dd>
 </dl>

-<div algorithm="remote end steps for browsingContext.setViewport">
+<div algorithm>


I think this helper algorithms could be moved outside of the command section so that remote end steps follow the CDDL type definitions for the command.

OrKoN · 2025-02-20T19:53:40Z

index.bs

-      viewport=] to be the <code>width</code> of |viewport| in CSS pixels and
-      set the height of the |navigable|'s [=layout viewport=] to be the
-      <code>height</code> of |viewport| in CSS pixels.
+1. If the <code>context</code> field of |command parameters| is present:


I think we could use [=map/contains=] instead of "is present"

OrKoN · 2025-02-20T19:54:47Z

index.bs


-      1. Otherwise:
+      1. [=map/Set=] [=viewport overrides map=][|user context|] to a new [=/map=].


isn't [=viewport overrides map=][|user context|] defined as a struct?

OrKoN · 2025-02-20T19:57:19Z

index.bs

+         1. Set |viewport overrides map|[|user context|]["<code>devicePixelRatio</code>"]
+            to |command parameters|["<code>devicePixelRatio</code>"].
+
+      1. [=list/For each=] |top-level traversable| in the list of all [=/top-level traversables=]


lutien commented Feb 11, 2025

View reviewed changes

Add support for "userContexts" argument to "browsingContext.setViewpo…

751867e

…rt" command.

lutien force-pushed the add-user-contexts-to-set-viewport branch from c607300 to 751867e Compare February 11, 2025 15:28

lutien marked this pull request as ready for review February 11, 2025 15:49

lutien requested a review from jgraham February 11, 2025 15:49

sadym-chromium mentioned this pull request Feb 12, 2025

Add support for "userContexts" argument to "browsingContext.setViewport" command #851

Open

jgraham requested changes Feb 17, 2025

View reviewed changes

lutien force-pushed the add-user-contexts-to-set-viewport branch from e9f1941 to 1259908 Compare February 18, 2025 11:33

Address review feedback

404847d

lutien force-pushed the add-user-contexts-to-set-viewport branch from 1259908 to 404847d Compare February 18, 2025 12:00

lutien closed this Feb 18, 2025

lutien reopened this Feb 18, 2025

lutien requested a review from jgraham February 18, 2025 18:12

juliandescottes reviewed Feb 18, 2025

View reviewed changes

Address review feedback

d11c34a

OrKoN reviewed Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for "userContexts" argument to "browsingContext.setViewport" command. #876

Add support for "userContexts" argument to "browsingContext.setViewport" command. #876

lutien commented Feb 11, 2025 •

edited by pr-preview bot

Loading

lutien Feb 11, 2025

jgraham Feb 17, 2025

lutien Feb 18, 2025

jgraham Feb 17, 2025

lutien Feb 18, 2025

jgraham Feb 17, 2025

lutien Feb 18, 2025

OrKoN Feb 20, 2025

jgraham Feb 17, 2025

jgraham Feb 17, 2025

lutien Feb 18, 2025

jgraham Feb 17, 2025

lutien Feb 18, 2025

lutien commented Feb 18, 2025

lutien commented Feb 18, 2025

juliandescottes Feb 18, 2025

lutien Feb 19, 2025

juliandescottes Feb 18, 2025

lutien Feb 19, 2025

OrKoN Feb 20, 2025 •

edited

Loading

OrKoN Feb 20, 2025

OrKoN Feb 20, 2025

OrKoN Feb 20, 2025

OrKoN Feb 20, 2025

OrKoN Feb 20, 2025


		<div algorithm="set viewport when a top-level traversable is created">

		When the [=set up a window environment settings object=]


		1. For the \|navigable\| and all [=descendant navigables=]:
		1. If \|user context\| is null, return [=error=] with [=error code=] [=no such user context=].


		1. [=Set viewport=] with \|navigable\| and \|viewport\|.

		1. Run the [[cssom-view-1#resizing-viewports]] steps.


		1. If [=viewport overrides map=] [=map/contains=] \|user context\|:

		1. If \|viewport overrides map\|[\|user context\|] [=map/contains=] the <code>viewport</code> field:


		1. Otherwise:
		1. [=map/Set=] [=viewport overrides map=][\|user context\|] to a new [=/map=].

	1. [=list/For each=] \|top-level traversable\| in the list of all [=/top-level traversables=]
	1. [=list/For each=] \|top-level traversable\| of the list of all [=/top-level traversables=]

Add support for "userContexts" argument to "browsingContext.setViewport" command. #876

Are you sure you want to change the base?

Add support for "userContexts" argument to "browsingContext.setViewport" command. #876

Conversation

lutien commented Feb 11, 2025 • edited by pr-preview bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lutien commented Feb 18, 2025

lutien commented Feb 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OrKoN Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lutien commented Feb 11, 2025 •

edited by pr-preview bot

Loading

OrKoN Feb 20, 2025 •

edited

Loading