608446 - Test failure in testSetToCurrentPage.js and testRestoreHomePageToDefault.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Browser home page preference is failing to be set when clicking on the 'Use Current Page' button for the Home Page in the General Preferences pane.

Test output is:

ERROR | Test Failure: {"exception": {"message": "could not validate element ID: urlbar with value http://localhost:43336/layout/mozilla.html", "lineNumber": 876, "stack": "([object Object],\"http://localhost:43336/layout/mozilla.html\")@resource://mozmill/modules/controller.js:876\n()@resource://mozmill/modules/frame.js -> file:///Users/dave/workspace/my-mozmill-tests/firefox/testPreferences/testSetToCurrentPage.js:92\n((function () {controller.open(LOCAL_TEST_PAGES[1]);controller.waitForPageLoad();controller.click(new elementslib.ID(controller.window.document, \"home-button\"));controller.waitForPageLoad();controller.sleep(30000);controller.assertValue(locationBar.urlbar, LOCAL_TEST_PAGES[0]);}))@resource://mozmill/modules/frame.js:530\n([object Object])@resource://mozmill/modules/frame.js:598\n([object Object])@resource://mozmill/modules/frame.js:641\n(\"/Users/dave/workspace/my-mozmill-tests/firefox/testPreferences/testSetToCurrentPage.js\")@resource://mozmill/modules/frame.js:480\n(\"/Users/dave/workspace/my-mozmill-tests/firefox/testPreferences/testSetToCurrentPage.js\")@resource://mozmill/modules/frame.js:653\n((function (filename, invokedFromIDE) {var runner = new Runner(new Collector, invokedFromIDE);runner.runTestFile(filename);runner.end();return true;}),[object Proxy])@resource://jsbridge/modules/server.js:164\n(\"2be4607e-e3ba-11df-90bc-d830626155ec\",(function (filename, invokedFromIDE) {var runner = new Runner(new Collector, invokedFromIDE);runner.runTestFile(filename);runner.end();return true;}),[object Proxy])@resource://jsbridge/modules/server.js:168\n@:0\n@resource://jsbridge/modules/server.js:249\n", "fileName": "resource://mozmill/modules/controller.js"}}
TEST-UNEXPECTED-FAIL | /Users/dave/workspace/my-mozmill-tests/firefox/testPreferences/testSetToCurrentPage.js | testHomeButton

When running with debug the following appeared in the console:

Warning: reference to undefined property window.documentLoaded
Source File: resource://mozmill/modules/controller.js
Line: 310

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

In case of qa-horus I was able to fix this failure by replacing the following line:

http://hg.mozilla.org/qa/mozmill-tests/file/default/shared-modules/prefs.js#l138

with:

> this._controller.keypress(null, 'W', {accelKey: true});

Dave, can you please check if that works for you? Not sure yet about the mozmill failure itself. Could have been raised already earlier?

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I can confirm that this has fixed the failure. I can also confirm that in Firefox 3.5 and 3.6 the user can hit the ESCAPE key to dismiss the preferences pane and the modified home page will persist. In Firefox 4 Beta 6 and Minefield, hitting the ESCAPE key loses the modification.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So the issue with ESC on 4.0 with any value inside the textbox? This sounds like a regression in Firefox we should investigate. Please check former beta builds or even alpha builds + nightly builds to get a regression range.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Oh, and how does accelKey+W work on Linux?

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Discovered this to be a regression, as detailed in bug 608878

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The same failure should happen for the restore homepage to default test. Can you please check this and update the summary?

Comment 7

[Aaron Train [:aaronmt]]

(In reply to comment #6)
> The same failure should happen for the restore homepage to default test. Can
> you please check this and update the summary?

Absolutely correct. The test will fail if the OSX Keyboard Preferences, 'Full Keyboard Access' are set to: 'Text Box and Lists Only'

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Given that the preference dialog class never knows which element has the focus, and probably shouldn't have to check this, I would say that we should change the close function to not make use of the Escape key but the AccelKey+W combo.

Dave, can you please check if that works on Linux as well?

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The AccelKey+W is working on Linux, however should we not ideally exercise both method of dismissing the preferences dialog in our tests? Otherwise we wouldn't pick up of this type of bug.

Comment 10

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Somewhere there should be tests that exercise closing the dialog and checking that the values in general persist, yeah.

Whether it should be -this- particular test, unsure. The tests that aren't specifically checking for on-close behavior should use the most robust close method available to decrease our maintenance.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

In general tests should use a default method to close dialogs/windows or other types of widgets. As Geo mentioned I would go on here and set Accel+W as the default action. We could come up with a simple and single test which can test this specific feature.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Good point. I suggest a patch to change this test to use AccelKey+W and another patch to add tests for the persist preferences when closing the dialog. Can find some time to work on this together when I'm in MV next week?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I would prefer that we fix this bug ASAP. Means simply create a patch which fixes the close() function inside the prefs.js module. Can you do that?

Before we can create any test for the escape issue, we will have to wait for a fix on bug 608878.

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I will create that patch now.

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Changed the keyboard shortcut for closing preferences from Escape to Accel+W

Comment 16

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 488512 [details] [diff] [review]
Changed the keyboard shortcut for closing preferences from Escape to Accel+W

>-      this._controller.keypress(null, 'VK_ESCAPE', {});
>+      this._controller.keypress(null, 'W', {accelKey: true});

Can you do a quick respin w/ lowercase 'w'?  Normally I'd just edit it and land, but it'll let you try out the "obsolete patch" feature, etc.

keypress() doesn't treat uppercase as shift+W, so this will work. However, we've pretty much standardized on lowercase keycodes to keep things as obvious as possible.

Otherwise, looks fine.

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Changed the keyboard shortcut for closing preferences from Escape to Accel+w

Changed the keyboard shortcut for closing preferences from Escape to Accel+w

Comment 18

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 488557 [details] [diff] [review]
Changed the keyboard shortcut for closing preferences from Escape to Accel+w

Looks great, thanks for the change! 

I'll land it presently.

Comment 19

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Landed on default as http://hg.mozilla.org/qa/mozmill-tests/rev/2fbfb150fefa

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is a fix for a broken behavior of the shared module and should also be fixed on the branches. The reason why it doesn't happen there is simply a focus bug in Firefox.

Comment 21

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Sorry about that--I thought the older behavior was correct and we were working around a trunk bug.  I'm assigning it to myself--I'll get the patches applied to 1.9.2 and 1.9.1 as well.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Assignee is still Dave, who wrote the patch. I don't think any update is necessary to get this patch into the older branches.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dave, can you please provide an update of the patch for the 1.9.2 branch? There is no need for 1.9.1.

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Changed the keyboard shortcut for closing preferences from Escape to Accel+w (1.9.2) v1

(In reply to comment #24)
> Dave, can you please provide an update of the patch for the 1.9.2 branch?
> There is no need for 1.9.1.

Patch attached for branch 1.9.2

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 531511 [details] [diff] [review]
Changed the keyboard shortcut for closing preferences from Escape to Accel+w (1.9.2) v1

Thanks!

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Landed on 1.9.2 as:
http://hg.mozilla.org/qa/mozmill-tests/rev/3d74a850fb80