Closed Bug 1400256 Opened 7 years ago Closed 7 years ago

WebElement abstraction for representing web element references

Categories

(Remote Protocol :: Marionette, enhancement)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(11 files, 12 obsolete files)

Bug 1400256 - Add web element abstractions.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1400256 - Remove element.isWebElementReference.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1400256 - Use WebElement.generateUUID to make session ID.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1400256 - Serialise IPC messages with evaluate.toJSON.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1400256 - Drop unused arguments to evaluate.toJSON/fromJSON.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1400256 - Make element.Store work with web elements.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1400256 - Use WebElement for marshaling web elements in evaluate.fromJSON.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1400256 - Recognise web element references in evaluate.toJSON.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1400256 - Marshal IPC messages to and from frame script.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1400256 - Use web element references in action tests.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1400256 - Adapt actions for implicitly unmarshaled elements.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
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 > });
Assignee: nobody → ato
Status: NEW → ASSIGNED
Blocks: marionette-window-tracking
Depends on: 1400225
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.
Attachment #8915663 - Flags: review?(hskupin) → review+
Comment on attachment 8915664 [details] Bug 1400256 - Fix element.findByLinkText docs. https://reviewboard.mozilla.org/r/186844/#review192142
Attachment #8915664 - Flags: review?(hskupin) → review+
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
Attachment #8915665 - Flags: review?(hskupin) → review-
Comment on attachment 8915665 [details] Bug 1400256 - Return single anon element from WebDriver:FindElement. https://reviewboard.mozilla.org/r/186846/#review192148
Attachment #8915665 - Flags: review-
Comment on attachment 8915666 [details] Bug 1400256 - Bind this.resetValues after performing legacy action chain. https://reviewboard.mozilla.org/r/186848/#review192150
Attachment #8915666 - Flags: review?(hskupin) → review+
Comment on attachment 8915667 [details] Bug 1400256 - Document element.isCollection. https://reviewboard.mozilla.org/r/186850/#review192152
Attachment #8915667 - Flags: review?(hskupin) → review+
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.
Attachment #8915668 - Flags: review?(hskupin) → review+
Comment on attachment 8915672 [details] Bug 1400256 - Add test for element.isXULElement. https://reviewboard.mozilla.org/r/186860/#review192156
Attachment #8915672 - Flags: review?(hskupin) → review+
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?
Attachment #8915671 - Flags: review?(hskupin) → review-
Comment on attachment 8915670 [details] Bug 1400256 - Add element.isDOMElement. https://reviewboard.mozilla.org/r/186856/#review192160
Attachment #8915670 - Flags: review?(hskupin) → review+
Comment on attachment 8915674 [details] Bug 1400256 - Use WebElement.generateUUID to make session ID. https://reviewboard.mozilla.org/r/186864/#review192162
Attachment #8915674 - Flags: review?(hskupin) → review+
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.
Attachment #8915675 - Flags: review?(hskupin) → review+
Comment on attachment 8915676 [details] Bug 1400256 - Drop unused arguments to evaluate.toJSON/fromJSON. https://reviewboard.mozilla.org/r/186868/#review192166
Attachment #8915676 - Flags: review?(hskupin) → review+
Comment on attachment 8915679 [details] Bug 1400256 - Recognise web element references in evaluate.toJSON. https://reviewboard.mozilla.org/r/186874/#review192168
Attachment #8915679 - Flags: review?(hskupin) → review+
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.
Attachment #8915669 - Flags: review?(hskupin)
Comment on attachment 8915673 [details] Bug 1400256 - Remove element.isWebElementReference. https://reviewboard.mozilla.org/r/186862/#review192182
Attachment #8915673 - Flags: review?(hskupin) → review+
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.
Attachment #8915677 - Flags: review?(hskupin)
Comment on attachment 8915678 [details] Bug 1400256 - Use WebElement for marshaling web elements in evaluate.fromJSON. https://reviewboard.mozilla.org/r/186872/#review192188
Attachment #8915678 - Flags: review?(hskupin) → review+
Comment on attachment 8915680 [details] Bug 1400256 - Marshal IPC messages to and from frame script. https://reviewboard.mozilla.org/r/186876/#review192192
Attachment #8915680 - Flags: review?(hskupin) → review+
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.
Attachment #8915681 - Flags: review?(hskupin) → review-
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.
Attachment #8916731 - Attachment is obsolete: true
Attachment #8916731 - Flags: review?(hskupin)
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/
Attachment #8915665 - Flags: review?(hskupin)
Attachment #8915671 - Flags: review?(hskupin)
Attachment #8915669 - Flags: review?(hskupin)
Attachment #8915677 - Flags: review?(hskupin)
Attachment #8915681 - Flags: review?(hskupin)
Attachment #8916732 - Flags: review?(hskupin) → review?(dburns)
Comment on attachment 8915665 [details] Bug 1400256 - Return single anon element from WebDriver:FindElement. Not sure why these got re-set.
Attachment #8915665 - Flags: review?(hskupin)
Attachment #8915669 - Flags: review?(hskupin)
Attachment #8915671 - Flags: review?(hskupin)
Attachment #8915677 - Flags: review?(hskupin)
Attachment #8915681 - Flags: review?(hskupin)
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
Attachment #8916732 - Flags: review?(dburns) → review-
(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.
Attachment #8915672 - Attachment is obsolete: true
Attachment #8915681 - Attachment is obsolete: true
Attachment #8915681 - Flags: review?(hskupin)
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
Attachment #8916732 - Flags: review?(dburns) → review+
Comment on attachment 8915665 [details] Bug 1400256 - Return single anon element from WebDriver:FindElement. https://reviewboard.mozilla.org/r/186846/#review192144 > 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>}`? So reviewing findByXPath, findByXPathAll, findByLinkText, findAnonymousNodes, and findByPartialLinkText it looks like they all have the right API documentation for return values, e.g. Array.<DOMAnchorElement>, Array.<DOMElement> and so forth. The correct return type from findAnonymousNodes would be Iterable.<XULElement> since it is a generator function that returns an iterator and not a collection of elements. I think this resolves the issue?
Attachment #8915665 - Flags: review?(hskupin)
Attachment #8915669 - Flags: review?(hskupin)
Attachment #8915671 - Flags: review?(hskupin)
Attachment #8918856 - Flags: review?(hskupin)
Attachment #8915677 - Flags: review?(hskupin)
Attachment #8916730 - Flags: review?(hskupin)
I sense the risk of a backout is high with this one. Before we move on with the rest of the review I will attempt to split some of the patches in this changeset out as separate bugs to reduce the risk. This will also make it easier to review each change atomically. Sorry about the noise this causes, but I believe it will be for the best.
Depends on: 1409026
Depends on: 1409031
Depends on: 1409036
Depends on: 1409037
Depends on: 1409040
Attachment #8915663 - Attachment is obsolete: true
Attachment #8915664 - Attachment is obsolete: true
Attachment #8915665 - Attachment is obsolete: true
Attachment #8915666 - Attachment is obsolete: true
Attachment #8915667 - Attachment is obsolete: true
Attachment #8915668 - Attachment is obsolete: true
Attachment #8915670 - Attachment is obsolete: true
Attachment #8915671 - Attachment is obsolete: true
Attachment #8918856 - Attachment is obsolete: true
Comment on attachment 8915669 [details] Bug 1400256 - Add web element abstractions. https://reviewboard.mozilla.org/r/186854/#review196002 Great update which now is way more clearer after moving all other changes out to separte patches. I like how all this looks now, but also have a couple of things you want to have a look at, or which are unclear to me. ::: testing/marionette/element.js:25 (Diff revision 8) > -this.EXPORTED_SYMBOLS = ["element"]; > +this.EXPORTED_SYMBOLS = [ > + "ChromeWebElement", > + "ContentWebElement", > + "element", > + "WebElement", > +]; We don't want to export the other new classes? ::: testing/marionette/element.js:1146 (Diff revision 8) > + * DOM element, or a XUL element. > + */ > + static from(node) { > + const uuid = WebElement.generateUUID(); > + > + if (element.isDOMElement(node) || element.isSVGElement(node)) { Nowadays we also use SVG images in chrome. So in such a case we would return a ContentWebElement instead of a ChromeWebElement. ::: testing/marionette/element.js:1235 (Diff revision 8) > + */ > + static fromUUID(uuid, context) { > + assert.string(uuid); > + > + switch (context) { > + case "chrome": You can now include and use the constants from `browser.js` right? ::: testing/marionette/element.js:1272 (Diff revision 8) > + return true; > + } > + return false; > + } > + > + static generateUUID() { Can you please add the docs? ::: testing/marionette/element.js:1316 (Diff revision 8) > + */ > +class ContentWebWindow extends WebElement { > + toJSON() { > + return { > + [ContentWebWindow.Identifier]: this.uuid, > + [ContentWebElement.LegacyIdentifier]: this.uuid, Do we have to carry the legacy identifier around even it is not used? This would also apply to the other classes except ContentWebElement. ::: testing/marionette/element.js:1360 (Diff revision 8) > + > +/** > + * XUL elements in chrome space are represented as chrome web elements > + * over the wire protocol. > + */ > +class ChromeWebElement extends WebElement { Will you add a ChromeWindowElement later? ::: testing/marionette/element.js:1370 (Diff revision 8) > + }; > + } > + > + static fromJSON(json) { > + if (!(ChromeWebElement.Identifier in json)) { > + throw new TypeError("Expected web element reference " + I wouldn't give `XUL` a `web` notation. Can't we use `Expected XUL element reference` here? ::: testing/marionette/test_element.js:169 (Diff revision 8) > + ok(webElNew.is(webElOld)); > + ok(webElOld.is(webElNew)); > + > + let refBoth = { > + [Identifier]: "foo", > + [LegacyIdentifier]: "foo", Please use a different identifier value for legacy, so we can make sure that the other has precedence. ::: testing/marionette/test_element.js:217 (Diff revision 8) > + Assert.throws(() => WebElement.fromJSON(null), /Expected JSON Object/); > + run_next_test(); > +}); > + > +add_test(function test_WebElement_fromUUID() { > + let xulWebEl = WebElement.fromUUID("foo", "chrome"); using the context constants from browser.js now?
Attachment #8915669 - Flags: review?(hskupin) → review-
Comment on attachment 8916730 [details] Bug 1400256 - Use web element references in action tests. https://reviewboard.mozilla.org/r/187798/#review196014
Attachment #8916730 - Flags: review?(hskupin) → review+
Comment on attachment 8915677 [details] Bug 1400256 - Make element.Store work with web elements. https://reviewboard.mozilla.org/r/186870/#review196018 ::: testing/marionette/element.js:140 (Diff revision 8) > add(el) { > + const isDOMElement = element.isDOMElement(el); > + const isSVGElement = element.isSVGElement(el); > + const isDOMWindow = element.isDOMWindow(el); > + const isXULElement = element.isXULElement(el); > + const context = isXULElement ? "chrome" : "content"; Please use the constants now. ::: testing/marionette/element.js:183 (Diff revision 8) > * > * @return {boolean} > * True if element is in the store, false otherwise. > */ > - has(uuid) { > - return Object.keys(this.els).includes(uuid); > + has(webEl) { > + return Object.keys(this.els).includes(webEl.uuid); This will raise an error if `webEl` is not an instance of `WebElement`. Please add a check similar to `get()`. ::: testing/marionette/element.js:230 (Diff revision 8) > - delete this.els[uuid]; > + delete this.els[webEl.uuid]; > } > > if (element.isStale(el, window)) { > throw new StaleElementReferenceError( > - pprint`The element reference of ${el || uuid} stale; ` + > + pprint`The element reference of ${el || webEl.uuid} stale; ` + This line misses an `is` before stale.
Attachment #8915677 - Flags: review?(hskupin) → review-
Comment on attachment 8915669 [details] Bug 1400256 - Add web element abstractions. https://reviewboard.mozilla.org/r/186854/#review196002 > We don't want to export the other new classes? You’re right. We will probably want to expose all of them. > Nowadays we also use SVG images in chrome. So in such a case we would return a ContentWebElement instead of a ChromeWebElement. I’m not sure there is anything we can do about this? If the element’s namespace is SVG, there’s really no way to detect that it’s an SVG element embedded in a chrome context. That implies that deducing the web element type from the namespace/prototype of the DOM element might not be the right thing to do, but I’m not sure what a right fix for that would be. > You can now include and use the constants from `browser.js` right? Hm, no we still can’t apparently because browser.js imports element.js for use in browser.Context. However wee should be able to get rid of that dependency when I do the window tracking patch because the known element store will be initialised in a slightly different way. > Can you please add the docs? Fixed. > Do we have to carry the legacy identifier around even it is not used? This would also apply to the other classes except ContentWebElement. The LegacyIdentifier is being used by the existing clients (Marionette Python client and geckodriver). We need to adapt these clients to pick up the new identifiers, but this isn’t prerequisite to landing this change. > Will you add a ChromeWindowElement later? Maybe we could, but I don’t really see any immediate need for it. The purpose of this patch is to move us closer to WebDriver conformance, which technically only requires a web window and a web frame. > I wouldn't give `XUL` a `web` notation. Can't we use `Expected XUL element reference` here? Fixed. > Please use a different identifier value for legacy, so we can make sure that the other has precedence. Good idea. Done. > using the context constants from browser.js now? Same reply as above. I will fix this once we don’t rely on element.js though.
Comment on attachment 8915677 [details] Bug 1400256 - Make element.Store work with web elements. https://reviewboard.mozilla.org/r/186870/#review196018 > Please use the constants now. Unfortunately we can’t quite yet because browser.js depends on element.js, causing a circular dependency. element.Store is being initialised in browser.Context. However, because we intend to get rid of browser.Context in the window tracking patch we should be able to move these to Context.{Chrome,Content} soon! > This will raise an error if `webEl` is not an instance of `WebElement`. Please add a check similar to `get()`. Good point. Made it so. > This line misses an `is` before stale. Fixed.
Comment on attachment 8915669 [details] Bug 1400256 - Add web element abstractions. https://reviewboard.mozilla.org/r/186854/#review196002 > I’m not sure there is anything we can do about this? If the > element’s namespace is SVG, there’s really no way to detect that > it’s an SVG element embedded in a chrome context. > > That implies that deducing the web element type from the > namespace/prototype of the DOM element might not be the right thing > to do, but I’m not sure what a right fix for that would be. Maybe we defer that for later, when there is really a use case. So far I think it's only used for CSS styling, and as such no real nodes of this type would exist? > Maybe we could, but I don’t really see any immediate need for > it. The purpose of this patch is to move us closer to WebDriver > conformance, which technically only requires a web window and a web > frame. I just wonder how chrome windows will be handled when your window refactor code lands.
Comment on attachment 8915677 [details] Bug 1400256 - Make element.Store work with web elements. https://reviewboard.mozilla.org/r/186870/#review196760
Attachment #8915677 - Flags: review?(hskupin) → review+
Comment on attachment 8915669 [details] Bug 1400256 - Add web element abstractions. https://reviewboard.mozilla.org/r/186854/#review196762
Attachment #8915669 - Flags: review?(hskupin) → review+
Comment on attachment 8915669 [details] Bug 1400256 - Add web element abstractions. https://reviewboard.mozilla.org/r/186854/#review196002 > Maybe we defer that for later, when there is really a use case. So far I think it's only used for CSS styling, and as such no real nodes of this type would exist? Yes we can do that, but you point out a real flaw. Perhaps we need to pass a Context.Chrome variant or something, but we can defer this.
Comment on attachment 8915669 [details] Bug 1400256 - Add web element abstractions. https://reviewboard.mozilla.org/r/186854/#review196002 > I just wonder how chrome windows will be handled when your window refactor code lands. These abstractions only affect references to objects as they are serialised for return from an execute script call. The window tracking patches will introduce another level of abstraction that it useful for working with ChromeWindows internally, but these will be separate.
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/135c0568ca51 Add web element abstractions. r=whimboo https://hg.mozilla.org/integration/autoland/rev/3a30959e7e63 Remove element.isWebElementReference. r=whimboo https://hg.mozilla.org/integration/autoland/rev/3ad6ce2681da Use WebElement.generateUUID to make session ID. r=whimboo https://hg.mozilla.org/integration/autoland/rev/e0212cd1523a Serialise IPC messages with evaluate.toJSON. r=whimboo https://hg.mozilla.org/integration/autoland/rev/4652aa604ba9 Drop unused arguments to evaluate.toJSON/fromJSON. r=whimboo https://hg.mozilla.org/integration/autoland/rev/73c8be3825e4 Use web element references in action tests. r=whimboo https://hg.mozilla.org/integration/autoland/rev/942796169d21 Make element.Store work with web elements. r=whimboo https://hg.mozilla.org/integration/autoland/rev/0e151127c330 Use WebElement for marshaling web elements in evaluate.fromJSON. r=whimboo https://hg.mozilla.org/integration/autoland/rev/1cb9faa02226 Recognise web element references in evaluate.toJSON. r=whimboo https://hg.mozilla.org/integration/autoland/rev/cb636836e68c Marshal IPC messages to and from frame script. r=whimboo https://hg.mozilla.org/integration/autoland/rev/60454fbd0eff Adapt actions for implicitly unmarshaled elements. r=automatedtester
I'm having difficulties with this changeset and Selenium on .NET. Specifically this code: > - } else if (!WebElement.isReference(obj)) { > - throw new InvalidArgumentError("Expected 'origin' to be a string or a " + > - pprint`web element reference, got ${obj}`); > + } else if (!element.isDOMElement(obj)) { > + throw new InvalidArgumentError("Expected 'origin' to be undefined, " + > + '"viewport", "pointer", ' + > + pprint`or an element, got: ${obj}`); > } In actions.js line 57 is making Drag and Drop Actions unusable on <path> elements for me. I've attached a link to the relevant trace level log, so maybe you can see if it's an error on your or Selenium's side. Here's the log: https://ghostbin.com/paste/6xty3 Kind regards - curtisy1
Flags: needinfo?(ato)
Curtis, can you please file a new bug under testing/Marionette and make sure to also attach a trace level log? How to do that can be found here: https://firefox-source-docs.mozilla.org/testing/geckodriver/geckodriver/TraceLogs.html But before filing the bug please test with the latest Firefox Nightly, if that is still a problem for you. Thanks.
Flags: needinfo?(ato) → needinfo?(lausklaus44)
Whoops, sorry about that. I opened a new bug now, as you proposed. Thanks!
Flags: needinfo?(lausklaus44)
Blocks: 1424635
Depends on: 1463085
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: