In Marionette it would be useful to represent web elements in a WebElement abstraction so that specific functionality could be associated with it. What prompted me to think about this is https://bugzilla.mozilla.org/show_bug.cgi?id=1400225 which is going to introduce a WebElementEventTarget type that would normally be an extension of a WebElement type to match Element and EventTarget in the web platform API. Having a WebElement abstraction would allow adding event listeners directly on web element references in chrome: > let webElement = await this.listener.findElement("window", …); > await new Promise(resolve => { > webElement.addEventListener("visibilitychange", resolve, {once: true}); > // calls WebElementEventTarget.prototype.addEventListener > });

Comment on attachment 8915663 [details] Bug 1400256 - Add missing break statement to find_. https://reviewboard.mozilla.org/r/186842/#review192140 I don't really see that as necessary because default is always the last case, and also our linter never complains about.

Comment on attachment 8915664 [details] Bug 1400256 - Fix element.findByLinkText docs. https://reviewboard.mozilla.org/r/186844/#review192142

Comment on attachment 8915665 [details] Bug 1400256 - Return single anon element from WebDriver:FindElement. https://reviewboard.mozilla.org/r/186846/#review192144 ::: commit-message-8187c:5 (Diff revision 1) > +Bug 1400256 - Return single anon element from WebDriver:FindElements. r?whimboo > + > +The WebDriver:FindElements command returned an array of elements when > +looking up anonymous elements. This patch rectifies the behaviour > +so that only a single element is returned. I assume you mean `WebDriver::FindElement` here. FindElements should still return an array. ::: commit-message-8187c:12 (Diff revision 1) > +It introduces a new helper function called element.findAnonymousNodes, > +akin to similar helper functions for other strategies. This function > +returns an iterator of anonymous nodes so that WebDriver:FindElements > +(plural) and WebDriver:FindElement (singular) can share the same > +code path. > + So it means searching for anon nodes would behave differently then all the other strategies? ::: testing/marionette/element.js:392 (Diff revision 1) > /** > + * Find anonymous nodes of <var>node</var>. > + * > + * @param {XULElement} rootNode > + * Root node of the document. > + * @param {XULELement} node nit: XULElement

Comment on attachment 8915665 [details] Bug 1400256 - Return single anon element from WebDriver:FindElement. https://reviewboard.mozilla.org/r/186846/#review192148

Comment on attachment 8915666 [details] Bug 1400256 - Bind this.resetValues after performing legacy action chain. https://reviewboard.mozilla.org/r/186848/#review192150

Comment on attachment 8915667 [details] Bug 1400256 - Document element.isCollection. https://reviewboard.mozilla.org/r/186850/#review192152

Comment on attachment 8915668 [details] Bug 1400256 - Make element.isXULElement more resiliant. https://reviewboard.mozilla.org/r/186852/#review192154 r=me+wc ::: testing/marionette/element.js:972 (Diff revision 1) > }; > > +/** > + * Ascertains whether <var>el</var> is a XUL element. > + * > + * @param {XULElement} el It's not required to only allow XULElements. In such a case this method would be superfluous.

Comment on attachment 8915672 [details] Bug 1400256 - Add test for element.isXULElement. https://reviewboard.mozilla.org/r/186860/#review192156

Comment on attachment 8915671 [details] Bug 1400256 - Add element.isDOMWindow. https://reviewboard.mozilla.org/r/186858/#review192158 ::: testing/marionette/element.js:991 (Diff revision 1) > }; > > +/** > + * Ascertains whether <var>node</var> is a <code>WindowProxy</code>. > + * > + * @param {WindowProxy} node This can be any kind of node. Otherwise this method would also be superfluous. ::: testing/marionette/element.js:998 (Diff revision 1) > + * > + * @return {boolean} > + * True if <var>node</var> is a DOM window. > + */ > +element.isDOMWindow = function(node) { > + let t = Object.prototype.toString.call(node); Doesn't that give a linter failure now given that t is not used? Can't you remove that line at all?

Comment on attachment 8915670 [details] Bug 1400256 - Add element.isDOMElement. https://reviewboard.mozilla.org/r/186856/#review192160

Comment on attachment 8915674 [details] Bug 1400256 - Use WebElement.generateUUID to make session ID. https://reviewboard.mozilla.org/r/186864/#review192162

Comment on attachment 8915675 [details] Bug 1400256 - Serialise IPC messages with evaluate.toJSON. https://reviewboard.mozilla.org/r/186866/#review192164 ::: testing/marionette/proxy.js:151 (Diff revision 1) > } > }; > > // The currently selected tab or window has been closed. No clean-up > // is necessary to do because all loaded listeners are gone. > - this.closeHandler = event => { > + this.closeHandler = ({type, target}) => { Personally I'm not such a fan of doing all that in the parameter list. It makes it harder to read, and you cannot easily search for listeners by using 'event'. But well... ::: testing/marionette/proxy.js (Diff revision 1) > - let payload; > - if (data && typeof data.toJSON == "function") { > + let data = evaluate.toJSON(payload); > + const msg = {type, data}; > - payload = data.toJSON(); > - } else { > - payload = data; > - } That clarifies the misuse of "payload", which was confusing me in the past multiple times. Thanks.

Comment on attachment 8915676 [details] Bug 1400256 - Drop unused arguments to evaluate.toJSON/fromJSON. https://reviewboard.mozilla.org/r/186868/#review192166

Comment on attachment 8915679 [details] Bug 1400256 - Recognise web element references in evaluate.toJSON. https://reviewboard.mozilla.org/r/186874/#review192168

Comment on attachment 8915669 [details] Bug 1400256 - Add web element abstractions. https://reviewboard.mozilla.org/r/186854/#review192170 I only skimmed the patch because the try build shows a lot of failures. Please make sure to get those fixed first, before requesting another review. Thanks. ::: testing/marionette/element.js:1077 (Diff revision 1) > + * @return {boolean} > + * True if this and <var>other</var> are the same. False > + * otherwise. > + */ > + is(other) { > + return this instanceof WebElement && this.uuid === other.uuid; why do we need the instance check for this? Shouldn't this be done for other? ::: testing/marionette/element.js:1104 (Diff revision 1) > + const uuid = WebElement.generateUUID(); > + > + if (element.isDOMElement(node)) { > + return new ContentWebElement(uuid); > + } else if (element.isDOMWindow(node)) { > + if (node.opener === null) { I'm skeptical that we can use the opener to make a decision if the element is a frame or window. Opener is a reference to a window, which opened the current window. So why do frames have an opener? ::: testing/marionette/element.js:1113 (Diff revision 1) > + } else if (element.isXULElement(node)) { > + return new ChromeWebElement(uuid); > + } > + > + throw new TypeError("Expected DOM window/element " + > + pprint`or XUL element, got: ${node}`); Which details of noe does pprint output here? ::: testing/marionette/element.js:1190 (Diff revision 1) > + */ > + static fromUUID(uuid, context) { > + assert.string(uuid); > + > + switch (context) { > + case "chrome": Please use the name of the constant. Same for content below ::: testing/marionette/element.js:1308 (Diff revision 1) > + } > + let uuid = json[ContentWebFrame.Identifier]; > + return new ContentWebFrame(uuid); > + } > +} > +ContentWebFrame.Identifier = "frame-075b-4da1-b6ba-e579c2d3230a"; I assume there is no legacy identifier as for elements? Is that something we would need in webdriver_rust too? ::: testing/marionette/element.js:1332 (Diff revision 1) > + } > + let uuid = json[ChromeWebElement.Identifier]; > + return new ChromeWebElement(uuid); > + } > +} > +ChromeWebElement.Identifier = "xulelement-9fc5-4b51-a3c8-01716eedeb04"; Same question as above for webdriver_rust.

Comment on attachment 8915673 [details] Bug 1400256 - Remove element.isWebElementReference. https://reviewboard.mozilla.org/r/186862/#review192182

Comment on attachment 8915677 [details] Bug 1400256 - Make element.Store work with web elements. https://reviewboard.mozilla.org/r/186870/#review192184 I will wait reviewing this until the WebElement implementation patch can be fully reviewed.

Comment on attachment 8915678 [details] Bug 1400256 - Use WebElement for marshaling web elements in evaluate.fromJSON. https://reviewboard.mozilla.org/r/186872/#review192188

Comment on attachment 8915680 [details] Bug 1400256 - Marshal IPC messages to and from frame script. https://reviewboard.mozilla.org/r/186876/#review192192

Comment on attachment 8915681 [details] Bug 1400256 - Make IPC messages work with implicitly marshaled elements. https://reviewboard.mozilla.org/r/186878/#review192194 ::: testing/marionette/driver.js:1736 (Diff revision 1) > > let {id, element, focus} = cmd.parameters; > > + let webEl; > + if (typeof element != "undefined") { > + webEl = WebElement.fromUUID(element, Context.CHROME); Why are we forcing always chrome here? Also please move this whole block down to where it is actually used. ::: testing/marionette/driver.js:2185 (Diff revision 1) > */ > GeckoDriver.prototype.clickElement = async function(cmd) { > assert.window(this.getCurrentWindow()); > assert.noUserPrompt(this.dialog); > > - let id = cmd.parameters.id; > + let webEl = WebElement.fromUUID(cmd.parameters.id, this.context); We don't check for a valid id. Can you please add an assertion and update the documentation? ::: testing/marionette/driver.js:2245 (Diff revision 1) > GeckoDriver.prototype.getElementAttribute = async function(cmd, resp) { > assert.window(this.getCurrentWindow()); > assert.noUserPrompt(this.dialog); > > let {id, name} = cmd.parameters; > + let webEl = WebElement.fromUUID(id, this.context); We don't assert for a valid id here too. This also applies to following methods in this file. ::: testing/marionette/driver.js:2382 (Diff revision 1) > > - let id = cmd.parameters.id; > - > switch (this.context) { > case Context.CHROME: > - let el = this.curBrowser.seenEls.get(id); > + let el = this.curBrowser.seenEls.get(webEl); I think you missed to paste the code for generating the webEl above. Instead it' only in content with a hard-coded context. ::: testing/marionette/driver.js:2549 (Diff revision 1) > assert.window(this.getCurrentWindow()); > assert.noUserPrompt(this.dialog); > > let {id, text} = cmd.parameters; > + let webEl = WebElement.fromUUID(id, this.context); > assert.string(text); We should assert first. ::: testing/marionette/driver.js:3383 (Diff revision 1) > // we don't arbitrarily save previousFrameElement, since > // we allow frame switching after modals appear, which would > // override this value and we'd lose our reference > if (message.json.storePrevious) { > - this.previousFrameElement = this.currentFrameElement; > + this.previousFrameElement = > + new ChromeWebElement(this.currentFrameElement); Why a ChromeWebElement? ::: testing/marionette/driver.js:3390 (Diff revision 1) > + if (message.json.frameValue) { > + this.currentFrameElement = > + new ChromeWebElement(message.json.frameValue); > + } else { > + // null > - this.currentFrameElement = message.json.frameValue; > + this.currentFrameElement = message.json.frameValue; Just assign null for clarity. ::: testing/marionette/legacyaction.js:209 (Diff revision 1) > event.sendKeyUp(pack[1], keyModifiers, this.container.frame); > this.actions(chain, touchId, i, keyModifiers, cb); > break; > > case "click": > - el = this.seenEls.get(pack[1]); > + webEl = WebElement.fromUUID(pack[1], "content"); I assume that for legacyActions we only want to support content? ::: testing/marionette/listener.js:783 (Diff revision 1) > > async function executeInSandbox(script, args, timeout, opts) { > opts.timeout = timeout; > - > let sb = sandboxes.get(opts.sandboxName, opts.newSandbox); > - let wargs = evaluate.fromJSON( > + return await evaluate.sandbox(sb, script, args, opts); You should remove await here similar to execute. ::: testing/marionette/listener.js (Diff revision 1) > } > > let el = await element.find(curContainer, strategy, selector, opts); > - let elRef = seenEls.add(el); > + return seenEls.add(el); > - let webEl = element.makeWebElement(elRef); > - return webEl; We should return a WebElement here too. Maybe there was a problem in rebasing the patch. ::: testing/marionette/listener.js:1333 (Diff revision 1) > } catch (e) { > sendError(e, commandID); > } > } > > -function getElementAttribute(id, name) { > +function getElementAttribute(el, name) { If you find the time to add documentation for those two methods would be fine. ::: testing/marionette/listener.js (Diff revision 1) > * web element. > - * > - * @param {String} id > - * Web element reference. > - * @param {String} prop > - * The CSS property to get. You don't want to keep the parameters anywhere? ::: testing/marionette/listener.js:1475 (Diff revision 1) > } > return; > } > > let foundShadowRoot; > - let hostEl = seenEls.get(id); > + foundShadowRoot = el.shadowRoot; Combine both lines in case we need this extra variable at all. ::: testing/marionette/listener.js:1679 (Diff revision 1) > > - let highlightEls = highlights.map(ref => seenEls.get(ref)); > - > let canvas; > + let highlightEls = highlights > + .map(ref => WebElement.fromUUID(ref, "content")) Please use Context.Content in both cases.

Comment on attachment 8915665 [details] Bug 1400256 - Return single anon element from WebDriver:FindElement. https://reviewboard.mozilla.org/r/186846/#review192144 > I assume you mean `WebDriver::FindElement` here. FindElements should still return an array. Fixed. > So it means searching for anon nodes would behave differently then all the other strategies? Yes, other helper functions return arrays which is a bit pointless.

Comment on attachment 8915665 [details] Bug 1400256 - Return single anon element from WebDriver:FindElement. https://reviewboard.mozilla.org/r/186846/#review192144 > nit: XULElement Fixed.

Comment on attachment 8915668 [details] Bug 1400256 - Make element.isXULElement more resiliant. https://reviewboard.mozilla.org/r/186852/#review192154 > It's not required to only allow XULElements. In such a case this method would be superfluous. Not sure what you’re saying here.

Comment on attachment 8915669 [details] Bug 1400256 - Add web element abstractions. https://reviewboard.mozilla.org/r/186854/#review192170 > why do we need the instance check for this? Shouldn't this be done for other? Keenly spotted! This is meant to be "other". > I'm skeptical that we can use the opener to make a decision if the element is a frame or window. Opener is a reference to a window, which opened the current window. So why do frames have an opener? Right, this is meant to be window.parent !== window. > Which details of noe does pprint output here? You can check pprint in testing/marionette/error.js: https://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/testing/marionette/error.js#151-179 OTOH for DOM elements it will output a prettified HTML tag with IDs and classes, and for objects it will output the Object.prototype.toString name of node, unless it has a custom toJSON function. > Please use the name of the constant. Same for content below We can’t do that because the Context constant is defined in testing/marionette/driver.js which would cause a circular dependency between the two files. > I assume there is no legacy identifier as for elements? Is that something we would need in webdriver_rust too? Actually, when serialised, I always include the LegacyIdentifier so that this will be backwards compatible with existing clients: > class ContentWebFrame extends WebElement { > toJSON() { > return { > [ContentWebFrame.Identifier]: this.uuid, > [ContentWebElement.LegacyIdentifier]: this.uuid, > }; > } This will need a follow-up change for clients to pick up the right identifiers. > Same question as above for webdriver_rust. And same answer as above. Dropping this issue so we can track the conversation in the other.

Comment on attachment 8915671 [details] Bug 1400256 - Add element.isDOMWindow. https://reviewboard.mozilla.org/r/186858/#review192158 > This can be any kind of node. Otherwise this method would also be superfluous. Do you mean you want it annotated with "*"? > Doesn't that give a linter failure now given that t is not used? Can't you remove that line at all? It does now when this changeset is rebased on m-c which contains the lint patch for unused variables.

Comment on attachment 8915681 [details] Bug 1400256 - Make IPC messages work with implicitly marshaled elements. https://reviewboard.mozilla.org/r/186878/#review192194 > Why are we forcing always chrome here? > > Also please move this whole block down to where it is actually used. Because WebElement.fromUUID is a workaround for when the provided element ID doesn’t contain any information about what type of reference it is. If all was well we would receive a JSON Object {"xulelement-xxx": "<uuid>"} which would tell us what type to derive, but here we receive {element: <uuid>}. Because we know the web element passed to WebDriver:SwitchToFrame is always a XUL element, we can force chrome. This workaround will be rendered unnecessary if the client at some point is changed from passing UUIDs by string to actual web element reference objects. > We don't check for a valid id. Can you please add an assertion and update the documentation? Well we didn’t previously either, but I have rectified this. > We don't assert for a valid id here too. This also applies to following methods in this file. Fixed. > I think you missed to paste the code for generating the webEl above. Instead it' only in content with a hard-coded context. Fixed. > We should assert first. Fixed. > Why a ChromeWebElement? Because this.previousFrameElement in GeckoDriver#getActiveFrame is passed back to the client, so it needs to be a web element representation. > Just assign null for clarity. Fixed. > I assume that for legacyActions we only want to support content? Yes, it is only supported in content. > You should remove await here similar to execute. Fixed. > We should return a WebElement here too. Maybe there was a problem in rebasing the patch. We do return a web element. seenEls.add(el) returns a web element reference. > You don't want to keep the parameters anywhere? For the content frame script this isn’t of equal value as in driver.js, mostly because we control its input in driver.js. The listener.js API is not “formalised” like the one in driver.js, where functions only receive a "cmd" object as input that can really be anything. > Combine both lines in case we need this extra variable at all. Fixed. > Please use Context.Content in both cases. We don’t have access to the Context enum in listener.js.

OK, in my latest update almost all tests are passing, except this one: > LOG ERROR | InvalidArgumentError: Expected 'origin' to be undefined, "viewport", "pointer", or an element, got: [object Number] -1 at chrome://marionette/content/error.js:239 [log…] I fixed the mouse action WPT tests, but forgot to update the xpcshell unit test. The original problem was that action.js was expecting web element references, but the content frame script now deserialises these automatically to DOM elements, and I’m assuming this xpcshell failure is now simply wrong.

> and I’m assuming this xpcshell failure is now simply wrong. s/failure/assertion/

Comment on attachment 8915665 [details] Bug 1400256 - Return single anon element from WebDriver:FindElement. Not sure why these got re-set.

Comment on attachment 8916732 [details] Bug 1400256 - Adapt actions for implicitly unmarshaled elements. https://reviewboard.mozilla.org/r/187802/#review193072 This has xpcshell test failures on try

(In reply to David Burns :automatedtester from comment #105) > Comment on attachment 8916732 [details] > Bug 1400256 - Adapt actions for implicitly unmarshaled elements. > > https://reviewboard.mozilla.org/r/187802/#review193072 > > This has xpcshell test failures on try Yes, see https://bugzilla.mozilla.org/show_bug.cgi?id=1400256#c102.

Comment on attachment 8915668 [details] Bug 1400256 - Make element.isXULElement more resiliant. https://reviewboard.mozilla.org/r/186852/#review192154 > Not sure what you’re saying here. I inferred what meant here from a later comment, so fixed!

Comment on attachment 8915665 [details] Bug 1400256 - Return single anon element from WebDriver:FindElement. https://reviewboard.mozilla.org/r/186846/#review192144 > Yes, other helper functions return arrays which is a bit pointless. So why don't you also return an array here so that we have a single type for whatever strategy in use. Or convert others to iterators which we could land before? Is that so complicated? Having different types is far from ideal. When we have to quickly get the patch in, please file a new bug so we don't forget about to make them all iterators.

Comment on attachment 8915669 [details] Bug 1400256 - Add web element abstractions. https://reviewboard.mozilla.org/r/186854/#review192170 > You can check pprint in testing/marionette/error.js: > > https://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/testing/marionette/error.js#151-179 > > OTOH for DOM elements it will output a prettified HTML tag > with IDs and classes, and for objects it will output the > Object.prototype.toString name of node, unless it has a custom > toJSON function. Fantastic. The `pprint` template should be moved to `format.js` now. Please file a separate bug for that. > We can’t do that because the Context constant is defined in > testing/marionette/driver.js which would cause a circular dependency > between the two files. I had this problem recently too for the Firefox application ID. Maybe we should add a new file which serves various constants. > Actually, when serialised, I always include the LegacyIdentifier so > that this will be backwards compatible with existing clients: > > > class ContentWebFrame extends WebElement { > > toJSON() { > > return { > > [ContentWebFrame.Identifier]: this.uuid, > > [ContentWebElement.LegacyIdentifier]: this.uuid, > > }; > > } > > This will need a follow-up change for clients to pick up the right > identifiers. Ok, please file the required bugs and mark them as dependencies. Once done mark it as fixed. Thanks.

Comment on attachment 8915671 [details] Bug 1400256 - Add element.isDOMWindow. https://reviewboard.mozilla.org/r/186858/#review192158 > Do you mean you want it annotated with "*"? I assume your question is a no-op now and you figured it out yourself. But yes, the input can be any kind of node, and not only `WindowProxy`.

Comment on attachment 8915681 [details] Bug 1400256 - Make IPC messages work with implicitly marshaled elements. https://reviewboard.mozilla.org/r/186878/#review192194 > Because WebElement.fromUUID is a workaround for when the provided > element ID doesn’t contain any information about what type of > reference it is. If all was well we would receive a JSON Object > {"xulelement-xxx": "<uuid>"} which would tell us what type to > derive, but here we receive {element: <uuid>}. Because we know > the web element passed to WebDriver:SwitchToFrame is always a XUL > element, we can force chrome. > > This workaround will be rendered unnecessary if the client at some > point is changed from passing UUIDs by string to actual web element > reference objects. At this line of code the element can be chrome or content. The separtion will happen later in the if blocks. So if webEl is only used in the chrome block, move those lines in there, or let it pick up the current context. > Because this.previousFrameElement in GeckoDriver#getActiveFrame > is passed back to the client, so it needs to be a web element > representation. What I'm confused with is that this block of code handles the messages between the driver and listener. It means the frame we handle here lives in content. But ChromeWebElement is an element which lives in chrome scope. > We do return a web element. seenEls.add(el) returns a web element > reference. In that case all fine. Thanks. > For the content frame script this isn’t of equal value as in > driver.js, mostly because we control its input in driver.js. The > listener.js API is not “formalised” like the one in driver.js, > where functions only receive a "cmd" object as input that can really > be anything. It would still be helpful, but I'm ok with getting it removed.

Comment on attachment 8915665 [details] Bug 1400256 - Return single anon element from WebDriver:FindElement. https://reviewboard.mozilla.org/r/186846/#review192144 > So why don't you also return an array here so that we have a single type for whatever strategy in use. Or convert others to iterators which we could land before? Is that so complicated? Having different types is far from ideal. When we have to quickly get the patch in, please file a new bug so we don't forget about to make them all iterators. I don’t want to convert the other functions because I’m not touching them in this patch. Let’s remember that these are helper functions and not published APIs. The effect I want to achieve here is to not do unnecessary computation, so I think you’ll agree that this is the right approach to take.

Comment on attachment 8915669 [details] Bug 1400256 - Add web element abstractions. https://reviewboard.mozilla.org/r/186854/#review192170 > Fantastic. The `pprint` template should be moved to `format.js` now. Please file a separate bug for that. Fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=1408454. > I had this problem recently too for the Firefox application ID. Maybe we should add a new file which serves various constants. Yes, I think ideally should clean up our dependency hierarchy here. Context.{CONTENT,CHROME} should probably be specifying in testing/marionette/browser.js. Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1408508. > Ok, please file the required bugs and mark them as dependencies. Once done mark it as fixed. Thanks. Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1408509.

Comment on attachment 8915665 [details] Bug 1400256 - Return single anon element from WebDriver:FindElement. https://reviewboard.mozilla.org/r/186846/#review192144 > I don’t want to convert the other functions because I’m not > touching them in this patch. Let’s remember that these are helper > functions and not published APIs. > > The effect I want to achieve here is to not do unnecessary computation, > so I think you’ll agree that this is the right approach to take. Ok, sounds fine. Maybe at least add it to the description of the return value that it is clear? Btw. should the return value actually be `{Array.<DOMElement>}`?

Comment on attachment 8916732 [details] Bug 1400256 - Adapt actions for implicitly unmarshaled elements. https://reviewboard.mozilla.org/r/187802/#review194590