Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make innerText locator work with non-element nodes #705

Merged
merged 3 commits into from
May 17, 2024
Merged

Conversation

OrKoN
Copy link
Contributor

@OrKoN OrKoN commented Apr 30, 2024

This PR changes the innerText locator to handle non-Element nodes correctly.


Preview | Diff

@OrKoN OrKoN requested a review from sadym-chromium April 30, 2024 06:12
index.bs Outdated Show resolved Hide resolved
Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
index.bs Outdated Show resolved Hide resolved
@sadym-chromium
Copy link
Contributor

sadym-chromium commented May 17, 2024

@OrKoN do we have any blockers for this PR?

@OrKoN
Copy link
Contributor Author

OrKoN commented May 17, 2024

@sadym-chromium Mozilla's review I believe

@OrKoN OrKoN requested a review from whimboo May 17, 2024 11:12
@OrKoN OrKoN merged commit 34104f2 into main May 17, 2024
5 checks passed
@OrKoN OrKoN deleted the orkon/innerText-fix branch May 17, 2024 11:28
github-actions bot added a commit that referenced this pull request May 17, 2024
SHA: 34104f2
Reason: push, by OrKoN

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@sadym-chromium
Copy link
Contributor

sadym-chromium commented May 17, 2024

There is still an open question how to deal with request with document as a start node, and maxDepth equals zero:

{
    "type": "innerText",
    "maxDepth": 0,
    "value": "BAR"
}

As long as document does not have innerText, we can:

  1. Traverse to the nearest child html element(s) and perform search there
  2. Return an empty array.
  3. Do not decrease maxDepth on non-html elements.

@OrKoN @jgraham

@OrKoN
Copy link
Contributor Author

OrKoN commented May 17, 2024

@sadym-chromium I think #2 would be something I would expect. I.e., the document does match and the max depth is 0, therefore, no recursive calls.

@sadym-chromium
Copy link
Contributor

@sadym-chromium I think #2 would be something I would expect. I.e., the document does match and the max depth is 0, therefore, no recursive calls.

Currently spec says in this case the search should go to all the depth.

@sadym-chromium
Copy link
Contributor

@jgraham
Copy link
Member

jgraham commented May 17, 2024

I feel like users are probably going to expect not decreasing depth on document elements i.e. passing in document is equivalent to document.documentElement, but we should at least be consistent between different selector types. That would also be inconsistent with the way that node serialization works, but maybe that is less important?

@whimboo
Copy link
Contributor

whimboo commented May 17, 2024

@OrKoN are you going to create / update wpt tests for this change?

@sadym-chromium
Copy link
Contributor

I feel like users are probably going to expect not decreasing depth on document elements i.e. passing in document is equivalent to document.documentElement, but we should at least be consistent between different selector types. That would also be inconsistent with the way that node serialization works, but maybe that is less important?

Here is the spec change for this option: #713

@OrKoN
Copy link
Contributor Author

OrKoN commented May 21, 2024

@whimboo @sadym-chromium will update it after the remaining issue is resolved

sadym-chromium added a commit to GoogleChromeLabs/chromium-bidi that referenced this pull request May 21, 2024
…s` (#2218)

* w3c/webdriver-bidi#705
* w3c/webdriver-bidi#713
* web-platform-tests/wpt#46345

---------

Signed-off-by: Browser Automation Bot <browser-automation-bot@google.com>
Co-authored-by: Browser Automation Bot <browser-automation-bot@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants