Alexei thankfully run the Java Selenium tests for the nightly version of geckodriver and sent me the following log file. As it can be seen we have a lot of regressions especially for actions. It looks like there is a bit of work left for me to do. I will use this bug for tracking.

Actually checking the log in detail I can only see a single point of failure which is deserializing a PointerActionItem: > InvalidArgumentException: data did not match any variant of untagged enum PointerActionItem at line %x column %y Alexei would you mind to just run one of those failing tests with trace logging enabled? I would appreciate those details.

Thank you! Quickly glancing over the tracing output I think it is the following origin reference and specifically the `ELEMENT` key: "origin": { "ELEMENT": "cbd0b1e7-a456-4730-bf2b-b8ba7ec6aba1", "element-6066-11e4-a52e-4f735466cecf": "cbd0b1e7-a456-4730-bf2b-b8ba7ec6aba1" } Where does `ELEMENT` come from? Is that a legacy key we still have to support? I may have missed that because it's not part of the WebDriver spec: > An ECMAScript Object represents a web element if it has a web element identifier own property. > The web element identifier is the string constant "element-6066-11e4-a52e-4f735466cecf".

This key exists here for backward compatibility with early versions of geckodriver. As I read the spec it does not deny having extra keys: "The JSON serialization of an element is a JSON Object where the web element identifier key is mapped to the element’s web element reference."

Given that all bindings actually send the element as requested by the spec, we are safe to simply ignore any other key.

Yes, this applies generally to the WebDriver protocol: it was explicitly designed so that backwards compatibility can be achieved by ignoring unknown fields. In particular, this is what makes new session creation work. Of course, if a vendor introduces anything _new_ and vendor-specific, this should go in "-vendor-prefix" styled fields.

This is actually harder than I thought because `PointerOrigin` is handled as an untagged enum, and as such only accepts known variants. Here an example in the playground: https://play.rust-lang.org/?gist=ec22336dd31436f130b9b433f8394d61&version=stable&mode=debug&edition=2015 I asked for help, so maybe I can continue on Monday. https://github.com/serde-rs/serde/issues/1377

To keep backward compatibility, the legacy "ELEMENT" key for an instance of PointerOrigin has to be supported, but ignored. This workaround can be removed once legacy support gets dropped from geckodriver.

Comment on attachment 9008023 [details] [diff] [review] [webdriver] Ignore any unknown variant of enum PointerOrigin Review of attachment 9008023 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/webdriver/src/actions.rs @@ +183,4 @@ > } > } > > +// TODO: Can be removed once legacy ELEMENT key is no longer supported (Bug 1489141) Supported _where_? I assume this means when Selenium clients are no longer using it? In this case we should link to a tracking bug in Selenium. @@ +199,5 @@ > + } else if value == "viewport" { > + Ok(PointerOrigin::Viewport) > + } else { > + Err(de::Error::custom(format!( > + "unknown value `{}`, expected `pointer`, `viewport`, or `element-6066-11e4-a52e-4f735466cecf`", It’s not appropriate to use backticks/Markdown here. The expected inputs are given as strings, so these should all be in double quotes, except the first which shouldn’t have any because we could derive its type from the value variable.

(In reply to Andreas Tolfsen ❲:ato❳ from comment #10) > > +// TODO: Can be removed once legacy ELEMENT key is no longer supported (Bug 1489141) > > Supported _where_? I assume this means when Selenium clients are > no longer using it? > > In this case we should link to a tracking bug in Selenium. I will file an issue for Selenium, but I think it will go away with the 4.0 release of Selenium where they want to break backward compatibility. > > + } else if value == "viewport" { > > + Ok(PointerOrigin::Viewport) > > + } else { > > + Err(de::Error::custom(format!( > > + "unknown value `{}`, expected `pointer`, `viewport`, or `element-6066-11e4-a52e-4f735466cecf`", > > It’s not appropriate to use backticks/Markdown here. The expected > inputs are given as strings, so these should all be in double quotes, This is how Serde outputs the failures, and I want to have parity with that. See https://play.rust-lang.org/?gist=7a27ca462f2aa272fe3fd1788921488c&version=stable&mode=debug&edition=2015. > except the first which shouldn’t have any because we could derive > its type from the value variable. It's not the type we would want here but the name of the key. And this could be any kind of type including a Map. So do you have an idea how to best output it?

(In reply to Henrik Skupin (:whimboo) from comment #11) > (In reply to Andreas Tolfsen ❲:ato❳ from comment #10) > > > > + } else if value == "viewport" { > > > + Ok(PointerOrigin::Viewport) > > > + } else { > > > + Err(de::Error::custom(format!( > > > + "unknown value `{}`, expected `pointer`, `viewport`, or `element-6066-11e4-a52e-4f735466cecf`", > > > > It’s not appropriate to use backticks/Markdown here. The expected > > inputs are given as strings, so these should all be in double quotes, > > This is how Serde outputs the failures, and I want to have parity with that. > See https://play.rust-lang.org/?gist=7a27ca462f2aa272fe3fd1788921488c&version=stable&mode=debug&edition=2015. Oh OK. So it does that for the internal Rust types which I guess is correct.

So I will update the patch with the Selenium issue (https://github.com/SeleniumHQ/selenium/issues/6393), and push it. Thank you for the review.

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/285eb0ebe26f [webdriver] Ignore any unknown variant of enum PointerOrigin.

Please uplift this test-only change to beta. Thanks.