From 2bb5df4de715f58af68f4a6decd215d12f952aaf Mon Sep 17 00:00:00 2001 From: Sebastian Hengst Date: Wed, 26 Apr 2017 14:12:13 +0200 Subject: [PATCH] Backed out changeset e084deb550c2 (bug 1335778) --- testing/marionette/driver.js | 20 +- .../tests/unit/test_click.py | 57 ++---- .../tests/unit/test_modal_dialogs.py | 2 +- .../marionette_harness/www/clicks.html | 2 - testing/marionette/interaction.js | 21 ++- testing/marionette/listener.js | 171 ++++-------------- 6 files changed, 61 insertions(+), 212 deletions(-) diff --git a/testing/marionette/driver.js b/testing/marionette/driver.js index bb256ae1811a..8f28e0faa845 100644 --- a/testing/marionette/driver.js +++ b/testing/marionette/driver.js @@ -2026,25 +2026,7 @@ GeckoDriver.prototype.clickElement = function* (cmd, resp) { // listen for it and then just send an error back. The person making the // call should be aware something isnt right and handle accordingly this.addFrameCloseListener("click"); - - let click = this.listener.clickElement({id: id, pageTimeout: this.timeouts.pageLoad}); - - // If a remoteness update interrupts our page load, this will never return - // We need to re-issue this request to correctly poll for readyState and - // send errors. - this.curBrowser.pendingCommands.push(() => { - let parameters = { - // TODO(ato): Bug 1242595 - command_id: this.listener.activeMessageId, - pageTimeout: this.timeouts.pageLoad, - startTime: new Date().getTime(), - }; - this.mm.broadcastAsyncMessage( - "Marionette:waitForPageLoaded" + this.curBrowser.curFrameId, - parameters); - }); - - yield click; + yield this.listener.clickElement(id); break; } }; diff --git a/testing/marionette/harness/marionette_harness/tests/unit/test_click.py b/testing/marionette/harness/marionette_harness/tests/unit/test_click.py index 3871cfed34ec..7779ee9a3f36 100644 --- a/testing/marionette/harness/marionette_harness/tests/unit/test_click.py +++ b/testing/marionette/harness/marionette_harness/tests/unit/test_click.py @@ -8,7 +8,7 @@ from marionette_driver.by import By from marionette_driver import errors from marionette_driver.wait import Wait -from marionette_harness import MarionetteTestCase, run_if_e10s +from marionette_harness import MarionetteTestCase def inline(doc): @@ -97,7 +97,11 @@ class TestLegacyClick(MarionetteTestCase): test_html = self.marionette.absolute_url("clicks.html") self.marionette.navigate(test_html) self.marionette.find_element(By.LINK_TEXT, "333333").click() - self.marionette.find_element(By.ID, "username") + # Bug 1335778 - Missing implicit wait for page being loaded + Wait(self.marionette, timeout=self.marionette.timeout.page_load, + ignored_exceptions=errors.NoSuchElementException).until( + lambda m: m.find_element(By.ID, "username"), + message="Username field hasn't been found") self.assertEqual(self.marionette.title, "XHTML Test Page") def test_clicking_an_element_that_is_not_displayed_raises(self): @@ -111,7 +115,11 @@ class TestLegacyClick(MarionetteTestCase): test_html = self.marionette.absolute_url("clicks.html") self.marionette.navigate(test_html) self.marionette.find_element(By.ID, "overflowLink").click() - self.marionette.find_element(By.ID, "username") + # Bug 1335778 - Missing implicit wait for page being loaded + Wait(self.marionette, timeout=self.marionette.timeout.page_load, + ignored_exceptions=errors.NoSuchElementException).until( + lambda m: m.find_element(By.ID, "username"), + message="Username field hasn't been found") self.assertEqual(self.marionette.title, "XHTML Test Page") def test_scroll_into_view_near_end(self): @@ -269,46 +277,3 @@ class TestClick(TestLegacyClick): "does not have pointer events enabled"): button.click() self.assertFalse(self.marionette.execute_script("return window.clicked", sandbox=None)) - - -class TestClickNavigation(MarionetteTestCase): - - def setUp(self): - super(TestClickNavigation, self).setUp() - - def test_click_link_page_load(self): - test_html = self.marionette.absolute_url("clicks.html") - self.marionette.navigate(test_html) - self.marionette.find_element(By.LINK_TEXT, "333333").click() - self.assertNotEqual(self.marionette.get_url(), test_html) - self.assertEqual(self.marionette.title, "XHTML Test Page") - - def test_click_link_anchor(self): - test_html = self.marionette.absolute_url("clicks.html") - self.marionette.navigate(test_html) - self.marionette.find_element(By.ID, "anchor").click() - self.assertEqual(self.marionette.get_url(), "{}#".format(test_html)) - - def test_click_no_link(self): - test_html = self.marionette.absolute_url("clicks.html") - self.marionette.navigate(test_html) - self.marionette.find_element(By.ID, "showbutton").click() - self.assertEqual(self.marionette.get_url(), test_html) - - @run_if_e10s("Requires e10s mode enabled") - def test_click_remoteness_change(self): - test_html = self.marionette.absolute_url("clicks.html") - - self.marionette.navigate("about:robots") - self.marionette.navigate(test_html) - self.marionette.find_element(By.ID, "anchor") - self.marionette.navigate("about:robots") - with self.assertRaises(errors.NoSuchElementException): - self.marionette.find_element(By.ID, "anchor") - - self.marionette.go_back() - self.marionette.find_element(By.ID, "anchor") - self.marionette.find_element(By.ID, "history-back").click() - with self.assertRaises(errors.NoSuchElementException): - self.marionette.find_element(By.ID, "anchor") - diff --git a/testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py b/testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py index f9a93ec611ae..52020e00f016 100644 --- a/testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py +++ b/testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py @@ -8,7 +8,7 @@ from marionette_driver import errors from marionette_driver.marionette import Alert from marionette_driver.wait import Wait -from marionette_harness import MarionetteTestCase, WindowManagerMixin +from marionette_harness import MarionetteTestCase, skip_if_e10s, WindowManagerMixin class BaseAlertTestCase(WindowManagerMixin, MarionetteTestCase): diff --git a/testing/marionette/harness/marionette_harness/www/clicks.html b/testing/marionette/harness/marionette_harness/www/clicks.html index bb2ffc931e21..e48cb1b2f7d6 100644 --- a/testing/marionette/harness/marionette_harness/www/clicks.html +++ b/testing/marionette/harness/marionette_harness/www/clicks.html @@ -17,8 +17,6 @@ I'm a normal link -Back in browser history -Forward in browser history I go to an anchor I open a window with javascript Click me diff --git a/testing/marionette/interaction.js b/testing/marionette/interaction.js index a691576b496a..dbad95631dc8 100644 --- a/testing/marionette/interaction.js +++ b/testing/marionette/interaction.js @@ -178,8 +178,8 @@ function* webdriverClickElement (el, a11y) { yield interaction.flushEventLoop(win); // step 10 - // if the click causes navigation, the post-navigation checks are - // handled by the load listener in listener.js + // TODO(ato): if the click causes navigation, + // run post-navigation checks } function* chromeClick (el, a11y) { @@ -291,19 +291,20 @@ interaction.selectOption = function (el) { * |win| has closed or been unloaded before the queue can be flushed. */ interaction.flushEventLoop = function* (win) { - return new Promise(resolve => { - let handleEvent = event => { - win.removeEventListener("beforeunload", this); - resolve(); - }; + let unloadEv; + return new Promise((resolve, reject) => { if (win.closed) { - resolve(); + reject(); return; } - win.addEventListener("beforeunload", handleEvent); - win.requestAnimationFrame(handleEvent); + unloadEv = reject; + win.addEventListener("unload", unloadEv, {once: true}); + + win.requestAnimationFrame(resolve); + }).then(() => { + win.removeEventListener("unload", unloadEv); }); }; diff --git a/testing/marionette/listener.js b/testing/marionette/listener.js index a4e907b2edb9..f85604944e48 100644 --- a/testing/marionette/listener.js +++ b/testing/marionette/listener.js @@ -117,10 +117,8 @@ var sandboxName = "default"; */ var loadListener = { command_id: null, - seenUnload: null, timeout: null, - timerPageLoad: null, - timerPageUnload: null, + timer: null, /** * Start listening for page unload/load events. @@ -138,81 +136,48 @@ var loadListener = { this.command_id = command_id; this.timeout = timeout; - this.seenUnload = false; - - this.timerPageLoad = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); - this.timerPageUnload = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); - // In case of a remoteness change, only wait the remaining time timeout = startTime + timeout - new Date().getTime(); if (timeout <= 0) { - this.notify(this.timerPageLoad); + this.notify(); return; } + this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); + this.timer.initWithCallback(this, timeout, Ci.nsITimer.TYPE_ONE_SHOT); + if (waitForUnloaded) { addEventListener("hashchange", this, false); addEventListener("pagehide", this, false); - - // The event can only be received if the listener gets added to the - // currently selected frame. - curContainer.frame.addEventListener("beforeunload", this, false); - - Services.obs.addObserver(this, "outer-window-destroyed"); } else { addEventListener("DOMContentLoaded", loadListener, false); addEventListener("pageshow", loadListener, false); } - - this.timerPageLoad.initWithCallback(this, timeout, Ci.nsITimer.TYPE_ONE_SHOT); }, /** * Stop listening for page unload/load events. */ stop: function () { - if (this.timerPageLoad) { - this.timerPageLoad.cancel(); - } - - if (this.timerPageUnload) { - this.timerPageUnload.cancel(); + if (this.timer) { + this.timer.cancel(); + this.timer = null; } removeEventListener("hashchange", this); removeEventListener("pagehide", this); removeEventListener("DOMContentLoaded", this); removeEventListener("pageshow", this); - - // If the original content window, where the navigation was triggered, - // doesn't exist anymore, exceptions can be silently ignored. - try { - curContainer.frame.removeEventListener("beforeunload", this); - } catch (e if e.name == "TypeError") {} - - // In the case when the observer was added before a remoteness change, - // it will no longer be available. Exceptions can be silently ignored. - try { - Services.obs.removeObserver(this, "outer-window-destroyed"); - } catch (e) {} }, /** * Callback for registered DOM events. */ handleEvent: function (event) { - logger.debug(`Received DOM event "${event.type}" for "${event.originalTarget.baseURI}"`); - switch (event.type) { - case "beforeunload": - this.seenUnload = true; - break; - case "pagehide": if (event.originalTarget === curContainer.frame.document) { - this.seenUnload = true; - removeEventListener("hashchange", this); removeEventListener("pagehide", this); @@ -258,43 +223,9 @@ var loadListener = { * Callback for navigation timeout timer. */ notify: function (timer) { - switch (timer) { - // If the page unload timer is raised, ensure to properly stop the load - // listener, and return from the currently active command. - case this.timerPageUnload: - if (!this.seenUnload) { - logger.debug("Canceled page load listener because no navigation " + - "has been detected"); - this.stop(); - sendOk(this.command_id); - } - break; - - case this.timerPageLoad: - this.stop(); - sendError(new TimeoutError(`Timeout loading page after ${this.timeout}ms`), - this.command_id); - break; - } - }, - - observe: function (subject, topic, data) { - const winID = subject.QueryInterface(Components.interfaces.nsISupportsPRUint64).data; - const curWinID = curContainer.frame.QueryInterface(Ci.nsIInterfaceRequestor) - .getInterface(Ci.nsIDOMWindowUtils).outerWindowID; - - logger.debug(`Received observer notification "${topic}" for "${winID}"`); - - switch (topic) { - // In the case when the currently selected frame is about to close, - // there will be no further load events. Stop listening immediately. - case "outer-window-destroyed": - if (curWinID === winID) { - this.stop(); - sendOk(this.command_id); - } - break; - } + this.stop(); + sendError(new TimeoutError("Timeout loading page after " + this.timeout + "ms"), + this.command_id); }, /** @@ -320,15 +251,23 @@ var loadListener = { * @param {number} command_id * ID of the currently handled message between the driver and listener. * @param {number} pageTimeout - * Timeout in milliseconds the method has to wait for the page finished loading. - * @param {boolean=} loadEventExpected - * Optional flag, which indicates that navigate has to wait for the page - * finished loading. + * Timeout in milliseconds the method has to wait for the page being finished loading. * @param {string=} url * Optional URL, which is used to check if a page load is expected. */ - navigate: function (trigger, command_id, timeout, loadEventExpected = true, - useUnloadTimer = false) { + navigate: function (trigger, command_id, timeout, url = undefined) { + let loadEventExpected = true; + + if (typeof url == "string") { + try { + let requestedURL = new URL(url).toString(); + loadEventExpected = navigate.isLoadEventExpected(requestedURL); + } catch (e) { + sendError(new InvalidArgumentError("Malformed URL: " + e.message), command_id); + return; + } + } + if (loadEventExpected) { let startTime = new Date().getTime(); this.start(command_id, timeout, startTime, true); @@ -338,14 +277,8 @@ var loadListener = { yield trigger(); }).then(val => { - if (!loadEventExpected) { + if (!loadEventExpected) { sendOk(command_id); - return; - } - - // If requested setup a timer to detect a possible page load - if (useUnloadTimer) { - this.timerPageUnload.initWithCallback(this, 200, Ci.nsITimer.TYPE_ONE_SHOT); } }).catch(err => { @@ -353,6 +286,7 @@ var loadListener = { this.stop(); } + // Check why we do not raise an error if err is of type Event sendError(err, command_id); return; }); @@ -468,6 +402,7 @@ function removeMessageListenerId(messageName, handler) { var getTitleFn = dispatch(getTitle); var getPageSourceFn = dispatch(getPageSource); var getActiveElementFn = dispatch(getActiveElement); +var clickElementFn = dispatch(clickElement); var getElementAttributeFn = dispatch(getElementAttribute); var getElementPropertyFn = dispatch(getElementProperty); var getElementTextFn = dispatch(getElementText); @@ -522,7 +457,7 @@ function startListeners() { addMessageListenerId("Marionette:findElementContent", findElementContentFn); addMessageListenerId("Marionette:findElementsContent", findElementsContentFn); addMessageListenerId("Marionette:getActiveElement", getActiveElementFn); - addMessageListenerId("Marionette:clickElement", clickElement); + addMessageListenerId("Marionette:clickElement", clickElementFn); addMessageListenerId("Marionette:getElementAttribute", getElementAttributeFn); addMessageListenerId("Marionette:getElementProperty", getElementPropertyFn); addMessageListenerId("Marionette:getElementText", getElementTextFn); @@ -627,7 +562,7 @@ function deleteSession(msg) { removeMessageListenerId("Marionette:findElementContent", findElementContentFn); removeMessageListenerId("Marionette:findElementsContent", findElementsContentFn); removeMessageListenerId("Marionette:getActiveElement", getActiveElementFn); - removeMessageListenerId("Marionette:clickElement", clickElement); + removeMessageListenerId("Marionette:clickElement", clickElementFn); removeMessageListenerId("Marionette:getElementAttribute", getElementAttributeFn); removeMessageListenerId("Marionette:getElementProperty", getElementPropertyFn); removeMessageListenerId("Marionette:getElementText", getElementTextFn); @@ -1166,26 +1101,15 @@ function waitForPageLoaded(msg) { */ function get(msg) { let {command_id, pageTimeout, url} = msg.json; - let loadEventExpected = true; try { - if (typeof url == "string") { - try { - let requestedURL = new URL(url).toString(); - loadEventExpected = navigate.isLoadEventExpected(requestedURL); - } catch (e) { - sendError(new InvalidArgumentError("Malformed URL: " + e.message), command_id); - return; - } - } - // We need to move to the top frame before navigating sendSyncMessage("Marionette:switchedToFrame", {frameValue: null}); curContainer.frame = content; loadListener.navigate(() => { curContainer.frame.location = url; - }, command_id, pageTimeout, loadEventExpected); + }, command_id, pageTimeout, url); } catch (e) { sendError(e, command_id); @@ -1331,36 +1255,15 @@ function getActiveElement() { /** * Send click event to element. * - * @param {number} command_id - * ID of the currently handled message between the driver and listener. * @param {WebElement} id * Reference to the web element to click. - * @param {number} pageTimeout - * Timeout in milliseconds the method has to wait for the page being finished loading. */ -function clickElement(msg) { - let {command_id, id, pageTimeout} = msg.json; - - try { - let loadEventExpected = true; - - let target = getElementAttribute(id, "target"); - - if (target === "_blank") { - loadEventExpected = false; - } - - loadListener.navigate(() => { - return interaction.clickElement( - seenEls.get(id, curContainer), - capabilities.get("moz:accessibilityChecks"), - capabilities.get("specificationLevel") >= 1 - ); - }, command_id, pageTimeout, loadEventExpected, true); - - } catch (e) { - sendError(e, command_id); - } +function clickElement(id) { + let el = seenEls.get(id, curContainer); + return interaction.clickElement( + el, + capabilities.get("moz:accessibilityChecks"), + capabilities.get("specificationLevel") >= 1); } function getElementAttribute(id, name) {