1798464 - Replace usage of "isElementEnabled" atom with basic DOM checks

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, task, P3)

Description

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

Per WebDriver classic specification the isElementEnabled doesn't need to be used but we call it on various places in interaction.sys.mjs:

https://searchfox.org/mozilla-central/source/remote/marionette/interaction.sys.mjs

We should get rid of it if nothing else blocks us from doing so.

Comment 1

[Victoria Ajala]

Hello @whimboo, I would be working on this

Comment 2

[Victoria Ajala]

Attachment: Bug 1798464 - [marionette] Removed usage of 'isElementEnabled' Selenium atom.

Comment 3

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

Hi Victoria. I wanted to ask if you maybe have an update for us and could let us know if you are still interested to work on this bug? Thanks a lot!

Comment 4

[Victoria Ajala]

Hello Whimboo, My apologies for the inactivity regarding this bug, I encountered issues with my device that resulted in not being able to address the problem promptly. I have resolved the device problem and would be updating my patch soon. Thanks!

Comment 5

[Julian Descottes [:jdescottes]]

:whimboo will be on PTO for 3 weeks, in the meantime please reach out to me if there is anything I can do to help with the bug.

Comment 6

[Victoria Ajala]

Thanks :jdescottes I'll be in touch via matrix

Comment 7

[Julian Descottes [:jdescottes]]

Hi Victoria,

Just wanted to check if you needed some help with this bug? Don't hesitate to ask if we can unblock you.

Comment 8

[Julian Descottes [:jdescottes]]

Hi Henrik,

Can you clarify the expected result from this bug? The initial description says that we should remove isElementEnabled because it doesn't need to be used.

Per WebDriver classic specification the isElementEnabled doesn't need to be used but we call it on various places in interaction.sys.mjs [...] We should get rid of it if nothing else blocks us from doing so.

But then on the review, you wrote that we have to re-implement it:

Also we would still have to check if the element is enabled or not. As such we need a new method in element.sys.mjs with the same name, that we need to call instead. And that method has to implement the steps as given in the Webdriver specification.

Did you investigate and find out that we actually need to check for isElementEnabled after all? If so, let's update the description? Re-implementing this might be a bit complex for a mentored bug?

Comment 9

[Julian Descottes [:jdescottes]]

Looked at this a bit more, atom.isElementEnabled is still used from driver.sys.mjs isElementEnabled, for the "WebDriver:IsElementEnabled" command:

• driver.sys.mjs:isElementEnabled (searchfox)
• marionetteActors:isElementEnabled (searchfox)
• interaction.sys.mjs:isElementEnabled (searchfox)
• atom.js:isElementEnabled (searchfox)

So even if we stop using atom.isElementEnabled for internal checks in chromeClick and seleniumClickElement, we still have to keep an implementation for the actual command, which means we can't just remove the atom.

Comment 10

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

Sorry when the summary of the bug is a bit misleading. It's indeed required to keep logic that checks if an element is enabled or not.

What needs to be done here is the following (which I already laid out in the review on Phabricator):

• Remove the usage of the atom.
• Implement step 4 and 5 from the remote steps of the Is Element Enabled command.
• Ensure we have enough tests to cover certain scenarios.

Again sorry that this was unclear. But I'm happy to help in whatever you need to get this bug finished. Please let me know if you are still interested in Victoria! Thanks.

Comment 11

[Victoria Ajala]

Attachment: Bug 1798464 - Removed usage of 'isElementEnabled' atom. r=whimboo

Comment 12

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

Hi Victoria. Given that I haven't gotten a reply on Matrix yet I wanted to ask if you are still interested to work on this bug. Please let me know. If it turns out to be too complicated we can find something else for you. Thanks!

Comment 13

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

Removing needinfo for Victoria given that she updated the patch.

Comment 14

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

Hi Victoria, are you still interested to work on this bug? In case you don't have the time please let me know. Thanks!

Comment 15

[Victoria Ajala]

Hello, I sincerely apologize for the delay, I have resolved the issue from the comment on Phabricator and updated the patch.

Comment 17

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

Victoria, I've now attached patches on bug 1863266 which also extend the wdspec tests for Is Element Enabled. Hopefully nothing in your patch would need to change, but I'll respond back once the patches are on mozilla-central and I can test it together with your changes.

Overall we should be able to land your patch soon!

Comment 18

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

Sorry that it took longer than expected to fix bug 1863266, which was actually blocking the landing of your patch. But now it got landed and all blockers should be gone. I've also added more tests to the Is Element Enabled WebDriver tests to make sure that your patch could land mostly as is. Nevertheless some changes happened int he past months so that an update of your patch is required. A rebase to the most recent revision in our repository shows two conflicts. Would you have the time to update your patch? If not please let me know and I can do it for you. Thanks and sorry again for the long delay.

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 20

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

The patch that we are going to land is still from Victoria. I only did a rebase.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7ca2f47b940
[marionette] Removed usage of 'isElementEnabled' Selenium atom. r=webdriver-reviewers,Sasha

Comment 22

[Sandor Molnar[:smolnar]]

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

Comment 23

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

Victoria, thanks again for all the work you put into this patch, and sorry again for the delay in landing.