Closed Bug 1570605 Opened 5 years ago Closed 2 years ago

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

Categories

(Remote Protocol :: Marionette, defect, P3)

Version 3
defect

Tracking

(firefox97 fixed)

RESOLVED FIXED
97 Branch
Tracking Status
firefox97 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

()

Details

Attachments

(2 files)

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.

Depends on: 1524064
Attached file 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.

Flags: needinfo?(botond)
No longer depends on: 1524064

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.

(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().

Flags: needinfo?(botond) → needinfo?(hikezoe.birchill)

(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.

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

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?

Flags: needinfo?(hskupin)

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

Flags: needinfo?(hskupin)
Keywords: meta
Summary: [meta] "element.scrollIntoView()" sometimes returns too early → "element.scrollIntoView()" sometimes returns too early

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.

Flags: needinfo?(hikezoe.birchill)
Summary: "element.scrollIntoView()" sometimes returns too early → "element.scrollIntoView()" uses invalid scrolling behavior of "instant"
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/979d4e44b30d
[marionette] Remove invalid "instant" behavior for scrollIntoView(). r=webdriver-reviewers,jdescottes
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: