Add scrollIntoView action - #1964
Conversation
| given optional arguments <var>onlyIfNecessary</var> (default true), | ||
| <var>behavior</var> (default "<code>instant</code>"), | ||
| <var>block</var> (default "<code>end</code>") | ||
| and <var>inline</var> (default "<code>nearest</code>"): |
There was a problem hiding this comment.
Not sure if nearest would work as default. Usually we use the element's in-view center point for the interaction and scrolling only as little as possible most likely would still keep the majority of the element outside of the viewport. Should we maybe use end here as well similar to what we use for block?
There was a problem hiding this comment.
I wanted the scrollIntoView action defaults to match what WebDriver does for the interaction commands.
| <dt><a>Logical scroll position "<code>inline</code>"</a> | ||
| <dd>"<code>nearest</code>" | ||
| <li><p>If <var>onlyIfNecessary</var> is true | ||
| and <var>target</var> is <a data-lt="range is in view">in view</a>, return. |
There was a problem hiding this comment.
This renders in view similarly to the above option and is confusing. We most likely want target's <a>range is in view</a> or similar.
Also here we only test the viewport overlapping for the first client rect (shouldn't we take all into account?).
Per AI it might cause issues with overflow: hidden when in view would return true but it's actually clipped by the container. So onlyIfNecessary would wrongly skip the scroll. If that's acceptable it might warrant at least a note. By lets see what @jgraham will think about it.
There was a problem hiding this comment.
I changed it to the <var>target</var> <a>range is in view</a> (not target's range because target itself is a range here).
Also here we only test the viewport overlapping for the first client rect (shouldn't we take all into account?).
Again I tried to match how scrolling into view is done for WebDriver interaction commands: the in view check uses the pointer-interactable paint tree which uses the element's in-view center point which is "the intersection between the element's first DOMRect of getClientRects() and the initial viewport" (but I was surprised by this logic as well).
| <li><p>Let <var>inline</var> be the <code>inline</code> | ||
| property of <var>action object</var>. | ||
|
|
||
| <li><p><a>Scroll into view</a> <var>node</var> with |
There was a problem hiding this comment.
If we have a scrolling behavior of auto or smooth the scrolling will be asynchronous. We should wait until the scrolling is done before continuing dispatching the remaining actions.
There was a problem hiding this comment.
I assumed this is already covered by waiting for "pending asynchronous waits arising from the last invocation of the dispatch tick actions steps" after dispatching an action because scroll a target into view returns a Promise that resolves when scrolling is done.
@jgraham did I understand this correctly?
|
|
||
| <li><p>Run <a>Function.[[\Call]]</a>(<a>scrollIntoView</a>, <var>options</var>) | ||
| with <var>element</var> as the this value. | ||
| <li><p><a>Scroll <var>target</var> into view</a> |
There was a problem hiding this comment.
Is it ok to put a var inside an anchor?
There was a problem hiding this comment.
It's not ideal, so I changed it to <a>Scroll into view</a> <var>target</var>.
| <var>action</var>, and <var>actions options</var>:</p> | ||
|
|
||
| <ol class="algorithm"> | ||
| <li><p>Let <var>node</var> be the result of <a>getting the property</a> |
There was a problem hiding this comment.
Some lines up we use getting a property. What shall we use?
There was a problem hiding this comment.
Both getting the property and getting a property are used throughout the spec.
efcfe63 to
d55acf1
Compare
d55acf1 to
9cbc5d6
Compare
This PR adds
scrollIntoViewto the general actions.The action should also be able to scroll text nodes (which can be passed in from WebDriver BiDi) and elements with
display: contentsinto view. To this end, the "scroll into view" steps are updated to create aRangeobject for such nodes and call the "Scroll target into view" steps in the CSSOM View spec (which supportRanges) directly.Note that this is a breaking change for interaction commands that implicitly scroll an element into view: previously, the implicit attempt to scroll an element with
display: contentsinto view would do nothing but now it will be scrolled into view.Fixes #1005.
This change is
Preview | Diff