mozilla / geckodriver

WebDriver for Firefox
https://firefox-source-docs.mozilla.org/testing/geckodriver/
Mozilla Public License 2.0
7.03k stars 1.51k forks source link

"Get Element Property" returns "classList" wrapped inside a "value" field now #2096

Closed titusfortner closed 1 year ago

titusfortner commented 1 year ago

System

Testcase

Issue discovered from this test case in Selenium test suite: https://github.com/SeleniumHQ/selenium/blob/selenium-4.8.1/java/test/org/openqa/selenium/ElementDomPropertyTest.java#L89 With this result:

https://github.com/SeleniumHQ/selenium/actions/runs/4432225435/jobs/7776269002

I've reproduced the issue and have trace logs for both Prod (v110) & Dev (v112) versions of Firefox: https://gist.github.com/titusfortner/356e093b73be834b33e7d60b9d3ffbe0

Looks like the values coming back from marionette have changed nesting level and need to be pulled from a value key now.

1679063012194 Marionette DEBUG 0 <- [1,4,null,{"0":"nameA","1":"nameBnoise","2":"nameC","item":{},"contains":{},"add":{},"remove":{},"replace":{},"toggle":{},"supports":{},"keys":{},"values":{},"entries":{},"forEach":{},"toString":{},"length":3,"value":"nameA nameBnoise nameC"}]

1679062916567 Marionette DEBUG 0 <- [1,4,null,{"value":{"0":"nameA","1":"nameBnoise","2":"nameC","item":{},"contains":{},"add":{},"remove":{},"replace":{},"toggle":{},"supports":{},"keys":{},"values":{},"entries":{},"forEach":{},"toString":{},"length":3,"value":"nameA nameBnoise nameC"}}]

whimboo commented 1 year ago

So this is related to my Marionette changes on https://bugzilla.mozilla.org/show_bug.cgi?id=1819029, which made the behavior of Get Element Property more robust and consistent for whatever type of data is returned. Previously this was a bug in Firefox.

Given that no existing wdspec test triggered a notification to me regarding this change we should extend the payload tests for these commands that could return different data types. As such I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1823355 to get this done.

Given that this is a wanted change in behavior I'm closing this issue. Thanks for letting us know @titusfortner, and please update your implementation(s) to handle the response accordingly to the WebDriver classic specification.

titusfortner commented 1 year ago

This can't be right.

Surely the Property of classList is an array of class names and not this whole object?

{"0":"nameA","1":"nameBnoise","2":"nameC","item":{},"contains":{},"add":{},"remove":{},"replace":{},"toggle":{},"supports":{},"keys":{},"values":{},"entries":{},"forEach":{},"toString":{},"length":3,"value":"nameA nameBnoise nameC"}
whimboo commented 1 year ago

Your initial comment is just about the value wrapped response. And that is what I replied on. So if there is something else as well it would be great if you could file a separate issue.

edit: Please note that even with the Firefox 102ESR release I can see the object to be returned. classList is of type DOMTokenList and here we do not have special treatment in the WebDriver classic spec as I can see. But maybe there is an issue with cloning an object.

whimboo commented 1 year ago

Regarding the remaining issue, I've created https://github.com/w3c/webdriver/pull/1728 to get the DOMTokenList support added.

titusfortner commented 1 year ago

With that PR merged in the spec, will geckodriver eventually return the previous result?

whimboo commented 1 year ago

What do you mean with that? We return the proper payload now means the DOMTokenList wrapped within the value field.