Cannot switch to <frameset> frame in Marionette

RESOLVED FIXED in mozilla32

Status

defect
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: james.h.evans.jr, Assigned: automatedtester)

Tracking

(Blocks 1 bug)

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webdriver-compat][Uplifting? See comment 27 for ride along])

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

6 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130910160258

Steps to reproduce:

Consider a page containing HTML similar to the following:

<frameset cols="*, *, *">
    <frame name="first" src="page/1"/>
    <frame name="second" src="page/2?title=Fish"/>
    <frame id="fifth" src="xhtmlTest.html"/>
</frameset>

Issue a Marionette command similar to either of the following:

{"to":"0","name":"switchToFrame","sessionId":"13","parameters":{"id":"second","focus":true}}

{"to":"0","name":"switchToFrame","sessionId":"13","parameters":{"id":"fifth","focus":true}}



Actual results:

Received a result like this:

{"from":"0","error":{"message":"Unable to locate frame: second","status":8,"stacktrace":null}}



Expected results:

The frame should be found and switched to.
Reporter

Comment 1

6 years ago
Turns out it doesn't matter whether you attempt to switch to the frame via name, ID, index, or element reference, they all fail for <frameset> frames.
Summary: Cannot switch to frame by ID or name in Marionette → Cannot switch to <frameset> frame in Marionette
Assignee

Comment 2

6 years ago
replicated
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee

Updated

6 years ago
Duplicate of this bug: 929161
Assignee

Updated

6 years ago
Flags: needinfo?(dburns)
Assignee

Comment 6

6 years ago
pushed updated version to try updated version, all *appears* green locally but lets see

https://tbpl.mozilla.org/?tree=Try&rev=3ae37fd364dc
https://tbpl.mozilla.org/?tree=Try&rev=7ea3c7d36738
Flags: needinfo?(dburns)
Assignee

Comment 7

6 years ago
Attachment #8362134 - Flags: review?(mdas)
Comment on attachment 8362134 [details] [diff] [review]
Allow Marionette to switch to frameset frames;

Review of attachment 8362134 [details] [diff] [review]:
-----------------------------------------------------------------

The rest looks good

::: testing/marionette/marionette-frame-manager.js
@@ +97,5 @@
>      let frameWindow = Services.wm.getOuterWindowWithId(message.json.win); //get the original frame window
> +    //find the OOP frame
> +    let oopFrame = frameWindow.document.getElementById(message.json.searchTerm);
> +    if (oopFrame == null) {
> +      oopFrame = frameWindow.document.getElementByName(message.json.searchTerm);

same problem here with retrieving elements that are of types other than iframe/frame

::: testing/marionette/marionette-listener.js
@@ +1824,5 @@
>        case "string" :
> +        // If we got this far we are just looking for name or id of element
> +        foundFrame = curFrame.document.getElementById(msg.json.id);
> +        if (foundFrame === null) {
> +          foundFrame = curFrame.document.getElementsByName(msg.json.id)[0];

according to the webdriver spec here https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#switching-frames, if we get a string, we should be doing window.frames[id] first, then name, then lastly, look up by id.

Also, this getElementsBy[Name|Id] approach doesn't respect the algorithm outlined in rule 3.2 of this section, ie: we have to look at the names and ids of all the frames in window.frames. If we just do getElementsByName on the document, and we have a div element named "someName" and a frame element called "someName", then we might get back the div element when we should be looking at just frame elements.

As a note, we should clarify in the webdriver spec if we select iframes or frames first, that is, if we send the string "1", should we return the 1-indexed iframe or frame? I'm not sure what to do here.
Attachment #8362134 - Flags: review?(mdas) → review-
(In reply to Malini Das [:mdas] from comment #8)
> As a note, we should clarify in the webdriver spec if we select iframes or
> frames first, that is, if we send the string "1", should we return the
> 1-indexed iframe or frame? I'm not sure what to do here.

Did you file a W3C bug about this?  I think it would be useful to get formal
decisions on a few loose threads at the F2F. (-:
Assignee

Comment 10

6 years ago
(In reply to Malini Das [:mdas] from comment #8)
> 
> Also, this getElementsBy[Name|Id] approach doesn't respect the algorithm
> outlined in rule 3.2 of this section, ie: we have to look at the names and
> ids of all the frames in window.frames. If we just do getElementsByName on
> the document, and we have a div element named "someName" and a frame element
> called "someName", then we might get back the div element when we should be
> looking at just frame elements.

The algorithm here has been lifted directly and was that way for legacy (read no one remembers why) reasons. We shouldnt be looping around when we can just ask the DOM nicely for the info we need and then, as you suggest, just check its the right type of element.
Assignee

Updated

6 years ago
Attachment #8362134 - Attachment is obsolete: true
(In reply to Andreas Tolfsen (:ato) from comment #9)
> (In reply to Malini Das [:mdas] from comment #8)
> > As a note, we should clarify in the webdriver spec if we select iframes or
> > frames first, that is, if we send the string "1", should we return the
> > 1-indexed iframe or frame? I'm not sure what to do here.
> 
> Did you file a W3C bug about this?  I think it would be useful to get formal
> decisions on a few loose threads at the F2F. (-:

I just realized that the w3c spec does cover this case, if "1" is passed in, it searches window.frames first and will retrieve the element there.
Comment on attachment 8365419 [details] [diff] [review]
Allow Marionette to switch to frameset frames;

Review of attachment 8365419 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/marionette-listener.js
@@ +1804,5 @@
>        }
>        catch(e) {
>          sendError(e.message, e.code, e.stack, command_id);
>        }
> +      for (let i = 0; i < iframes.length; i++) {

Instead of looking at frames vs. iframes, can't we just replace:
frames = curFrame.document.getElementsByTagName("iframe");

from the original code, with:
frames = curFrame.frames;

And then iterate over this?

@@ +1826,5 @@
>    if (foundFrame == null) {
>      switch(typeof(msg.json.id)) {
>        case "string" :
> +        // If we got this far we are just looking for name or id of element
> +        foundFrame = curFrame.document.getElementsByName(msg.json.id)[0];

This logic assumes the frame will be the first element in this list. What if the frame is the 2nd element in getElementsByName? Also, the spec implies that we should walk through window.frames and check the 'name' and 'id' properties in rule 3.2 https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#switching-frames. I would switch to that in case getElementsByName/Id returns things in a different order than window.frames.

@@ +1838,3 @@
>          }
>          break;
>        case "number":

I don't think we actually need to differeniate between iframes or frames anymore. We can do curFrame.frames[msg.json.id] like the spec suggests in rule https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#switching-frames, and it should return iframe or frames according to what's on the page.

@@ +1866,5 @@
>      // The frame we want to switch to is a remote (out-of-process) frame;
>      // notify our parent to handle the switch.
>      curFrame = content;
> +    sendToServer('Marionette:switchToFrame', {win: parWindow,
> +                                              searchTerm: msg.json.id,

Ah, I missed this last time:

if I do marionette.switch_to_frame(1) in b2g, then it should get the 1-indexed iframe which will be some OOP frame. In this case, the msg.json.id will be 1, which refers to an index into window.frames.

In marionette-frames.js, we use msg.json.id as an index to getElementsByName or getElementsById, but neither of these will return the frame we want. We need to let marionette-frames.js look at the current window.frames for indices in this case.

If we can use curFrame.frames for both iframes and frames, then instead of having searchTerm, we can go back to just using 'frame' and 'win' parameters, where frame will be an index into curFrame.frames and marionette-frames.js won't have to bother looking at the result of getElementsByName/Id
Attachment #8365419 - Flags: review?(mdas) → review-
Assignee

Comment 15

5 years ago
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24635 was raised today so will hold off( I have held off since last review because I knew this bug was coming) until we have a resolution.
Assignee

Comment 16

5 years ago
raised bug 972902 as it is part of the conformance for the webdriver spec in this section
Assignee

Updated

5 years ago
Whiteboard: [webdriver-compat]
Assignee

Comment 18

5 years ago
they were orange but I realised my error before they came back. did 2 more try push.

Mobile - which is green
https://tbpl.mozilla.org/?tree=Try&rev=1d9b6f70c5b2

Desktop - running when comment was posted 
https://tbpl.mozilla.org/?tree=Try&rev=aa8d307f3a18
Assignee

Comment 19

5 years ago
Try is green, will clean up the patch and post for review
Assignee

Updated

5 years ago
Attachment #8365419 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Priority: -- → P1
Comment on attachment 8410252 [details] [diff] [review]
Part 1: Allow Marionette to switch to frameset frames.

Review of attachment 8410252 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/marionette-listener.js
@@ +1847,5 @@
>        }
> +
> +      if (frames.length > 0) {
> +        for (let i = 0; i < frames.length; i++) {
> +          logger.info("frame[i] is " + frames[i].frameElement);

logger line should be removed

@@ +1850,5 @@
> +        for (let i = 0; i < frames.length; i++) {
> +          logger.info("frame[i] is " + frames[i].frameElement);
> +          // use XPCNativeWrapper to compare elements; see bug 834266
> +          if (XPCNativeWrapper(frames[i].frameElement) == XPCNativeWrapper(wantedFrame)) {
> +            logger.info("we have found it")

same here
Attachment #8410252 - Flags: review?(mdas) → review+
Attachment #8410253 - Flags: review?(mdas) → review+
Assignee

Comment 25

5 years ago
Comment on attachment 8414232 [details] [diff] [review]
Part 1: Allow Marionette to switch to frameset frames.

Review of attachment 8414232 [details] [diff] [review]:
-----------------------------------------------------------------

carrying r+ over after nits
Attachment #8414232 - Flags: review+
Assignee

Updated

5 years ago
Attachment #8410252 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d8af7e51c4c0
https://hg.mozilla.org/mozilla-central/rev/dfe1fe631991
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee

Comment 27

5 years ago
If this is uplifted please make sure to uplift bug 1004089 too!
Assignee

Updated

5 years ago
Whiteboard: [webdriver-compat] → [webdriver-compat][Uplifting? See comment 27 for ride along]
You need to log in before you can comment on or make changes to this bug.