1863266 - "WebDriver:ElementClear" fails for elements as part of a disabled parent "fieldset"

Status: RESOLVED FIXED

(Remote Protocol :: Agent, defect, P1)

Description

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

WebDriver:ElementClear uses dom.isDisabled to check if the element to clear is not disabled. This method is broken at the moment for elements that are child nodes of a disabled fieldset.

Hereby we need to take care of the following requirements:
https://html.spec.whatwg.org/#concept-fe-disabled

Without the updated check this also blocks the replacement of the isElementEnabled atom with our own code (see bug 1798464).

Comment 1

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

Maybe there is an internal API that we could use which actually give us the disabled status for an element directly so that we do not have to check the parent nodes as well.

Comment 2

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

There is actually https://developer.mozilla.org/en-US/docs/Web/API/Element/matches that could potentially used here like:

function isDisabled(elem) {
  return elem.matches(":disabled");
}

As such we won't need recursion anymore and could directly return the disabled state. I'll check by adding various tests.

Note that the call might trigger a flush of layout/styling, which we might not want. So lets check that with a perf profile.

Comment 3

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

Attachment: Bug 1863266 - [wdspec] Improve tests for "Element Clear" and "Is Element Enabled" for disabled elements.

Comment 4

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

Attachment: Bug 1863266 - [remote] Improve DOM.isDisabled() for complex disabled checks.

Depends on D193200

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ff7b69bf4d6
[wdspec] Improve tests for "Element Clear" and "Is Element Enabled" for disabled elements. r=webdriver-reviewers,jgraham
https://hg.mozilla.org/integration/autoland/rev/8dd3f736b169
[remote] Improve DOM.isDisabled() for complex disabled checks. r=webdriver-reviewers,jgraham

Comment 6

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

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

Comment 7

[Iulian Moraru]

Backed out for causing xpcshell failures on test_DOM.js.

• Push with failures

• Failure log

• Backout link

[task 2023-11-09T19:39:33.964Z] 19:39:33     INFO -  TEST-START | remote/shared/test/xpcshell/test_DOM.js
[task 2023-11-09T19:39:34.906Z] 19:39:34  WARNING -  TEST-UNEXPECTED-FAIL | remote/shared/test/xpcshell/test_DOM.js | xpcshell return code: 0
[task 2023-11-09T19:39:34.906Z] 19:39:34     INFO -  TEST-INFO took 938ms
[task 2023-11-09T19:39:34.906Z] 19:39:34     INFO -  >>>>>>>
[task 2023-11-09T19:39:34.907Z] 19:39:34     INFO -  PID 12588 | [Parent 12588, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/checkouts/gecko/toolkit/crashreporter/nsExceptionHandler.cpp:2886
[task 2023-11-09T19:39:34.908Z] 19:39:34     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2023-11-09T19:39:34.908Z] 19:39:34     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2023-11-09T19:39:34.909Z] 19:39:34     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2023-11-09T19:39:34.909Z] 19:39:34     INFO -  running event loop
[task 2023-11-09T19:39:34.910Z] 19:39:34     INFO -  PID 12588 | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2023-11-09T19:39:34.910Z] 19:39:34     INFO -  PID 12588 | [Parent 12588, Main Thread] WARNING: Could not get the program name for a cubeb stream.: 'NS_SUCCEEDED(rv)', file /builds/worker/checkouts/gecko/dom/media/CubebUtils.cpp:402
[task 2023-11-09T19:39:34.912Z] 19:39:34     INFO -  remote/shared/test/xpcshell/test_DOM.js | Starting test_findClosest
[task 2023-11-09T19:39:34.912Z] 19:39:34     INFO -  (xpcshell/head.js) | test test_findClosest pending (2)
[task 2023-11-09T19:39:34.913Z] 19:39:34     INFO -  PID 12588 | [Parent 12588, Renderer] WARNING: Failed to create EGLContext with khr_rbab_attribs: file /builds/worker/checkouts/gecko/gfx/gl/GLContextProviderEGL.cpp:737
[task 2023-11-09T19:39:34.913Z] 19:39:34     INFO -  PID 12588 | [Parent 12588, Renderer] WARNING: Failed to create EGLContext with khr_robustness_attribs: file /builds/worker/checkouts/gecko/gfx/gl/GLContextProviderEGL.cpp:749
[task 2023-11-09T19:39:34.915Z] 19:39:34     INFO -  PID 12588 | Initializing context 0x7fc01403a2e0 surface (nil) on display 0x7fc016d17600
[task 2023-11-09T19:39:34.916Z] 19:39:34     INFO -  PID 12588 | GL_VENDOR: VMware, Inc.
[task 2023-11-09T19:39:34.917Z] 19:39:34     INFO -  PID 12588 | mVendor: VMware, Inc.
[task 2023-11-09T19:39:34.918Z] 19:39:34     INFO -  PID 12588 | GL_RENDERER: llvmpipe (LLVM 10.0.0, 256 bits)
[task 2023-11-09T19:39:34.922Z] 19:39:34     INFO -  PID 12588 | mRenderer: Unknown
[task 2023-11-09T19:39:34.922Z] 19:39:34     INFO -  PID 12588 | mIsMesa: 1
[task 2023-11-09T19:39:34.923Z] 19:39:34     INFO -  PID 12588 | [Parent 12588, Renderer] WARNING: robust_buffer_access_behavior marked as unsupported: file /builds/worker/checkouts/gecko/gfx/gl/GLContextFeatures.cpp:638
[task 2023-11-09T19:39:34.924Z] 19:39:34     INFO -  PID 12588 | [Parent 12588, Renderer] WARNING: Robustness supported, strategy is not LOSE_CONTEXT_ON_RESET!: file /builds/worker/checkouts/gecko/gfx/gl/GLContext.cpp:968
[task 2023-11-09T19:39:34.925Z] 19:39:34     INFO -  PID 12588 | [Parent 12588, Renderer] WARNING: robustness marked as unsupported: file /builds/worker/checkouts/gecko/gfx/gl/GLContextFeatures.cpp:638
[task 2023-11-09T19:39:34.926Z] 19:39:34     INFO -  PID 12588 | [Parent 12588, Main Thread] WARNING: PuppetWidget without Tab does not have transform information.: file /builds/worker/checkouts/gecko/widget/PuppetWidget.cpp:251
[task 2023-11-09T19:39:34.927Z] 19:39:34     INFO -  PID 12588 | [Parent 12588, Main Thread] WARNING: PuppetWidget without Tab does not have transform information.: file /builds/worker/checkouts/gecko/widget/PuppetWidget.cpp:251
[task 2023-11-09T19:39:34.927Z] 19:39:34     INFO -  TEST-PASS | remote/shared/test/xpcshell/test_DOM.js | test_findClosest - [test_findClosest : 109] null == null
[task 2023-11-09T19:39:34.928Z] 19:39:34     INFO -  TEST-PASS | remote/shared/test/xpcshell/test_DOM.js | test_findClosest - [test_findClosest : 110] {} == {}

Comment 8

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

Upstream PR was closed without merging

Comment 9

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

Interesting. The xpcshell test for isDisabled fails with the option child not disabled if the select is disabled. Here a snippet that replicates the situation and can be executed via the console in DevTools:

var select = document.createElement("select");
var option = document.createElement("option");
select.appendChild(option);
select.disabled = true;

document.body.appendChild(select);

option.matches(":disabled");

This prints false while I can still see a disabled select element added to the page. I'll further check.

Comment 10

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

The required WebDriver classic specification change can be found at https://github.com/w3c/webdriver/pull/1769.

Comment 11

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

We should decide how to proceed on this bug. As we noticed the WebDriver classic spec is completely broken here given that Selenium also assumes a non-disabled option element within a disabled select as disabled. That means we would have to actually do heavy checks to get the same behavior with just DOM API checks, or maybe get it discussed with Selenium to update? If we think it's not worth the time we should drop, and potentially also not remove the Is Element Enabled atom.

Comment 12

[James Graham [:jgraham]]

I think the logic should just be:

• Is the element an option element? If so, set element the nearest ancestor select if any. If not return enabled=true.
• Otherwise if element.matches(":disabled") return enabled=false, otherwise return enabled=true

Comment 13

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

https://github.com/w3c/webdriver/pull/1769 finally got merged. That means that we now correctly reference the is element disabled checks. I'll take another look at the remaining work here (based on James' last comment) probably by next week.

Comment 14

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

(In reply to James Graham [:jgraham] from comment #12)

I think the logic should just be:

• Is the element an option element? If so, set element the nearest ancestor select if any. If not return enabled=true.
• Otherwise if element.matches(":disabled") return enabled=false, otherwise return enabled=true

I had another look at the Selenium atom behavior and this does not only apply to option but also optgroup. I'll work on a PR for the classic specification to get this fixed.

Comment 15

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

I've opened https://github.com/w3c/webdriver/pull/1807 for the special-casing of option and optgroup elements in Selenium.

Comment 16

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

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

I've opened https://github.com/w3c/webdriver/pull/1807 for the special-casing of option and optgroup elements in Selenium.

The PR was just merged. I'll land the patches on this bug once the linter jobs are done.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c4bbf768bcc
[wdspec] Improve tests for "Element Clear" and "Is Element Enabled" for disabled elements. r=webdriver-reviewers,jgraham
https://hg.mozilla.org/integration/autoland/rev/b00c8c3cf832
[remote] Improve DOM.isDisabled() for complex disabled checks. r=webdriver-reviewers,jgraham

Comment 18

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/9c4bbf768bcc
https://hg.mozilla.org/mozilla-central/rev/b00c8c3cf832

Comment 19

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

Upstream PR was closed without merging

Comment 20

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

The upstream sync PR is actually: https://github.com/web-platform-tests/wpt/pull/43060

Comment 21

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

Upstream PR merged by moz-wptsync-bot