fix(page): fix page.#scrollIntoViewIfNeeded method

[abozhilov]

This patch fixes page.#scrollIntoViewIfNeededso that it works with devtools protocol.
Now it blocks the main thread and waits until the scrolling action finishes.

Issues: #8627, #1805

BREAKING CHANGE: page.#scrollIntoViewIfNeeded throws erros which come from the internal implementation.

• Protocol error (DOM.scrollIntoViewIfNeeded): Node is detached from document
• Protocol error (DOM.scrollIntoViewIfNeeded): Node does not have a layout object
  Also now it does work with TextNode properly and does not throw an error.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[OrKoN]

it looks like Firefox implementation does not support DOM.scrollIntoViewIfNeeded :-/ let's fallback to the script evaluation if DOM.scrollIntoViewIfNeeded fails?

[abozhilov]

@OrKoN Not sure it's the best approach as it'll introduce differences between chrome/firefox. DOM.scrollIntoViewIfNeeded works with text nodes, while the former implementation doesn't.

See this test case: https://github.com/puppeteer/puppeteer/blob/main/test/src/elementhandle.spec.ts#L206
Even that the name says it should work, currently it doesn't due to #scrollIntoViewIfNeeded implementation. I've fixed this test case with my PR, but obviously it doesn't pass in Firefox.

I think we should log a feature request in Firefox bug tracker for DOM.scrollIntoViewIfNeeded and my PR could wait, unless you have a better solution of the problem.

[OrKoN]

I don't think Firefox is going to look into extending the CDP support anytime soon unfortunately. cc @whimboo Henrik, is it something that could be a priority for you?

[whimboo]

I doubt that we could take that unless someone is interested to actually help with the implementation. Also DOM.scrollIntoViewIfNeeded is marked as experimental which makes it lesser a priority. Sorry.

[abozhilov]

@OrKoN I'll investigate it a bit further and I think I'll come up with a solution which would work in both envs.

[abozhilov]

I ended up with this implementation which keeps backward compatibility as much as possible and it falls back to Element.scrollIntoView if DOM.scrollIntoViewIfNeeded is not supported. Anyway with the latest release 15.3.1 a lot of tests fail and it's hard for contributing.

  async #scrollIntoViewIfNeeded(): Promise<void> {
    const error = await this.evaluate(
      async (element): Promise<string | undefined> => {
        if (!element.isConnected) {
          return 'Node is detached from document';
        }
        if (element.nodeType !== Node.ELEMENT_NODE) {
          return 'Node is not of type HTMLElement';
        }
        return;
      }
    );

    if (error) {
      throw new Error(error);
    }

    try {
      await this._client.send('DOM.scrollIntoViewIfNeeded', {
        objectId: this._remoteObject.objectId,
      });
    } catch (_err) {
      // Fallback to Element.scrollIntoView if DOM.scrollIntoViewIfNeeded is not supported
      await this.evaluate(
        async (element, pageJavascriptEnabled): Promise<void> => {
          // force-scroll if page's javascript is disabled.
          if (!pageJavascriptEnabled) {
            element.scrollIntoView({
              block: 'center',
              inline: 'center',
              // @ts-expect-error Chrome still supports behavior: instant but
              // it's not in the spec so TS shouts We don't want to make this
              // breaking change in Puppeteer yet so we'll ignore the line.
              behavior: 'instant',
            });
          }
          const visibleRatio = await new Promise(resolve => {
            const observer = new IntersectionObserver(entries => {
              resolve(entries[0]!.intersectionRatio);
              observer.disconnect();
            });
            observer.observe(element);
          });
          if (visibleRatio !== 1.0) {
            element.scrollIntoView({
              block: 'center',
              inline: 'center',
              // @ts-expect-error Chrome still supports behavior: instant but
              // it's not in the spec so TS shouts We don't want to make this
              // breaking change in Puppeteer yet so we'll ignore the line.
              behavior: 'instant',
            });
          }
        },
        this.#page.isJavaScriptEnabled()
      );
    }
  }

[OrKoN]

@abozhilov I think 15.3.1 only contains type and documentation changes. The amount of flaky failures (which is unfortunately not zero) should be the same (but we removed automatic retries and we aim at fixing those). Could you update your PR with the latest code version?

[abozhilov]

I've just updated my PR.

[OrKoN]

Do you think we still need to mark this PR as a breaking change? It looks backward compatible.

[OrKoN]

Also, it looks like a few elementhandle.spec.ts tests broke due to the changes to the test file

[abozhilov]

@OrKoN It's backward compatible, but to be 100% sure I'll have a look at the test cases which you've mentioned.

[abozhilov]

@OrKoN I've just reverted offscreenbuttons.html to it's previous version, and now the failing tests would work.

[paliwal81]

Hi @OrKoN, I would like to know by when we can expect this pull request to be merged.
Currently my e2e test cases are failing because of the same issue (as UI form is long and elements are visible on view port).