In testing/webdriver/src/common.rs, WebElement is defined like
this:

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub struct WebElement {
    #[serde(rename = "element-6066-11e4-a52e-4f735466cecf")]
    pub id: String,
}

impl WebElement {
    pub fn new(id: String) -> WebElement {
        WebElement { id }
    }
}

We should make it a unit struct, like this:

pub struct WebElement(String);

And also remove its constructor. This will also involve updating
code under testing/geckodriver.

You can test your change using ./mach test testing/geckodriver.

See https://firefox-source-docs.mozilla.org/testing/geckodriver/
for documentation about working on geckodriver.

Hi,

I wanted to work on this.
Is it possible? Because it doesn't have the good-first-bug tag.

Hi Nupur! Of course you can work on this. You will find some
instructions how to work on Gecko code in
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide
and more specific documentation for geckodriver (and the webdriver
crate) at
https://firefox-source-docs.mozilla.org/testing/geckodriver/index.html#for-developers.

Please let me know if you have any questions. I am "ato" on
irc.mozilla.org.

So I looked a bit further into this and this is not as easy as I
first thought. serde doesn’t have a way to give field attributes
to individual parts of a struct unit. The recommended approach
appears to be to use an intermediary helper.

This adds some extra code, but I think it is worth it considering
a WebElement is just a string that happens to be represented
as a JSON Object when serialised, and would make the consumer code
nicer to write.

I’ve drafted up some example code for this (playground):

extern crate serde;
extern crate serde_json;

use serde::{Deserialize, Deserializer, Serialize, Serializer};

#[derive(Clone, Debug, PartialEq)]
pub struct WebElement(String);

// private
#[derive(Serialize, Deserialize)]
struct WebElementObject {
    #[serde(rename = "element-6066-11e4-a52e-4f735466cecf")]
    id: String,
}

impl Serialize for WebElement {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
        WebElementObject { id: self.0.clone() }.serialize(serializer)
    }
}

impl<'de> Deserialize<'de> for WebElement {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: Deserializer<'de>,
    {
        Deserialize::deserialize(deserializer).map(|WebElementObject { id }| WebElement(id))
    }
}

fn main() {
    let json = r#"{"element-6066-11e4-a52e-4f735466cecf": "foo"}"#;
    dbg!(serde_json::from_str::<WebElement>(&json).unwrap());

    let el = WebElement("foo".to_string());
    dbg!(serde_json::to_string(&el).unwrap());
}

I hope this helps!

Thank you so much for doing the research! This definitely helps :)

A new test is failing after adding the above codetest_json_frame_id_webelement The following are the logs -
left: "\"elem\"",
right: "{\"element-6066-11e4-a52e-4f735466cecf\":\"elem\"}"'
Do we need to change this test because serialise results in a key-value pair which isn't required here?

You’re now getting to the interesting part (-:

The first thing to be aware of is that there is a specification
text for WebDriver and a test suite for the specification. These
will both inform our decision what to do next. The implementation
should match the behaviour described in the specification.

There is a test for the Switch To Frame command that specifically
tests using a web element as input, called
test_frame_id_webelement_frame, and you can run it with these
arguments to see the raw HTTP request:

./mach build testing/geckodriver && ./mach wpt --headless --webdriver-arg=-vv testing/web-platform/tests/webdriver/tests/switch_to_frame/switch.py

(Omit --headless if you want to see the browser do things.)

FrameId can either be a Short (an unsigned 16-bit integer)
or an Element (a WebElement). This means you can switch to a
frame either by its index or by web element reference.

By index, the JSON input will look like this:

{"id": 42}

By web element, like this:

{"id": {"element-6066-11e4-a52e-4f735466cecf": "3c1021ce-9759-4e08-9455-5efe884d7abc"}}

The first element-6066-11e4-a52e-4f735466cecf part is the web
element identifier we use to tell that the input references a web
element. Its value is the UUID Marionette has assigned to the DOM
node. The above two JSON snippets is the input we’re expected to
be able to handle.

Now that you’ve provided a way for the program to automatically
serialise and deserialise web elements, we need to update the cases
where this serialisation is being done manually. This first of all
means you need to fix the test as you suggested, but that again
means you need to fix the code where FrameId is being converted
to Marionette input:
https://searchfox.org/mozilla-central/source/testing/geckodriver/src/marionette.rs#1461-1474

Here, in the ToMarionette implementation for SwitchToFrame, we
call the serialisation of WebElement, but Marionette only expected
the value of the object.

You can use switch.py as a reference as you make these changes,
then update the unit tests accordingly.

So according to my understanding from the switch.py tests, we are trying to convert {"id": {"element-6066-11e4-a52e-4f735466cecf": "7274549a-2be4-b647-8af2-2ff0384c98e1"}}to {"element":"7274549a-2be4-b647-8af2-2ff0384c98e1"} in the ToMarionette method for SwitchToFrame.
And we are manually calling the serialise method with serde_json::to_value() method here. If we need to avoid calling the serialise method, do we need to extract the string value directly? Do we need to exploit the fact that "element-6066-11e4-a52e-4f735466cecf” will be the key?

Or do we need to change the Serialise method for Webelement inside Element I had a thought that we could remove this method. Removing this did not affect the test results.

Precisely. In ToMarionette we need to extract the string value
and manually insert it into the HashMap, as we don’t have automatic
serde serialisation for Marionette.

I had another doubt with regard to changing the test mentioned in comment 5
I was wondering should I change the json specified in code from "elem" to {"elem-6066-11e4-a52e-4f735466cecf":"elem"} or change the functioning of the next line so that it outputs "elem". I have changed the marionette method as per discussion but it is not affecting the tests here '=D

Yes, in fact I believe test_json_frame_id_webelement is “wrong”
in that we serialise its inner value, instead of wrapping it in a
web element reference object.

We just haven’t noticed before now because SwitchToFrameParameters
is only deserialised from input data, and never serialised for
output.

Oh okay, I am assuming this means, that I need to wrap the input(json) with webelement reference thus becoming {"element-6066-11e4-a52e-4f735466cecf":"elem"}

Yes. The expected result of the test needs to be wrapped that way
to match the (corrected) output.

(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #3)

// private
#[derive(Serialize, Deserialize)]
struct WebElementObject {
#[serde(rename = "element-6066-11e4-a52e-4f735466cecf")]
id: String,
}

Instead of defining it here and polluting the global namespace with a private struct definition, I would suggest that we move it inside the serialization and deserialization helpers. Similar to what I did with others.

Thanks for your hard work here, Nupur!

If you have the time and inclination, I would suggest following up
with looking at https://bugzilla.mozilla.org/show_bug.cgi?id=1529289
that you originally wanted to fix. I don’t think it will be equally
as labour intensive as this bug (-:

Getting it merged made my day :)
Thanks for helping out!

Will get started with the next one as well..

Getting it merged made my day :)
Thanks for helping out!

Will get started with the next one as well..

https://hg.mozilla.org/mozilla-central/rev/4655f4514fa4