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

RESOLVED DUPLICATE of bug 1410652

Status

defect
P3
normal
RESOLVED DUPLICATE of bug 1410652
4 years ago
2 years ago

People

(Reporter: ato, Unassigned, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug, pi-marionette-server})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js])

Reporter

Description

4 years ago
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>}
Reporter

Updated

4 years ago
Whiteboard: [good first bug][lang=js]
Reporter

Comment 1

4 years ago
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.
Reporter

Comment 2

4 years ago
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.
Reporter

Comment 5

4 years ago
(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.
Reporter

Updated

4 years ago
Blocks: webdriver
Is this still required after recent refactorings? If so, can you update with the necessary pieces
Flags: needinfo?(ato)
Reporter

Comment 7

3 years ago
(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]
Mentor: ato
Priority: -- → P3
Andreas, this is also a dupe of bug 1410652 right?
Flags: needinfo?(ato)
Reporter

Comment 9

2 years ago
(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
Last Resolved: 2 years ago
Flags: needinfo?(ato)
Resolution: --- → DUPLICATE
Duplicate of bug: 1410652
You need to log in before you can comment on or make changes to this bug.