Remove, where possible, explicit frame switching references in favour of calls that switch to the displayed_app[.frame]. This is to match the natural behaviour of the user, for example when a dialog menu is closed we switch to the app that is displayed. In the case of the dialog not closed properly the test should fail. Currently the test may return a false positive in that scenario. In some cases we still do need to explicitly switch to the frame so it cannot be avoided.

Comment on attachment 8337696 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/14820 Two of the tests are not working as expected. See the PR comments for details.

Messages app changes landed on master in https://github.com/mozilla-b2g/gaia/commit/a51c02ebdf06beaacdf6680638292b2ee530c1fa I guess we should bring all of these over to v1.2 as well.

(In reply to Zac C (:zac) from comment #1) > Created attachment 8337696 [details] > First up, messages tests/app object The test_sms_contact.TestContacts will failed due to gaiatest/apps/messages/regions/new_message.py cannot auto swith from Contacts app to Message app. File Bug 943757 for it.

Comment on attachment 8338649 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/14820 One test is still failing. See the PR for details.

Comment on attachment 8338649 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/14820 r+ but tests are failing. I am concerned about merging this now, as it might just get backed out, even though this commit wouldn't be responsible for failures. Maybe we should wait to fix the failures via other PRs first before merging this?

OK don't merge it, I'll look at the test failures tomorrow. This pull is not important so it can wait a bit longer.

Comment on attachment 8338649 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/14820 Landed on master in https://github.com/mozilla-b2g/gaia/commit/def4baa9ad44843ea214b6f94d04c29613cbfadb

Pointer to Github pull-request

Comment on attachment 8344697 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/14820 uplifted to v1.2

Comment on attachment 8344697 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/14820 Landed on v1.2 in https://github.com/mozilla-b2g/gaia/commit/096722a9e2510ecdfe45ba7382d7d50826b82feb Thanks for doing this uplift. I think there may be other commits related to this bug that haven't been uplifted though. Do you mind checking that?

Changing status-b2g-v1.2 to wontfix

Just adding this for documentation purposes: The one use of explicit frame switching in the Browser app cannot be replaced as it is using a dialog from the System app. When that dialog is active, `self.apps.displayed_app.name` still reports 'Browser', so switching to the displayed app does nothing. I imagine we'll need to keep the explicit frame switching for this one, but I also wonder whether this may be a bug in `self.apps.displayed_app.name`. What do you think, Zac?

Bob, I would raise that test case against the AppWindowManager and CC the dev for AppWindowManager who is Alive Kuo. I think if you are definitely interacting with the System app there then the displayed_app should be System. There might be some other cases where we are blocked like this and in these cases we can just raise bugs against AppWindowManager.

I opened bug 951656 about the browser bookmark dialog and also bug 951660 about a similar problem with the Camera app. I will revisit trying to remove the explicit frame switching from them if and when those bugs are fixed.

Changes to the Contacts app

I submitted a PR for one change to the Contacts app. Other changes to Contacts will have to wait until bug 951815 is fixed.

Added bug 951837 for the Gallery app which reports incorrectly as the Contacts app when launched from Activities.

Added bug 951844 for the Messages app which reports incorrectly as the Contacts app when launched from Contacts.

Changes to frame switching logic for FTU tests

Changes to keyboard tests

Added bug 951891 for the Camera app which reports as the Homescreen app when unlocked from the Lockscreen with a passcode in effect.

Comment on attachment 8349728 [details] Changes to persona test r+

Comment on attachment 8349711 [details] Changes to keyboard tests r+ Merge in: https://github.com/mozilla-b2g/gaia/commit/7ecb4212419f332faad3b314c2d4f8df0b1a94e0

Comment on attachment 8349728 [details] Changes to persona test r+ Merged in: https://github.com/mozilla-b2g/gaia/commit/a641694d856e61a4906a4309f5d26f3d87c14a0c

Comment on attachment 8349680 [details] Changes to videoplayer app r+ Merged in : https://github.com/mozilla-b2g/gaia/commit/5ce7ee38389c563b155f2d48c1b2183073d7f7bd

Lol Bob, when I suggested you do it I thought it might be something to spend a few days on not just a few hours :)

Comment on attachment 8349618 [details] Changes to Contacts app r+ merged in: https://github.com/mozilla-b2g/gaia/commit/2827df4384144181fb42b7bd4700dbf1e0203725

Comment on attachment 8349696 [details] Changes to FTU tests r+ merged in: https://github.com/mozilla-b2g/gaia/commit/a09030b69a23ad26252714c8b0e55a2c875c36c9

All the merge links are in the previous comments. marking as RF

This bug has not been finished, there are still some fixes required, and some are blocked by other bugs (as can be seen on this bug), so I am reopening it. Please don't close bugs unless you are sure they are actually fixed, and if a bug has blockers that's a good sign that it's not done.

This is the last one for now, but please keep the bug open so others can be done as the blockers are fixed.

Comment on attachment 8350048 [details] A batch of changes to switch_to_displayed_app r+

Merged in : https://github.com/mozilla-b2g/gaia/commit/27f4249ba438930d3ce845d4a84de1bf83a8961d

Zac, if you get a chance while you're in Taipei, can you follow up with Alive about the blockers to this bug. There are a bunch of cases in which getDisplayedApp is not returning the app we expect.

Bob, I've been looking into this a bit more and think this is because we're not really interested in the app, we're interested in the topmost frame, regardless of whether it is an appWindow or activityCallee. From there we need some details like its name/src, etc but really we juts want the frame I'll have a think about a way to get that topmost iframe but it's complicated by gaiatest needing to support v1.3 and v1.4. (wonder if we can loop all iframes marionette can find, polling `is_displayed()` ?) That said if we can get the topmost frame then the `displayed_app` technique we're using here will be obsolete.

PS in the meantime we should do what we can within the limitations, if there's any more we are not blocked on in this bug.

I did a quick experiment by finding all iframes from System and then checking which ones are visible/etc. I did it while a activity menu is open, for example Messages app open and then Camera activity open to attach a photo to the new message. Marionette interprets both the Messages and Camera frames as displayed, probably just because they have sizes >0 (I didn't think to debug their sizes at the time). We already know Marionette cannot very well detect when one frame is sitting atop another. Other frames like homescreen, system, etc are correctly not displayed. AutomatedTester, do you have any ideas about how we can pick up the topmost iframe, regardless of how it got there or anything like that?

I can't think of one at the moment. You might be able to do > marionette.switch_to_frame() > top_iframe = marionette.execute_script("return document.elementFromPoint(x, y)") // x, y might be centre of view point. and get that to work. https://developer.mozilla.org/en-US/docs/Web/API/document.elementFromPoint?redirectlocale=en-US&redirectslug=DOM%2Fdocument.elementFromPoint

(In reply to David Burns :automatedtester from comment #42) > I can't think of one at the moment. You might be able to do > > > marionette.switch_to_frame() > > top_iframe = marionette.execute_script("return document.elementFromPoint(x, y)") // x, y might be centre of view point. > > and get that to work. > > https://developer.mozilla.org/en-US/docs/Web/API/document. > elementFromPoint?redirectlocale=en-US&redirectslug=DOM%2Fdocument. > elementFromPoint That works very well. When you run that from the context of the System app you get the visible iframe and not the app. I tested this in a few scenarios and it works well. However it does not work for nested iframes, ie the browser tabs inside the Browser app. It will only pick up the topmost frame. If you run the snippet in the context of an app then you get the particular HTMLElement, from which you could iterate up to get first iframe. But Marionette may have trouble switching to it if it's a nested one, we'd have to make two switches (this could get messy). It could be worth looking into this a bit more Bob, experimenting with a method like `switch_to_top_frame` and obsoleting switch_to_displayed_frame, the latter of which obviously has too many limitations to have widespread case. This would mean we have the scope to switch to the top frame that is managed by the AppWindowManager (app or activity) but any nested ones would need to be managed from within the test/page object itself.

Bob, et al, I have a prototype using comment #42 almost ready for review, but I'll try to get some other pulls in first.

OK guys here is a proposal for changing the frame switching logic. So far this is working quite well. It is very good at picking up the activity frame or the app frame. It will also fail if we try to switch frames when there is an alert prompt or something in the way because the elementFromPoint will not be an iframe. It needs to be improved around the way we wait for the frame. We can't wait for the name as it is not contained in the iframe attributes and even then lots of pages don't have names. Only the app itself has a name, not the page. More or less the most trustworthy thing we have is just the iframe src. I also don't like the name of GaiaFrameManager and the methods contained. The methods in GaiaFrameManager could be improved possibly, if we can reduce the amount of Marionette calls it does. I don't really like the way we switch to the frame and pass in the expected name/src from inside the test case often. Perhaps the app object Base could be extended with a generic method that will try to find that app's frame in the top frame, that way a lot of the matching and what not are hidden inside the app/region object. NB there may be bugs when running this on device still.

Comment on attachment 8371519 [details] github pr The change looks OK. I kind of like the idea of having a _frame_src_match var in each class also it works OK Ran: http://qa-selenium.mv.mozilla.com:8080/job/b2g.hamachi.mozilla-central.ui.adhoc/182/console

Copying this comment from Github as per Zac's request: As discussed in IRC, I like this approach but it seems like it would be great it we could somehow globally override some of marionette's methods (like find_element and find_elements) to always switch to the top frame first. That would allow us to not have to include any of this frame switching code in our tests or app objects. There is an issue with embedded frames, but that could probably be dealt with as well. I tried a number of "tricks" to get this to work and they all either didn't work or were really ugly, so I don't think I'm going to spend any more time on that. The only "not so ugly" thing I can think of would be to create our own versions of find_element and find_elements which do the switch first, but if we do that then we'd have to make sure we always use those, and not the built-in marionette methods, and I'm not sure that's a great thing to do. I'm going to test all the changes in this PR to make sure the tests still pass, and if they do I think we're good to merge this, although I would like to keep this as a possible future enhancement - to somehow make this required frame switching automatic. Pinging @davehunt who I know is away right now to see if he has any ideas when he gets back.

Comment on attachment 8371519 [details] github pr The code looks good, so I'm r+ on the changes, but there are a number of consistent failures which I've documented in the PR.

I addressed the failures, thanks Bob. What do you think of the matching? Some iframes do not have a src attribute which means we need a way to match on other attributes too. However I just thought of something.. If we locate the iframe using Marionette then we can compare it to the element return from elementFromPoint and if they are equal then we know the frame is the topmost one. The advantage here is that we store the locators for the frames as WebDriver By tuples which I would much prefer and alleviates some of my concerns in c#45.

another adhoc: http://qa-selenium.mv.mozilla.com:8080/job/b2g.hamachi.mozilla-central.ui.adhoc/202/console Considering that today's regular jobs had only 2 failures in total this should be close to that too.

(In reply to Bob Silverberg [:bsilverberg] from comment #47) > Copying this comment from Github as per Zac's request: > > As discussed in IRC, I like this approach but it seems like it would be > great it we could somehow globally override some of marionette's methods > (like find_element and find_elements) to always switch to the top frame > first. Having explicit frame switching shows in our code that we're expecting the displayed frame to change, so if we handle this automatically we'll lose that - however I can see that it might be a reasonable cost for simplifying our test code. Overriding the element locating methods of Marionette would add overhead and would also make it difficult to locate elements in alternate frames, if needed. I wonder if it would be possible for gaia to emit an event when the displayed frame changes, allowing us to immediately switch to that frame in gaiatest. This is a very gaia centric issue, so I don't think we need a Marionette level solution. It would also be necessary to clarify what we mean by top/displayed frame in order to accurately switch to the appropriate context. In the attached patch we're finding the element in the center of the viewport so long as it's an iframe, but we might need something more robust.

Dave and I talked about this in IRC and thought we should ask Jgriffin and Malini about whether this could/should live in Gaia and Marionette rather than in the test framework.

just to clarify, are you asking where automatic frame switching should live?

Both could or should live. We just want to explore a solution that would work higher up (in Marionette or Gaia) rather than this solution which we're probably only 90% happy with. Let's explore all possible solutions with ideas from other people and if no better can be found we'll go for a variation of this one.

I am pretty against implicit frame switching. It feels like it could get us in trouble. My personal preference is that tests be deterministic so we can look at the test and know exactly what it is doing. Removing that feels like it would make the code prettier but harder to debug if we hit and issue. -1 for it living in Marionette Server from me. I don't really want there to be the possibility of this bleeding through when people are using this for webdriver. ~1 for it living in marionette python code. Since we arent going to be releasing that as part of the webdriver code then I am less anxious of it causing issues to the thousands of users we have. +1 Gaia Test/Gaia - Since this is Gaia related it should live in those frameworks. This would allow people with the greatest context of what the test/code should be doing and can see possibly handle the magic that implicit frame switching will appear like.

Just in case it's not clear we have had a few cases where popup windows don't clear but the test runs and completes behind that because Marionette can only operate in the context of one frame at once. It's a real false positive. While not very frequent, the kind of one I think we should be picking up. I realise testers would probably never come across this kind of scenario on desktop/WebDriver testing so we might be the first ones to have to consider this heavy layered use of full-screen frames together with WebDriver API.

I wonder if we should make interactions on a non-topmost frame throw ElementNotVisible exceptions, or the like? This is totally outside the realm of the WebDriver spec, but as you say, we're really operating in an environment that WebDriver doesn't directly address in all ways. This wouldn't provide automatic frame switching (which I think I agree should not be in core Marionette), but it could provide protection against those false positives you mention.

Well if it was scooped up by the `is_displayed()` atom algorithm then it would be within the WebDriver spec (I think?) becuase the element is shrouded by a HTMLElement above it. However I do rememebr asking David about this in the early days and he did not think we could do it easily. I think we can handle the manual switching, I don't think automatic switching is necessary, just want a way to fail when the top frame or element is not the one we're trying to interact with.

(In reply to David Burns :automatedtester from comment #55) > I am pretty against implicit frame switching. It feels like it could get us > in trouble. My personal preference is that tests be deterministic so we can > look at the test and know exactly what it is doing. Removing that feels like > it would make the code prettier but harder to debug if we hit and issue. I'm also against having implicit frame switching in the Marionette server and the client. Marionette should just provide as many simple actions as possible, and as few compound actions as possible. Getting the modal dialog code in the first place was a pretty big hack to begin with, and the only reason it went in there was because it was halting the the running marionette listener's thread. I don't think it's Marionette's responsibility to keep track of previous frames, since it only needs to worry about the currently executing environment. Another issue is that in b2g, a frame can be unloaded in the background if its process is killed, and this would lead to fun exceptions and edge-cases we'd have to handle. Also, the switching between nested frames is something that's quite gaia specific, and not something I'd find useful outside of b2g's nested frame architecture, so I think it's best suited to be within gaiatest.

(In reply to Zac C (:zac) from comment #58) > Well if it was scooped up by the `is_displayed()` atom algorithm then it > would be within the WebDriver spec (I think?) becuase the element is > shrouded by a HTMLElement above it. However I do rememebr asking David about > this in the early days and he did not think we could do it easily. > > I think we can handle the manual switching, I don't think automatic > switching is necessary, just want a way to fail when the top frame or > element is not the one we're trying to interact with. Nothing has changed, we are governed by what APIs we can use to do this. What we really want is do some hit testing but nothing defines hit testing (bug raised https://www.w3.org/Bugs/Public/show_bug.cgi?id=23825) What we are trying to see is if something is interactable. Is the element interactable or is there some other element above it. I have an outstanding action (http://www.w3.org/2013/11/11-testing-minutes.html#action07) from the last webdriver meeting to describe this. At the moment I want something like "is hittable" as an API name but I can't use that one for obvious reasons (hint: camelcase those two words) I will try document that next week (WebDriver F2F) so we can see about implementing that

I would have liked to do this but I think it's a bit outside of the scope of all the resources we have available.