Closed Bug 1408509 Opened 7 years ago Closed 6 years ago

Recognise XUL element, WindowProxy, and <iframe> web element references in clients

Categories

(Remote Protocol :: Marionette, enhancement, P1)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(4 files)

Bug 1408509 - Support web elements, frames, and windows in geckodriver.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1408509 - Add web frame, web window, and chrome element support to Marionette client.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1408509 - Add initial support for web frame and web window to WPT WebDriver client.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1408509 - Turn some WebDriver client assertions into errors.
59 bytes, text/x-review-board-request
whimboo
: review+
Details 
After https://bugzil.la/1400256 lands there will be new web element
reference IDs for XUL elements, windows, and <iframe>s.  We need to
change all Marionette clients to recognise these.
Summary: [meta] → [meta] Recognise XUL element, WindowProxy, and <iframe> web element references in clients
[Mass Change 2018-01-15] Moving bugs to backlog
Priority: -- → P3
Priority: P3 → --
The identifiers are:
chromeelement-9fc5-4b51-a3c8-01716eedeb04
window-fcc6-11e5-b4f8-330a88ab9d7f
frame-075b-4da1-b6ba-e579c2d3230a
element-6066-11e4-a52e-4f735466cecf
Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: -- → P1
Summary: [meta] Recognise XUL element, WindowProxy, and <iframe> web element references in clients → Recognise XUL element, WindowProxy, and <iframe> web element references in clients
Comment on attachment 8986275 [details]
Bug 1408509 - Support  web elements, frames, and windows in geckodriver.

https://reviewboard.mozilla.org/r/251646/#review258368

::: testing/geckodriver/src/marionette.rs:657
(Diff revision 1)
>      }
>  
> +    /// Converts a Marionette JSON response into a `WebElement`.
> +    ///
> +    /// Note that it currently coerces all chrome elements, web frames, and web
> +    /// windows also into web elements.  This will change at a later point.

Would this need another geckodriver release first? Or why cannot we do it now? Maybe add a bug reference so it is clear what has to be done.

::: testing/geckodriver/src/marionette.rs:668
(Diff revision 1)
>  
> -        let web_element = data.get(ELEMENT_KEY);
>          let chrome_element = data.get(CHROME_ELEMENT_KEY);
> +        let element = data.get(ELEMENT_KEY);
> +        let frame = data.get(FRAME_KEY);
>          let legacy_element = data.get(LEGACY_ELEMENT_KEY);

I assume the legacy element key we have to keep support for until the minimum Firefox version we support is 58?
Attachment #8986275 - Flags: review?(hskupin) → review+
Comment on attachment 8986276 [details]
Bug 1408509 - Add web frame, web window, and chrome element support to Marionette client.

https://reviewboard.mozilla.org/r/251648/#review258370

::: testing/marionette/client/marionette_driver/marionette.py:29
(Diff revision 1)
>  from .keys import Keys
>  from .timeout import Timeouts
>  
> -WEBELEMENT_KEY = "ELEMENT"
> -W3C_WEBELEMENT_KEY = "element-6066-11e4-a52e-4f735466cecf"
> +CHROME_ELEMENT_KEY = "chromeelement-9fc5-4b51-a3c8-01716eedeb04"
> +FRAME_KEY = "frame-075b-4da1-b6ba-e579c2d3230a"
> +LEGACY_ELEMENT_KEY = "ELEMENT"

Do we still have to keep the legacy support? Marionette client on central doesn't support Firefox 57 anymore.

::: testing/marionette/client/marionette_driver/marionette.py:38
(Diff revision 1)
>  
>  class HTMLElement(object):
>      """Represents a DOM Element."""
>  
> +    identifiers = (WEB_ELEMENT_KEY, LEGACY_ELEMENT_KEY, CHROME_ELEMENT_KEY,
> +                   FRAME_KEY, WINDOW_KEY)

Mind putting each in its own line and sorted alphabetically?

::: testing/marionette/client/marionette_driver/marionette.py:186
(Diff revision 1)
>          body = {"id": self.id, "propertyName": property_name}
>          return self.marionette._send_message("WebDriver:GetElementCSSValue",
>                                               body, key="value")
>  
> +    @classmethod
> +    def _from_json(cls, json, marionette):

Why a protected/private class method for the factory method? That sounds scary.

::: testing/marionette/client/marionette_driver/marionette.py:1613
(Diff revision 1)
>          elif isinstance(args, dict):
>              wrapped = {}
>              for arg in args:
>                  wrapped[arg] = self._to_json(args[arg])
>          elif type(args) == HTMLElement:
> -            wrapped = {W3C_WEBELEMENT_KEY: args.id,
> +            wrapped = {HTMLElement.element_identifier: args.id,

There is no `element_identifier` for the `HTMLElement`. This also broke all tests.

::: testing/marionette/client/marionette_driver/marionette.py:1630
(Diff revision 1)
> +            return unwrapped
>          elif isinstance(value, dict):
>              unwrapped = {}
>              for key in value:
> -                if key == W3C_WEBELEMENT_KEY:
> -                    unwrapped = HTMLElement(self, value[key])
> +                if key in HTMLElement.identifiers:
> +                    return HTMLElement.from_json(value[key], self)

`_from_json(..)`, right?
Attachment #8986276 - Flags: review?(hskupin) → review-
Comment on attachment 8986277 [details]
Bug 1408509 - Add initial support for web frame and web window to WPT WebDriver client.

https://reviewboard.mozilla.org/r/251650/#review258372

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:291
(Diff revision 1)
>      def fullscreen(self):
>          return self.session.send_session_command("POST", "window/fullscreen")
>  
> +    @classmethod
> +    def from_json(cls, json, session):
> +        assert Window.identifier in json

Please note that in optimized code this check will not be present. Better raising an exception here?

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:304
(Diff revision 1)
> +    def __init__(self, session):
> +        self.session = session
> +
> +    @classmethod
> +    def from_json(cls, json, session):
> +        assert Frame.identifier in json

Same here.
Attachment #8986277 - Flags: review?(hskupin)
Comment on attachment 8986275 [details]
Bug 1408509 - Support  web elements, frames, and windows in geckodriver.

https://reviewboard.mozilla.org/r/251646/#review258368

> Would this need another geckodriver release first? Or why cannot we do it now? Maybe add a bug reference so it is clear what has to be done.

We can introduce these abstractions now, but that would entail a
lot more work.  So mapping them to WebElement seemed like the
quickest way to get geckodriver to recognise the new identifiers.

I think it’s best however to introduce them once the serde-ification
has landed.

> I assume the legacy element key we have to keep support for until the minimum Firefox version we support is 58?

That’s right.
Comment on attachment 8986276 [details]
Bug 1408509 - Add web frame, web window, and chrome element support to Marionette client.

https://reviewboard.mozilla.org/r/251648/#review258370

> Do we still have to keep the legacy support? Marionette client on central doesn't support Firefox 57 anymore.

The new identifiers were introduced in Firefox 58 IIRC, but it was
my understanding that in central we need to support everything back
to the last ESR?

> Why a protected/private class method for the factory method? That sounds scary.

I’m not doing anything new here, simply following the style of the
Marionette class.
Comment on attachment 8986277 [details]
Bug 1408509 - Add initial support for web frame and web window to WPT WebDriver client.

https://reviewboard.mozilla.org/r/251650/#review258372

> Please note that in optimized code this check will not be present. Better raising an exception here?

I didn’t know that!  I’ve added another commit fixing a couple of
assertions.

I’ve removed them from Frame and Window because the following line
would throw anyway.
Comment on attachment 8986275 [details]
Bug 1408509 - Support  web elements, frames, and windows in geckodriver.

https://reviewboard.mozilla.org/r/251646/#review258368

> We can introduce these abstractions now, but that would entail a
> lot more work.  So mapping them to WebElement seemed like the
> quickest way to get geckodriver to recognise the new identifiers.
> 
> I think it’s best however to introduce them once the serde-ification
> has landed.

The Serde work might still take a bit. So I wouldn't block features on it. Not sure how important such a change would be, or if it would cause us backward compatibility issues?
Comment on attachment 8986276 [details]
Bug 1408509 - Add web frame, web window, and chrome element support to Marionette client.

https://reviewboard.mozilla.org/r/251648/#review258370

> The new identifiers were introduced in Firefox 58 IIRC, but it was
> my understanding that in central we need to support everything back
> to the last ESR?

Latest ESR is 60. But wait, latest release is also 60, so we have to support 57. So just file a bug that we can remove it once 61 has been released.

> I’m not doing anything new here, simply following the style of the
> Marionette class.

Feel free to change it to `from_json()` in your patch. It shouldn't be protected/private when it has to be accessed from outside the class as factory.
Comment on attachment 8986277 [details]
Bug 1408509 - Add initial support for web frame and web window to WPT WebDriver client.

https://reviewboard.mozilla.org/r/251650/#review258372

> I didn’t know that!  I’ve added another commit fixing a couple of
> assertions.
> 
> I’ve removed them from Frame and Window because the following line
> would throw anyway.

Thanks!
Comment on attachment 8986277 [details]
Bug 1408509 - Add initial support for web frame and web window to WPT WebDriver client.

https://reviewboard.mozilla.org/r/251650/#review258892
Attachment #8986277 - Flags: review?(hskupin) → review+
Comment on attachment 8986276 [details]
Bug 1408509 - Add web frame, web window, and chrome element support to Marionette client.

https://reviewboard.mozilla.org/r/251648/#review258894
Attachment #8986276 - Flags: review?(hskupin) → review-
Comment on attachment 8986276 [details]
Bug 1408509 - Add web frame, web window, and chrome element support to Marionette client.

https://reviewboard.mozilla.org/r/251648/#review258896
Attachment #8986276 - Flags: review- → review+
Comment on attachment 8986276 [details]
Bug 1408509 - Add web frame, web window, and chrome element support to Marionette client.

https://reviewboard.mozilla.org/r/251648/#review258898

::: testing/marionette/client/marionette_driver/marionette.py:28
(Diff revision 2)
>  from .geckoinstance import GeckoInstance
>  from .keys import Keys
>  from .timeout import Timeouts
>  
> -WEBELEMENT_KEY = "ELEMENT"
> -W3C_WEBELEMENT_KEY = "element-6066-11e4-a52e-4f735466cecf"
> +CHROME_ELEMENT_KEY = "chromeelement-9fc5-4b51-a3c8-01716eedeb04"
> +FRAME_KEY = "frame-075b-4da1-b6ba-e579c2d3230a"

Well, actually changing this breaks some Marionette unit tests. Please see the latest try build.
Attachment #8986276 - Flags: review+ → review-
Comment on attachment 8987052 [details]
Bug 1408509 - Turn some WebDriver client assertions into errors.

https://reviewboard.mozilla.org/r/252304/#review258900

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:20
(Diff revision 1)
>              session = self
>  
>          if session.session_id is None:
>              session.start()
> -        assert session.session_id is not None
> +        if session.session_id is None:
> +            raise Exception("Session could not be started")

Shouldn't `session.start()` raise an exception if the session cannot be started? This looks weird.

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py
(Diff revision 1)
>          return (isinstance(other, Element) and self.id == other.id and
>                  self.session == other.session)
>  
>      @classmethod
>      def from_json(cls, json, session):
> -        assert Element.identifier in json

Why you don't want a check here anymore?
Attachment #8987052 - Flags: review?(hskupin) → review-
Comment on attachment 8986277 [details]
Bug 1408509 - Add initial support for web frame and web window to WPT WebDriver client.

https://reviewboard.mozilla.org/r/251650/#review258902

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:295
(Diff revision 2)
> +    def from_json(cls, json, session):
> +        uuid = json[Window.identifier]
> +        return cls(uuid, session)
> +
> +
> +class Frame(object):

wdspec tests are failing. Don't we correctly export this class?
Attachment #8986277 - Flags: review+ → review-
Comment on attachment 8986275 [details]
Bug 1408509 - Support  web elements, frames, and windows in geckodriver.

https://reviewboard.mozilla.org/r/251646/#review258368

> The Serde work might still take a bit. So I wouldn't block features on it. Not sure how important such a change would be, or if it would cause us backward compatibility issues?

Taking things in steps is fine.  The world doesn’t need to be perfect
in a single patch.
Comment on attachment 8986276 [details]
Bug 1408509 - Add web frame, web window, and chrome element support to Marionette client.

https://reviewboard.mozilla.org/r/251648/#review258370

> Latest ESR is 60. But wait, latest release is also 60, so we have to support 57. So just file a bug that we can remove it once 61 has been released.

Maybe http://whattrainisitnow.com/ is not up to date?  It says the
latest ESR is 52.8.1.
Comment on attachment 8986276 [details]
Bug 1408509 - Add web frame, web window, and chrome element support to Marionette client.

https://reviewboard.mozilla.org/r/251648/#review258370

> Feel free to change it to `from_json()` in your patch. It shouldn't be protected/private when it has to be accessed from outside the class as factory.

But that also exposes it to the Marionette API.  Are we OK with
this?  You’re basically arguing that the Marionette class design
is wrong, but I’m not sure this is the right patch to fix it.
Comment on attachment 8987052 [details]
Bug 1408509 - Turn some WebDriver client assertions into errors.

https://reviewboard.mozilla.org/r/252304/#review258900

> Why you don't want a check here anymore?

I said in a comment on the last patch: json[Element.identifier]
will throw if it can’t be found, so an explicit presence check is
unnecessary.
Comment on attachment 8986276 [details]
Bug 1408509 - Add web frame, web window, and chrome element support to Marionette client.

https://reviewboard.mozilla.org/r/251648/#review258898

> Well, actually changing this breaks some Marionette unit tests. Please see the latest try build.

Well I would fix that before landing it of course…
Comment on attachment 8987052 [details]
Bug 1408509 - Turn some WebDriver client assertions into errors.

https://reviewboard.mozilla.org/r/252304/#review258900

> Shouldn't `session.start()` raise an exception if the session cannot be started? This looks weird.

You’re right.  I think it’s probably fine to drop this too.
Blocks: 1470654
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1470654 to
introduce WebFrame and WebWindow abstractions in the webdriver
crate.
Comment on attachment 8986276 [details]
Bug 1408509 - Add web frame, web window, and chrome element support to Marionette client.

https://reviewboard.mozilla.org/r/251648/#review258370

> Maybe http://whattrainisitnow.com/ is not up to date?  It says the
> latest ESR is 52.8.1.

For safety they will switch to the current ESR when the previous one is no longer supported. This is just to offer companies a bit more time to migrate to the new one. For us this doesn't matter. Once a new ESR is out, the previous is no longer supported, also because updates will only be available from previous-esr to latest-esr.
Comment on attachment 8986276 [details]
Bug 1408509 - Add web frame, web window, and chrome element support to Marionette client.

https://reviewboard.mozilla.org/r/251648/#review258370

> But that also exposes it to the Marionette API.  Are we OK with
> this?  You’re basically arguing that the Marionette class design
> is wrong, but I’m not sure this is the right patch to fix it.

As I said "feel free" to do it in your patch. If not then just file as a new bug please, and drop this issue.
Comment on attachment 8987052 [details]
Bug 1408509 - Turn some WebDriver client assertions into errors.

https://reviewboard.mozilla.org/r/252304/#review258900

> I said in a comment on the last patch: json[Element.identifier]
> will throw if it can’t be found, so an explicit presence check is
> unnecessary.

Are we ok with the generic error type? Or would we have to cast it to WebDriverError? If we are fine just drop the issue.
Comment on attachment 8986276 [details]
Bug 1408509 - Add web frame, web window, and chrome element support to Marionette client.

https://reviewboard.mozilla.org/r/251648/#review258370

> For safety they will switch to the current ESR when the previous one is no longer supported. This is just to offer companies a bit more time to migrate to the new one. For us this doesn't matter. Once a new ESR is out, the previous is no longer supported, also because updates will only be available from previous-esr to latest-esr.

Thanks.

However, I don’t think we should be making that change as part of
this changeset.  I’ve filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1470915.
Comment on attachment 8987052 [details]
Bug 1408509 - Turn some WebDriver client assertions into errors.

https://reviewboard.mozilla.org/r/252304/#review258900

> Are we ok with the generic error type? Or would we have to cast it to WebDriverError? If we are fine just drop the issue.

This is similar to how geckodriver works.  If it receives data from
Marionette that isn’t valid it will return internal programming
error types.  Since this doesn’t describe WebDriver error behaviour
we shouldn’t use a WebDriver error here.
Comment on attachment 8987052 [details]
Bug 1408509 - Turn some WebDriver client assertions into errors.

https://reviewboard.mozilla.org/r/252304/#review259240
Attachment #8987052 - Flags: review?(hskupin) → review+
Blocks: 1470915
Comment on attachment 8986276 [details]
Bug 1408509 - Add web frame, web window, and chrome element support to Marionette client.

https://reviewboard.mozilla.org/r/251648/#review259394
Attachment #8986276 - Flags: review?(hskupin) → review+
Comment on attachment 8986277 [details]
Bug 1408509 - Add initial support for web frame and web window to WPT WebDriver client.

https://reviewboard.mozilla.org/r/251650/#review259396

::: testing/web-platform/meta/MANIFEST.json:523112
(Diff revision 3)
>    "css/css-scoping/shadow-host-with-before-after.html": [
>     "99af6e29e69b3131b59dbdc2b0eead52931123c2",
>     "reftest"
>    ],
>    "css/css-scoping/shadow-link-rel-stylesheet-no-style-leak.html": [
> -   "76a54dabd8bd09f7155ab0331e3d17d1a0cae243",
> +   "a46be006762a16c2deb3d1d3a760e3c4e348668c",

None of the changes in that file are related to your patch. I would propose that you just remove this file from the commit.
Attachment #8986277 - Flags: review?(hskupin) → review+
Comment on attachment 8986277 [details]
Bug 1408509 - Add initial support for web frame and web window to WPT WebDriver client.

https://reviewboard.mozilla.org/r/251650/#review259396

> None of the changes in that file are related to your patch. I would propose that you just remove this file from the commit.

I’m not going to stage lines that only affect my patch.  The process
for updating the manifests needs to be run "./mach wpt-manifest-update"
without manual staging.  That is simply to cumbersome.

Normally, if only a single change has been made to the other files,
there won’t be any merge conflicts with these sort of updates because
the SHA1 in another patch will have been changed to the same SHA1.
There might be a conflict if there have been two patches to the
test in the interim but in past experience that is quite rare unless
you have a significantly outdated patched.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/954db416bb72
Support  web elements, frames, and windows in geckodriver. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/3b352d73afc7
Add web frame, web window, and chrome element support to Marionette client. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/b9f5d6567484
Add initial support for web frame and web window to WPT WebDriver client. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/f97b37943daf
Turn some WebDriver client assertions into errors. r=whimboo
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11787 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/11787
* continuous-integration/appveyor/pr
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: