1906095 - ElementSendKeys calls scrollIntoView when an element is already visible

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P3)

Description

[Andrew Nicols]

Steps to reproduce:

With an element in view, call Element Send Keys.

Actual results:

The element was scrolled into view before the content was typed.

Expected results:

The element is already in view so should not be scrolled into view (again) before the content is typed.

https://www.w3.org/TR/webdriver2/#element-send-keys states:

NOTE

The Element Send Keys command scrolls into view the form control element and then sends the provided keys to the element. In case the element is not keyboard-interactable, an element not interactable error is returned.

The scroll into view definition states:

To scroll into view an element perform the following steps only if the element is not already in view:

(Emphasis mine)

The issue is that the Marionette interaction.sys.mjs::webdriverSendKeysToElement always calls lazy.dom.scrollIntoView(containerEl); and that always calls scrollIntoView().

That is to say that Marionette always scrolls into view, regardless of whether it is necessary or not.

Comment 1

[Andrew Nicols]

Adding some missing links to the original description:

https://www.w3.org/TR/webdriver2/#element-send-keys states:

NOTE
The Element Send Keys command scrolls into view the form control element and then sends the provided keys to the element. In case the element is not keyboard-interactable, an element not interactable error is returned.

The scroll into view definition states:

To scroll into view an element perform the following steps only if the element is not already in view:

(emphasis mine)

The issue is that the Marionette interaction.sys.mjs::webdriverSendKeysToElement always calls lazy.dom.scrollIntoView(containerEl); and that always calls scrollIntoView().

That is to say that Marionette always scrolls into view, regardless of whether it is necessary or not.

Comment 2

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

Andrew, are there actually noticeable behavioral differences? I assume the issue here is that if the element is visible at the top we scroll up so that it is then located at the end of the viewport?

Comment 3

[Andrew Nicols]

Attachment: modal-test.html

Hi Henry,

Essentially what we're seeing is that if we have:

• the page is scrolled down by at least a page
• a modal is centered on the screen

When we try to type into the field the scrollIntoView moves the modal such that the field is now at the bottom of the viewport.

When this happens, if we have interactable elements lower down in the modal, they are moved out of the viewport and can no longer be interacted with unless we again scroll the window. In our case we set the scrollbars hidden behaviour to prevent the user from accidentally scrolling the modal out of view.

Unfortunately we have some steps in our test suite which also check whether the item is visible first, and fail the test if not. These steps do not attempt to scroll into view first. That's an issue with our testsuite, but it's only an issue in the first place because Marionette is scrolling the item into view when it is already visible.

I'm going to try and work around this in our test suite but it would be great if Marionette didn't keep jumping the page unnecessarily.

I've attached a basic testcase which demonstrates this. This testcase scrolls the window by 800px so that the modal is in view (may need to be tweaked for screen size). If Marionette sends keys to the input, it jumps the modal such that the buttons can no longer be clicked. You can also reproduce this with the same scrollIntoView call that Marionette is using:

document.querySelector('input').scrollIntoView({block: "end", inline: "nearest"});

Comment 4

[Andrew Nicols]

Attachment: Bug 1906095: SendKeysToElement should only scrollIntoView if the element is not visible r=whimboo

Element Send Keys states that the commands scrolls into view the element and then sends the keys.
https://www.w3.org/TR/webdriver2/#element-send-keys

The definition for this scroll into view, found at https://www.w3.org/TR/webdriver2/#dfn-scrolls-into-view, states that
the element should only be scrolled into view if the element is not already scrolled into view.

The patch checks whether the element is visible before scrolling it into view.

Comment 5

[Andrew Nicols]

The patch added follows the same pattern which is already in place for webdriverClickElement and webdriverClearElement.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd4c88e80e86
SendKeysToElement should only scrollIntoView if the element is not visible r=webdriver-reviewers,jdescottes

Comment 7

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/47053 for changes under testing/web-platform/tests

Comment 8

[Cristian Tuns]

Backed out for causing wd failures in scroll_into_view.py

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | /webdriver/tests/classic/element_send_keys/scroll_into_view.py | test_element_already_in_viewport[{block: 'center'}] - assert 243 == 244

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 10

[Julian Descottes [:jdescottes]]

It seems the test intermittently gets a slightly different value after the first scrollIntoView.
One first thing we can check is try to use behavior: "instant" in the scrollIntoView options.

But I also stumbled upon Bug 1874551, where they have a test related to the issue we have here, and they seem to wait for a couple of request animation frames after the scrollIntoView. So we could also try that?
Edit: They actually wait for a scroll event right after the scrollIntoView. The rAF is only used for the second call.

Comment 11

[Julian Descottes [:jdescottes]]

I have tried a few things:

• wait for scroll event
• behavior: instant
• set layout.scroll.disable-pixel-alignment to true
• requestAnimationFrame

The first 3 didn't seem to help, but when adding the requestAnimationFrame, most of the failures seemed to go away, except for one occurrence: https://treeherder.mozilla.org/jobs?repo=try&revision=b1649911091a2093b524ab81755d7ec32ef8190a . I haven't tried to only use requestAnimationFrame yet, so I can't tell if it would help in isolation.

But since it still failed, I don't think this actually fixes the issue, it just makes it less frequent.
I am wondering if we should update the test to do a fuzzy comparison here?

Comment 12

[Andrew Nicols]

Yes, I was wondering whether a fuzzy approach may be better, or if there's a completely different approach to take which I haven't thought of.

Is there a standard way of handling fuzzy results in tests?

Comment 13

[Julian Descottes [:jdescottes]]

(let's ask :hiro before deciding on one approach or the other, otherwise I don't really know if we any helper to handle fuzzy results in web platform tests)

Hi Hiro!

Since you worked on Bug 1874551, do you have any suggestion on what might be wrong with the test we are adding here?

We perform a scroll with scrollIntoView, then measure the bounding rects of an element, then we call an API which is NOT supposed to scroll, and compare the bounding rects of the element with our previous measure. And sometimes it will be off by 1 pixel. Is it possible that this is a rounding issue?

Thanks

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Thanks Julian for letting me know the failure which looks suspicious scroll position rounding issue. Yeah, indeed it's very suspicious.

Though unfortunately I can't reproduce the failure locally on my Linux box with/without headless mode, even with this change which you posted to try server to set layout.scroll.disable-pixel-alignment to true, while the test is running the pref value is still false so that that's the reason why the test still fails?

Comment 15

[Hiroyuki Ikezoe (:hiro)]

FYI; I pushed a try run with changing layout.scroll.disable-pixel-alignment to true in StaticPrefList.yaml so that the test should run with layout.scroll.disable-pixel-alignment=true. The test looks pretty stable; https://treeherder.mozilla.org/jobs?repo=try&revision=7c923e7719f4b507ec8be979b86c045f4fc10298

Comment 16

[Julian Descottes [:jdescottes]]

Thanks Hiro! Yes I was suspecting that setting the pref via the ini file would not work here. Indeed with the pref properly forced it seems to work just need to figure out the best way to set it for this test.

Comment 17

[Julian Descottes [:jdescottes]]

Discussed with Henrik on matrix: sadly we don't have an easy way to set this preference only for this test.

Andrew, I think we should do a fuzzy comparison here.
I looked a bit in the codebase to see if we had helpers but the only thing I found was this pattern:

assert (expected - 1) <= actual <= (expected + 1)

at https://searchfox.org/mozilla-central/rev/f3e4b33a6122ce63bf81ae8c30cc5ac37458864b/testing/web-platform/tests/webdriver/tests/support/fixtures_bidi.py#282-283

Can you try to update the test to use something similar?

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Then I think waiting for two rAFs would be a workaround with commenting that the workaround should be dropped once after bug 1852884 and bug bug 1774315 have been fixed. We have a plan to do it in this H2.

Comment 19

[Andrew Nicols]

Thanks for that decision. I've added the fuzzy check and have run relevant treeherder jobs: https://treeherder.mozilla.org/jobs?repo=try&resultStatus=retry%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&revision=20baa4887b180f6dc511f8ba691a4c12da6f7e27

I've pushed an updated patchset to phab - do I need to create a new phab reqeust since the patch was rolled back?

Comment 20

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

(In reply to Andrew Nicols from comment #19)

I've pushed an updated patchset to phab - do I need to create a new phab reqeust since the patch was rolled back?

No, you won't have to. The revision is open again and as such allows updates to be pushed. Thanks for your work Andrew!

Comment 21

[Julian Descottes [:jdescottes]]

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

Then I think waiting for two rAFs would be a workaround with commenting that the workaround should be dropped once after bug 1852884 and bug bug 1774315 have been fixed. We have a plan to do it in this H2.

Thanks, I think we will drop all the workaround (including raf) and only do a fuzzy assert. We will revisit after bug 1852884 and bug bug 1774315 to see if we can start expecting a stable value.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f44e58d04deb
SendKeysToElement should only scrollIntoView if the element is not visible r=webdriver-reviewers,jdescottes

Comment 23

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/f44e58d04deb

Comment 24

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 25

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

Thanks a lot Andrew for your work on this bug! It's great to see that this misbehavior has been fixed.