Closed
Bug 929175
Opened 11 years ago
Closed 10 years ago
Cannot switch to <frameset> frame in Marionette
Categories
(Remote Protocol :: Marionette, defect, P1)
Remote Protocol
Marionette
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla32
People
(Reporter: james.h.evans.jr, Assigned: automatedtester)
References
(Blocks 1 open bug)
Details
(Whiteboard: [webdriver-compat][Uplifting? See comment 27 for ride along])
Attachments
(2 files, 3 obsolete files)
13.91 KB,
patch
|
mdas
:
review+
|
Details | Diff | Splinter Review |
8.17 KB,
patch
|
automatedtester
:
review+
|
Details | Diff | Splinter Review |
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.
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 4•11 years ago
|
||
pushed to try https://tbpl.mozilla.org/?tree=Try&rev=80279605ede5 https://tbpl.mozilla.org/?tree=Try&rev=d23bf578b63f
Assignee: nobody → dburns
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(dburns)
Assignee | ||
Comment 5•11 years ago
|
||
helps if I add the necessary html files https://tbpl.mozilla.org/?tree=Try&rev=2ca2ad832c13 https://tbpl.mozilla.org/?tree=Try&rev=8160b237e8bf
Assignee | ||
Comment 6•10 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•10 years ago
|
||
Attachment #8362134 -
Flags: review?(mdas)
Comment 8•10 years ago
|
||
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-
Comment 9•10 years ago
|
||
(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•10 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 | ||
Comment 11•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=0031a8df7db2 https://tbpl.mozilla.org/?tree=Try&rev=bb7d597b006f
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8365419 -
Flags: review?(mdas)
Assignee | ||
Updated•10 years ago
|
Attachment #8362134 -
Attachment is obsolete: true
Comment 13•10 years ago
|
||
(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•10 years ago
|
||
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•10 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•10 years ago
|
||
raised bug 972902 as it is part of the conformance for the webdriver spec in this section
Assignee | ||
Updated•10 years ago
|
Whiteboard: [webdriver-compat]
Assignee | ||
Comment 17•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b6bc97324313 https://tbpl.mozilla.org/?tree=Try&rev=f0164916c003
Assignee | ||
Comment 18•10 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•10 years ago
|
||
Try is green, will clean up the patch and post for review
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8410252 -
Flags: review?(mdas)
Assignee | ||
Updated•10 years ago
|
Attachment #8365419 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8410253 -
Flags: review?(mdas)
Assignee | ||
Updated•10 years ago
|
Priority: -- → P1
Comment 22•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8410253 -
Flags: review?(mdas) → review+
Assignee | ||
Comment 23•10 years ago
|
||
landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/d8af7e51c4c0 https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe1fe631991
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Comment 25•10 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•10 years ago
|
Attachment #8410252 -
Attachment is obsolete: true
Comment 26•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d8af7e51c4c0 https://hg.mozilla.org/mozilla-central/rev/dfe1fe631991
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee | ||
Comment 27•10 years ago
|
||
If this is uplifted please make sure to uplift bug 1004089 too!
Assignee | ||
Updated•10 years ago
|
Whiteboard: [webdriver-compat] → [webdriver-compat][Uplifting? See comment 27 for ride along]
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•