Closed Bug 1143908 Opened 9 years ago Closed 7 years ago

Switching to frame by element should accept element reference instead of UUID

Categories

(Remote Protocol :: Marionette, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1410652

People

(Reporter: ato, Unassigned, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, pi-marionette-server, Whiteboard: [lang=js])

switchToFrame takes a parameter "element" that it expects to be the UUID of the iframe element to switch to.  This should instead be an element reference:

    {"ELEMENT": <UUID>}
Whiteboard: [good first bug][lang=js]
What we need to do is extract the element reference's value and use that to look up known elements.

The tricky part to this patch is handling backwards compatibility.  Changes to the client bindings for Marionette will need to be done, but to handle backwards compatibility for some time we can look to see if msg.parameters.element is an object or a string first.
The relevant line that needs to change is in testing/marionette/marionette-listener.js: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js?from=marionette-listener.js#1911
(In reply to Andreas Tolfsen (:ato) from comment #2)
> The relevant line that needs to change is in
> testing/marionette/marionette-listener.js:
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-
> listener.js?from=marionette-listener.js#1911

That returns "not found".
I guess you mean http://mxr.mozilla.org/mozilla-central/source/testing/marionette/listener.js#1701 , perhaps?
I wonder if this bug is still valid.
(In reply to Martijn Wargers [:mwargers] (QA) from comment #4)
> I guess you mean
> http://mxr.mozilla.org/mozilla-central/source/testing/marionette/listener.
> js#1701 , perhaps?

Yes I do.

> I wonder if this bug is still valid.

And yes it is to be specification compatible.  Adding it as a blocker.
Blocks: webdriver
Is this still required after recent refactorings? If so, can you update with the necessary pieces
Flags: needinfo?(ato)
(In reply to David Burns :automatedtester from comment #6)
> Is this still required after recent refactorings? If so, can you update with
> the necessary pieces

Yes, this still is still actionable.  I haven't done any work in this area.

The client currently sends a dictionary {id: <number>, element: <string>}, but according to the WebDriver specification it should only send a single "id" element which value should be type checked:

  http://w3c.github.io/webdriver/webdriver-spec.html#switch-to-frame

When the client sends {id: <number|null|web element>}, the "id" field's value should be type checked:

  let id = cmd.parameters.id;

  // switch frame by index
  if (typeof id == "number") {
    …
  }

  // switch to current top-level browsing context
  else if (id === null) {
    …
  }

  // switch to frame by element
  else if (element.isReference(id)) {
    let el = elementManager.getKnownElement(id);
    …
  }

  else {
    throw new NoSuchFrameError();
  }

Where element.isReference is a fictional new function:

  element.isReference = function(obj) {
    if (typeof obj == "undefined" || obj === null) {
      return false;
    }
    let keys = Object.keys(obj);
    return element.Key in keys || element.LegacyKey in keys;
  };

The implementation shown above is straight forward.  The complicating factor is that we need to manage backwards compatible with existing Python client versions.  This would mean also checking the "element" field.

Maybe the following would be sufficient, but would have to be verified with a try run:

  let {id: id, element: el} = cmd.parameters;
  if (typeof el != "undefined") {
    id = element.makeWebElement(el);
  }
Flags: needinfo?(ato)
Keywords: good-first-bug
Whiteboard: [good first bug][lang=js] → [lang=js]
Priority: -- → P3
Andreas, this is also a dupe of bug 1410652 right?
Flags: needinfo?(ato)
(In reply to Henrik Skupin (:whimboo) from comment #8)
> Andreas, this is also a dupe of bug 1410652 right?

Yes, looks like it!
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(ato)
Resolution: --- → DUPLICATE
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.