1570605 - "element.scrollIntoView()" uses invalid scrolling behavior of "instant"

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As noticed yesterday in some cases the method element.scrollIntoView returns too early, and as such the element is not yet visible inside the viewport. In case of taking screenshots the wrong part of the web page will be taken.

https://searchfox.org/mozilla-central/rev/b38e3beb658b80e1ed03e0fdf64d225bd4a40327/testing/marionette/element.js#1263-1273

As figured out yesterday the used behavior instant is not actually valid, and as such it falls back to auto, which isn't really instant.

Anyway, as just noticed now I also get an assertion when manually calling scrollIntoView() inside the developer tools on that specific page, and element.isInView() also returns true too early.

Lets use this bug as meta bug, and I will file blocking bugs for whatever issue I find including the assertion above.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: marionette testcase

Here a Marionette test which demonstrates that a different area than from the specified element is taken a screenshot from. As of now I can only make this work when adding an additional sleep of more than 18ms after the call to scrollIntoView().

Note that for this case the above mentioned assertion doesn't occur. So that might have been a red herring.

Botond, you mentioned something on IRC yesterday, maybe you can summarize here on the bug again? What we basically want is a behavior of instant scrolling and not any other kind of smooth.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Are you trying to scroll the target element into view across cross-origin document boundaries? It doesn't synchronously happen unfortunately. So, if you are sure that the element causes scroll for the parent document, you can check scroll event just like this.

If you are unsure whether scrolling happens or not, I don't have any clear ideas right now. We can maybe add a notification which is sent when it finished.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

Are you trying to scroll the target element into view across cross-origin document boundaries? It doesn't synchronously happen unfortunately.

No, it's all within the same page, just an element located outside the current viewport. Means in Marionette itself it is done within a framescript, but maybe that causes the behavior as you mentioned above?

So, if you are sure that the element causes scroll for the parent document, you can check scroll event just like this.

That actually works great! Can I assume it is always only a single scroll event which gets fired, and indicates that the scrolling is done? It's not that clear for me when reading the spec.

If you are unsure whether scrolling happens or not, I don't have any clear ideas right now. We can maybe add a notification which is sent when it finished.

You mean it would be sent out nevertheless if a scroll actually happened or not? If that's possible fine, otherwise we could check if the element isn't in the current viewport, and only then call scrollIntoView().

Comment 4

[Botond Ballo [:botond]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #1)

Botond, you mentioned something on IRC yesterday, maybe you can summarize here on the bug again? What we basically want is a behavior of instant scrolling and not any other kind of smooth.

Since scrollIntoView() only supports the behaviours smooth and auto, and for us auto also defaults to smooth (depending on the general.smoothScroll pref), if you want instant scrolling in a test you probably need to do it using an internal API.

PresShell::ScrollContentIntoView() supports instant scrolling (if you pass it neither ScrollSmooth nor ScrollSmoothAuto in its aScrollFlags argument), and it could be exposed to JS via nsIDOMWindowUtils

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #3)

Can I assume it is always only a single scroll event which gets fired, and indicates that the scrolling is done?

I don't think that's guaranteed. I think in the case of smooth scrolling you can get multiple scroll events as the smooth scroll progresses.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

See also https://github.com/w3c/webdriver/issues/1449 for a change to the webdriver spec, which is about replacing instant with smooth.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The meta keyword is there, the bug doesn't depend on other bugs and there is no activity for 12 months.
:whimboo, maybe it's time to close this bug?

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We will have to re-check if it is still needed, but so far no need for a meta bug.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I tried to reproduce the failure with the given test case from comment 1 but I'm not able to anymore. As such I would say we just remove the invalid instant behavior and call the bug done.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1570605 - [marionette] Remove invalid "instant" behavior for scrollIntoView().

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/979d4e44b30d
[marionette] Remove invalid "instant" behavior for scrollIntoView(). r=webdriver-reviewers,jdescottes

Comment 11

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/979d4e44b30d