929175 - Cannot switch to <frameset> frame in Marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Jim Evans]

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.

Comment 1

[Jim Evans]

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.

Comment 2

[David Burns :automatedtester]

replicated

Comment 4

[David Burns :automatedtester]

pushed to try

https://tbpl.mozilla.org/?tree=Try&rev=80279605ede5
https://tbpl.mozilla.org/?tree=Try&rev=d23bf578b63f

Comment 5

[David Burns :automatedtester]

helps if I add the necessary html files

https://tbpl.mozilla.org/?tree=Try&rev=2ca2ad832c13
https://tbpl.mozilla.org/?tree=Try&rev=8160b237e8bf

Comment 6

[David Burns :automatedtester]

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

Comment 7

[David Burns :automatedtester]

Attachment: Allow Marionette to switch to frameset frames;

Comment 8

[Malini Das [:mdas] - Away, not checking bugmail]

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.

Comment 9

[Andreas Tolfsen ❲:ato❳]

(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. (-:

Comment 10

[David Burns :automatedtester]

(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.

Comment 11

[David Burns :automatedtester]

https://tbpl.mozilla.org/?tree=Try&rev=0031a8df7db2
https://tbpl.mozilla.org/?tree=Try&rev=bb7d597b006f

Comment 12

[David Burns :automatedtester]

Attachment: Allow Marionette to switch to frameset frames;

Comment 13

[Malini Das [:mdas] - Away, not checking bugmail]

(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 14

[Malini Das [:mdas] - Away, not checking bugmail]

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

Comment 15

[David Burns :automatedtester]

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.

Comment 16

[David Burns :automatedtester]

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

Comment 17

[David Burns :automatedtester]

https://tbpl.mozilla.org/?tree=Try&rev=b6bc97324313
https://tbpl.mozilla.org/?tree=Try&rev=f0164916c003

Comment 18

[David Burns :automatedtester]

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

Comment 19

[David Burns :automatedtester]

Try is green, will clean up the patch and post for review

Comment 20

[David Burns :automatedtester]

Attachment: Part 1: Allow Marionette to switch to frameset frames.

Comment 21

[David Burns :automatedtester]

Attachment: Part 2: Updating frames tests.

Comment 22

[Malini Das [:mdas] - Away, not checking bugmail]

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

Comment 23

[David Burns :automatedtester]

landed in

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8af7e51c4c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe1fe631991

Comment 24

[David Burns :automatedtester]

Attachment: Part 1: Allow Marionette to switch to frameset frames.

Comment 25

[David Burns :automatedtester]

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

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d8af7e51c4c0
https://hg.mozilla.org/mozilla-central/rev/dfe1fe631991

Comment 27

[David Burns :automatedtester]

If this is uplifted please make sure to uplift bug 1004089 too!