879418 - Create metro mozmill test for form filling

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Andreea Matei [:AndreeaMatei]]

This test should fill a form page for the metro mode.

Comment 1

[Andreea Matei [:AndreeaMatei]]

Referrence: bug 831977.

Comment 2

[Daniel Gherasim]

Attachment: metro_testBasicFormCompletion_v1.patch

I have created a new patch for this test that does the same thing as 
firefox/tests/functional/testFormManager/testBasicFormCompletion.js 
but for metrofirefox and using the FormHistory.jsm library.

Also checked to see it's functionality on latest aurora and nightly versions with 10 exhaustive tests and it works fine.

Comment 3

[Andrei Eftimie]

Comment on attachment 8362865 [details] [diff] [review]
metro_testBasicFormCompletion_v1.patch

Review of attachment 8362865 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start Daniel.

There are however a few issues we'll need to fix, please see inline.
Most are related to naming, style and conventions. I find the test itself in good shape.

::: metrofirefox/tests/functional/testFormManager/testBasicFormCompletion.js
@@ +13,5 @@
> +const TEST_DATA = BASE_URL + "form_manager/form.html";
> +
> +var setupModule = function(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  

nit: extra whitespace

To avoid these you can set your texteditor to automatically trim extra whitespaces on save.

@@ +15,5 @@
> +var setupModule = function(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  
> +  // Clear complete form history so we don't interfer with already added entries
> +  FormHistory.update({op:"remove"});

nit: add a space after the colon

@@ +20,5 @@
> +}
> +
> +var testFormCompletion = function() {
> +  var inputText = "John";
> +  var shortInputText = "Jo";

These should be constants.
And because for the second item we want the first 2 letters of the first, we can use substring to get them directly.

@@ +31,5 @@
> +  assert.ok(inputField.exists(), "Name field has been found");
> +
> +  // Fill out the name field with the input text and click the Submit button
> +  inputField.sendKeys(inputText);
> +

nit: please remove this extra line

@@ +32,5 @@
> +
> +  // Fill out the name field with the input text and click the Submit button
> +  inputField.sendKeys(inputText);
> +
> +  var submitButton = new elementslib.ID(controller.tabs.activeTab, 

nit: extra whitespace

@@ +37,5 @@
> +                                        "SubmitButton");
> +  submitButton.click();
> +  controller.waitForPageLoad();
> +
> +  // Go to a filler site: about:blank

I would call this 'page' instead of 'site'

@@ +41,5 @@
> +  // Go to a filler site: about:blank
> +  controller.open("about:blank");
> +  controller.waitForPageLoad();
> +
> +  // Go back to the starting local page

I think we can remove 'local' from this comment.

@@ +45,5 @@
> +  // Go back to the starting local page
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +
> +  // Verify name field element, and type in a portion of the field

We are not verifying the element here.
The text here can be simpler, like:
"Type in the first 2 letters of the previously used entry"

@@ +62,5 @@
> +  assert.waitFor(function() {
> +    var cs = controller.window.getComputedStyle(popAutoFillContainer.getNode(),
> +                                                null);
> +    return cs.getPropertyValue("opacity") >= 1;
> +  }, "Window popoup has been opened");

Is there anything else we can wait for besides the opacity change as a CSS value?

Please check for either a property on the element, or even better if we find an event that triggers when the autocomplete opens.

@@ +64,5 @@
> +                                                null);
> +    return cs.getPropertyValue("opacity") >= 1;
> +  }, "Window popoup has been opened");
> +
> +  inputField.keypress("VK_DOWN", {});

I don't think this step is needed. It should be enough to click on the item.

@@ +68,5 @@
> +  inputField.keypress("VK_DOWN", {});
> +  popAutoFill.click();
> +
> +  assert.equal(inputField.getNode().value, inputText,
> +               "Input field has the correct text in it");

I would drop the "in it" part.

Comment 4

[Daniel Gherasim]

Attachment: metro_testBasicFormCompletion_v2.patch

Thanks Andrei for the review.
The new patch is now updated with the requested adjustments.

I hope this test is ok now.

P.S.: Given that the popup window appears with an opacity transition, we're waiting for this event to finish now, so we can then select the wanted item and handle it.

Comment 5

[Andrei Eftimie]

Comment on attachment 8366539 [details] [diff] [review]
metro_testBasicFormCompletion_v2.patch

Review of attachment 8366539 [details] [diff] [review]:
-----------------------------------------------------------------

It would be great to have parts of code in a UI library, but we'll probably handle that later.

Just a few issues, see inline:

::: metrofirefox/tests/functional/testFormManager/testBasicFormCompletion.js
@@ +14,5 @@
> +const TEST_DATA = BASE_URL + "form_manager/form.html";
> +
> +// Sample strings used in our tests
> +const INPUT_TEXT = "John";
> +const SHORT_INPUT_TEXT = INPUT_TEXT.substring(0,2);

Don't make this another constant, just call this directly when you need it.

@@ +22,5 @@
> +
> +  // Clear complete form history so we don't interfer with already added entries
> +  FormHistory.update({op: "remove"});
> +}
> +

If anything fails in the test we need to clean in up in the teardownModule.

For this test we should close all tabs and open the default page.

@@ +54,5 @@
> +
> +  // Wait for the autofill popup to open
> +  var transitionend = false;
> +  function onTransitionEnd() {
> +     transitionend = true;

nit: this is indented with 3 spaces

@@ +62,5 @@
> +  var popAutoFillContainer = new elementslib.ID(controller.window.document,
> +                                                "autofill-container");
> +  popAutoFillContainer = popAutoFillContainer.getNode();
> +  popAutoFillContainer.addEventListener("transitionend",
> +                                        onTransitionEnd, false);

You can remove the 3rd argument here.
userCapture defaults to false.

See https://developer.mozilla.org/en-US/docs/Web/API/EventTarget.addEventListener

@@ +70,5 @@
> +    }, "Window popoup has been opened");
> +  }
> +  finally {
> +    popAutoFillContainer.removeEventListener("transitionend",
> +                                             onTransitionEnd, false);

Same here, 3rd argument can be removed.

@@ +77,5 @@
> +  // Select the first item of autofill window and click on it
> +  var popAutoFill = new elementslib.Selector(controller.window.document,
> +                                             "#autofill-container #menupopup-commands");
> +  var autoFillFirstItem = new elementslib.Elem(popAutoFill.getNode().getItemAtIndex(0));
> +  autoFillFirstItem.click();

I think all autofill popup methods and elements should be part of a UI class.

Given our current need to have some Metro tests running, I'm good with leaving this code in the test for now.
But please file a bug to create a UI library for handling context menus, and move this specific code to that Library.

Comment 6

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

(In reply to Andrei Eftimie from comment #5)
> Given our current need to have some Metro tests running, I'm good with
> leaving this code in the test for now.
> But please file a bug to create a UI library for handling context menus, and
> move this specific code to that Library.

No, please implement it correctly. It will not get my review. Also for context menus you have to use controller.Menu() instead. Please don't handle those yourself.

Comment 7

[Daniel Gherasim]

Attachment: metro_testBasicFormCompletion_v3.patch

Updated patch with requested changes.

Comment 8

[Daniel Gherasim]

Some comments related to the latest uploaded patch.

The controller.Menu() object doesn't apply for metrofirefox menus & contextmenus, at least it's open(), close() or _buildMenu() functions. The reason for this is that we don't have the <menupopup> and <popup> tags anymore and the menus are handled a bit differently (like the state parameter it's now popupState).

Given that this test doesn't need the options that don't work, just the ones that actually selects the menu having a jQuery like selector, we can use the object here.

I've used the existing methods(that work) since they are the ones we need for this particular case and I've opened bug 966963 to make the other methods of Menu() work on MetroFirefox.

Comment 9

[Andrei Eftimie]

Comment on attachment 8367942 [details] [diff] [review]
metro_testBasicFormCompletion_v3.patch

Review of attachment 8367942 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. 
Please make a small update as noted inline, then request review from Henrik for a final input.
With that change you have an r+ from me.

::: metrofirefox/tests/functional/testFormManager/testBasicFormCompletion.js
@@ +30,5 @@
> +
> +var teardownModule = function(aModule) {
> +  tabs.closeAllTabs(controller);
> +
> +  controller.open(utils.getDefaultHomepage());

I could have been more clear before. Sorry.
tabs.closeAllTabs() is enough since it already opens the default page, so no need to call this manually here.

We can remove this line.

Comment 10

[Daniel Gherasim]

Attachment: metro_testBasicFormCompletion_v3.1.patch

Thanks for reviews on this bug.
Updated patch as requested and asked Henrik for review.

Comment 11

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

Comment on attachment 8370695 [details] [diff] [review]
metro_testBasicFormCompletion_v3.1.patch

Review of attachment 8370695 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/tests/functional/testFormManager/testBasicFormCompletion.js
@@ +16,5 @@
> +
> +// Sample string used in our tests
> +const INPUT_TEXT = "John";
> +
> +var setupModule = function(aModule) {

Please no 'var' declarations.

@@ +23,5 @@
> +
> +  tabs.closeAllTabs(aModule.controller);
> +
> +  // Clear complete form history so we don't interfer with already added entries
> +  FormHistory.update({op: "remove"});

This method is asynchronously. So this will actually add a race condition given that you do not know how long it takes to actually perform this action. Also I would see code like this not in the test but in a backend library for form data.

@@ +27,5 @@
> +  FormHistory.update({op: "remove"});
> +}
> +
> +var teardownModule = function(aModule) {
> +  tabs.closeAllTabs(controller);

We also have to ensure to remove all newly stored form entries.

@@ +32,5 @@
> +}
> +
> +var testFormCompletion = function() {
> +
> +  // Open the local page

nit: no need for the blank line above.

@@ +40,5 @@
> +  var inputField = new elementslib.ID(controller.tabs.activeTab, "ship_fname");
> +  assert.ok(inputField.exists(), "Name field has been found");
> +
> +  // Fill out the name field with the input text and click the Submit button
> +  inputField.sendKeys(INPUT_TEXT);

I do not think that there is a need for this comment. The code is more than self-describing.

@@ +46,5 @@
> +                                        "SubmitButton");
> +  submitButton.click();
> +  controller.waitForPageLoad();
> +
> +  // Go to a filler page: about:blank

Same here. If you want a comment you should explain why we have to open it.

@@ +50,5 @@
> +  // Go to a filler page: about:blank
> +  controller.open("about:blank");
> +  controller.waitForPageLoad();
> +
> +  // Go back to the starting page

nit: 'test page'

@@ +55,5 @@
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +
> +  // Type in the first 2 letters of the previously used entry
> +  inputField = new elementslib.ID(controller.tabs.activeTab, "ship_fname");

With mozmill 2 there shouldn't be a need to retrieve the element again. It should work. Doesn't it?

@@ +56,5 @@
> +  controller.waitForPageLoad();
> +
> +  // Type in the first 2 letters of the previously used entry
> +  inputField = new elementslib.ID(controller.tabs.activeTab, "ship_fname");
> +  inputField.sendKeys(INPUT_TEXT.substring(0,2));

This will not work. You issue the action before you actually register the event listener. 

nit: space after comma.

@@ +76,5 @@
> +    }, "Window popoup has been opened");
> +  }
> +  finally {
> +    popAutoFillContainer.removeEventListener("transitionend", onTransitionEnd);
> +  }

All the above code is suited to be placed into a library but not within a test. This is definitely re-usable code and only blows up all of the form tests.

@@ +79,5 @@
> +    popAutoFillContainer.removeEventListener("transitionend", onTransitionEnd);
> +  }
> +
> +  // Click on the first item of the auto fill popup
> +  var autoFillMenu = controller.getMenu("#autofill-container #menupopup-commands");

I thought that the Menu code doesn't work? We should also get the ids into a lib.

Comment 12

[Daniel Gherasim]

Attachment: metro_testBasicFormCompletion_v4.patch

(In reply to Henrik Skupin (:whimboo) from comment #11)
> Comment on attachment 8370695 [details] [diff] [review]
> metro_testBasicFormCompletion_v3.1.patch
> 
> Review of attachment 8370695 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: metrofirefox/tests/functional/testFormManager/testBasicFormCompletion.js
> @@ +23,5 @@
> > +
> > +  tabs.closeAllTabs(aModule.controller);
> > +
> > +  // Clear complete form history so we don't interfer with already added entries
> > +  FormHistory.update({op: "remove"});
> 
> This method is asynchronously. So this will actually add a race condition
> given that you do not know how long it takes to actually perform this
> action. Also I would see code like this not in the test but in a backend
> library for form data.
> 
> @@ +76,5 @@
> > +    }, "Window popoup has been opened");
> > +  }
> > +  finally {
> > +    popAutoFillContainer.removeEventListener("transitionend", onTransitionEnd);
> > +  }
> 
> All the above code is suited to be placed into a library but not within a
> test. This is definitely re-usable code and only blows up all of the form
> tests.
> 

I uptated the test to wait for the remove action to complete & filled bug 969428 to create the form data library. When it's finished we can do a follow-up patch on this.

> @@ +79,5 @@
> > +    popAutoFillContainer.removeEventListener("transitionend", onTransitionEnd);
> > +  }
> > +
> > +  // Click on the first item of the auto fill popup
> > +  var autoFillMenu = controller.getMenu("#autofill-container #menupopup-commands");
> 
> I thought that the Menu code doesn't work? We should also get the ids into a
> lib.

For what we need on this test, this works just fine. Problems will occur if we want to use in other tests the other methods of controller.Menu().

Updated patch & asked Andrei and Adreea for a recheck.

Comment 13

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8372332 [details] [diff] [review]
metro_testBasicFormCompletion_v4.patch

Review of attachment 8372332 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/tests/functional/testFormManager/testBasicFormCompletion.js
@@ +26,5 @@
> +  clearFormHistory();
> +}
> +
> +function teardownModule(aModule) {
> +  tabs.closeAllTabs(controller);

aModule.controller

@@ +35,5 @@
> +/**
> + * Test form completion
> + */
> +function testFormCompletion() {
> +  // Open the local test page

No need for the comment.

@@ +45,5 @@
> +  inputField.sendKeys(INPUT_TEXT);
> +
> +  var submitButton = new elementslib.ID(controller.tabs.activeTab,
> +                                        "SubmitButton");
> +  submitButton.click();

Let's use tap() please.

@@ +65,5 @@
> +
> +  // Select the popup window
> +  var popAutoFillContainer = new elementslib.ID(controller.window.document,
> +                                                "autofill-container");
> +  popAutoFillContainer = popAutoFillContainer.getNode();

I would remove this line and simply use .getNode() where needed.

@@ +81,5 @@
> +  }
> +
> +  // Click on the first item of the auto fill popup
> +  var autoFillMenu = controller.getMenu("#autofill-container #menupopup-commands");
> +  autoFillMenu.click("richlistitem:first-child");

tap()

@@ +87,5 @@
> +  assert.equal(inputField.getNode().value, INPUT_TEXT,
> +               "Input field has the correct text");
> +}
> +
> +function clearFormHistory() {

We should add a TODO comment with the bug reference to replace this.

Comment 14

[Daniel Gherasim]

Attachment: metro_testBasicFormCompletion_v5.patch

So according to last Henrik's and Andreea's reviews, I made this changes for this test:

* Created a new class AutoFillPopup in ui/toolbars.js to handle this
** created methods for opening and closing it
** created methods for geting the popup or it's items (by index or with a jQuery selector)

Comment 15

[Andrei Eftimie]

Comment on attachment 8378277 [details] [diff] [review]
metro_testBasicFormCompletion_v5.patch

Review of attachment 8378277 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/lib/ui/toolbars.js
@@ +31,5 @@
> +
> +  /**
> +   * Wait for the autofill popup to close
> +   *
> +   * @params {function} aTriggerOpen

aTriggerClose?

@@ +65,5 @@
> +   * Retrieve list of UI elements based on the given specification
> +   *
> +   * @param {Object} aSpec
> +   *        Information of the UI elements which should be retrieved
> +   * @param {Object} type

You should prefix all subelements of an object with their parents name.
So `aSpec.type` etc

@@ +87,5 @@
> +        elem = new elementslib.Elem(container.querySelector("richlistbox"));
> +        break;
> +      case "result":
> +        elem = new elementslib.Elem(this.getElement({type: "results"}).
> +                                        getNode().getItemAtIndex(aSpec.value));

What happens if there's no aSpec.value?
We should either throw if we need a value and we don't have it, or gracefully use some defaults.
In this case I think defaulting to 0 might be good (to retrieve the first item).

@@ +89,5 @@
> +      case "result":
> +        elem = new elementslib.Elem(this.getElement({type: "results"}).
> +                                        getNode().getItemAtIndex(aSpec.value));
> +        break;
> +      case "result-selector":

Not sure. Do we really need this?

*BTW, that is a CSS selector, not a jQuery selector.
jQuery uses CSS selectors.

::: metrofirefox/tests/functional/testFormManager/testBasicFormCompletion.js
@@ +65,5 @@
> +    var autoFillItem = autoFillPopup.getElement({type: "result", value: 0});
> +    autoFillItem.tap();
> +  }
> +
> +  autoFillPopup.open(triggerPopupOpen);

Since these are so small I would suggest to use anonymous functions instead. Will yield better readability

Comment 16

[Daniel Gherasim]

Attachment: metro_testBasicFormCompletion_v5.1.patch

(In reply to Andrei Eftimie from comment #15)
> > +        break;
> > +      case "result-selector":
> 
> Not sure. Do we really need this?

No, we don't need this for the AutofillPopup ATM => removed it.

Updated the patch as requested.

Comment 17

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8379005 [details] [diff] [review]
metro_testBasicFormCompletion_v5.1.patch

Review of attachment 8379005 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/lib/ui/toolbars.js
@@ +12,5 @@
>  /**
>   * Constructor
> + */
> +function AutoFillPopup(aController) {
> +  this._controller = aController;

Thinking more about this, it shouldn't be in this library. It's not related to toolbars, it's a autocompletipn popup. Maybe more similar to context menus.. I think we should move it to a utils.js library, similar to the one in firefox.

@@ +34,5 @@
> +   *
> +   * @params {function} aTriggerClose
> +   *         Function that triggers the popup to open
> +   */
> +  close : function autofillpopup_close(aTriggerClose) {

All the cases with "function autofillpopup" should have camel case like you called the class: autoFillPopup

@@ +80,5 @@
> +  getElements : function autofillpopup_getElements(aSpec) {
> +    var elem = null;
> +    var type = aSpec.type || "";
> +
> +

You have 2 blank lines here.

@@ +115,5 @@
> +  /**
> +   * Trigger open/close events and wait for them to finish
> +   *
> +   * @params {function} aTrigger
> +   *         Function that triggers the popup to open/close

We call these functions aCallback in all libraries.

::: metrofirefox/tests/functional/testFormManager/testBasicFormCompletion.js
@@ +70,5 @@
> +               "Input field has the correct text");
> +}
> +
> +// TO DO
> +// Once bug 969428 is landed, remove this function and use the library

TODO's are in this form:

// Bug 969428
// TODO: Remove this function and use the forms library

Comment 18

[Daniel Gherasim]

Attachment: metro_testBasicFormCompletion_v5.2.patch

Thanks for review,
I have updated the patch now with the requested changes.

Comment 19

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8380493 [details] [diff] [review]
metro_testBasicFormCompletion_v5.2.patch

Review of attachment 8380493 [details] [diff] [review]:
-----------------------------------------------------------------

Other than a small update, looks good.

::: metrofirefox/lib/ui/utils.js
@@ +88,5 @@
> +        break;
> +      case "result":
> +        var value = aSpec.value || 0;
> +        elem = new elementslib.Elem(this.getElement({type: "results"}).
> +                                        getNode().getItemAtIndex(value));

There a indentation issue here.

Comment 20

[Daniel Gherasim]

Attachment: metro_testBasicFormCompletion_v5.3.patch

Thanks, 
Updated the patch and asked Dave & Henrik for review.

Comment 21

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

Comment on attachment 8382937 [details] [diff] [review]
metro_testBasicFormCompletion_v5.3.patch

Review of attachment 8382937 [details] [diff] [review]:
-----------------------------------------------------------------

Leaving the review request for Henrik, but I've added my thoughts.

::: metrofirefox/tests/functional/testFormManager/testBasicFormCompletion.js
@@ +19,5 @@
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.tabs = new tabs.TabBrowser(aModule.controller);
> +  aModule.autoFillPopup = new utils.AutoFillPopup(aModule.controller);

utils is a bit generic, we should put this into a forms library.

@@ +56,5 @@
> +  // Go back to the test page
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +
> +  autoFillPopup.open(function() {

I find these open/close methods to be counter-intuitive. I would prefer an API that more closely resembled the user's actions, such as autoFillPopup.sendKeys, which waited for the appropriate transition, and autoFillPopup.results array that can then be used to tap the first element.

@@ +70,5 @@
> +               "Input field has the correct text");
> +}
> +
> +// Bug 969428
> +// TODO: Remove this function and use the forms library

Can we block this bug on bug 969428 rather than come back to it?

Comment 22

[Andrei Eftimie]

(In reply to Dave Hunt (:davehunt) from comment #21)
> @@ +70,5 @@
> > +               "Input field has the correct text");
> > +}
> > +
> > +// Bug 969428
> > +// TODO: Remove this function and use the forms library
> 
> Can we block this bug on bug 969428 rather than come back to it?

We could.

Just bear in mind that we still have no Metro tests landed. We keep pushing them back to relegate functionality to specific libraries. And we wanted Metro running in CI by the 12th Feb

Comment 23

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

(In reply to Andrei Eftimie from comment #22)
> (In reply to Dave Hunt (:davehunt) from comment #21)
> Just bear in mind that we still have no Metro tests landed. We keep pushing
> them back to relegate functionality to specific libraries. And we wanted
> Metro running in CI by the 12th Feb

What's blocking bug 969428? Why would putting these methods in a more appropriate place delay landing the test? It doesn't need to be a complete library to serve all future tests, it could just serve this test initially and grow as needed. I don't see why we should add a TODO that can be taken care of immediately.

Comment 24

[Andrei Eftimie]

(In reply to Dave Hunt (:davehunt) from comment #23)
> It doesn't need to be a complete
> library to serve all future tests, it could just serve this test initially
> and grow as needed.

I strongly agree with this sentiment.
But this has not been the case yet in mozmill-tests in my experience so far.
A library will not get landed unless it's _complete_

Comment 25

[Daniel Gherasim]

(In reply to Dave Hunt (:davehunt) from comment #21)
> @@ +56,5 @@
> > +  // Go back to the test page
> > +  controller.open(TEST_DATA);
> > +  controller.waitForPageLoad();
> > +
> > +  autoFillPopup.open(function() {
> 
> I find these open/close methods to be counter-intuitive. I would prefer an
> API that more closely resembled the user's actions, such as
> autoFillPopup.sendKeys, which waited for the appropriate transition, and
> autoFillPopup.results array that can then be used to tap the first element.

We don't send the keys to the "autoFillPopup", we send to an <input> field. This will automatically trigger the popup to open. Also, when opening and closing the popup the same event is triggered, so having this code repeated often in the library (for open) and in the test (for close) will blow up our code.

You're right about the intuitive part, the open method doesn't actually opens the popup, it runs some code (in aCallback) that will trigger the event to open the popup, so maybe waitForOpen(aCallback) and waitForClose(aCallback) would be better ?

Comment 26

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

(In reply to Andrei Eftimie from comment #24)
> (In reply to Dave Hunt (:davehunt) from comment #23)
> > It doesn't need to be a complete
> > library to serve all future tests, it could just serve this test initially
> > and grow as needed.
> 
> I strongly agree with this sentiment.
> But this has not been the case yet in mozmill-tests in my experience so far.
> A library will not get landed unless it's _complete_

How do you define complete? I don't see an advantage including anything before it's required by a test. Henrik will be back on Monday. In the meantime there were some other comments that should be addressed.

Comment 27

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

(In reply to daniel.gherasim from comment #25)
> (In reply to Dave Hunt (:davehunt) from comment #21)
> You're right about the intuitive part, the open method doesn't actually
> opens the popup, it runs some code (in aCallback) that will trigger the
> event to open the popup, so maybe waitForOpen(aCallback) and
> waitForClose(aCallback) would be better ?

I do prefer this naming scheme.

Comment 28

[Daniel Gherasim]

Attachment: metro_testBasicFormCompletion_v5.4.patch

Did the requested changes and let the TO DO until we clarify what way we will go through with this.

Comment 29

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

Comment on attachment 8384482 [details] [diff] [review]
metro_testBasicFormCompletion_v5.4.patch

Review of attachment 8384482 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/lib/ui/forms.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I'm not that happy with forms.js given that auto-complete can also be used in chrome without a form. I think we might have to name the file popups.js? Then we can have it contain several kind of different popups. Also shouldn't this be in the base lib folder?

@@ +8,5 @@
> +
> +/**
> + * Constructor
> + */
> +function AutoFillPopup(aController) {

This misses the jsdoc parameters. Also the @constructor line.

@@ +14,5 @@
> +}
> +
> +/**
> + * Autofill popup class
> + */

I think you can remove that and better enhance the jsdoc comment for the constructor so it explains what this class is used for.

@@ +73,5 @@
> +        elem = new findElement.ID(this._controller.window.document, "autofill-container");
> +        break;
> +      case "results":
> +        var container = this.getElement({type: "container"}).getNode();
> +        elem = new elementslib.Elem(container.querySelector("richlistbox"));

Do we really need the richlistbox element? I don't think so. The 'results' element should actually return a list of results. Right now this is unclear.

@@ +76,5 @@
> +        var container = this.getElement({type: "container"}).getNode();
> +        elem = new elementslib.Elem(container.querySelector("richlistbox"));
> +        break;
> +      case "result":
> +        var value = aSpec.value || 0;

Defaults have to be defined at the beginning of the method.

@@ +78,5 @@
> +        break;
> +      case "result":
> +        var value = aSpec.value || 0;
> +        elem = new elementslib.Elem(this.getElement({type: "results"}).
> +                                    getNode().getItemAtIndex(value));

Can we use the nodeCollector here to retrieve the lines as an array similar to the autocomplete class for Firefox desktop?

@@ +93,5 @@
> +   *
> +   * @params {function} aCallbackOpen
> +   *         Function that triggers the popup to open
> +   */
> +  waitForOpen : function autoFillPopup_waitForOpen(aCallbackOpen) {

Those are not waitFor methods but trigger real actions. So call those open() and close(). Once we have bug 808586 implemented it would be easier to handle and separate.

@@ +134,5 @@
> +      }, aMessage);
> +    }
> +    finally {
> +      popAutoFillContainer.getNode().removeEventListener("transitionend", onTransitionEnd);
> +    }

This code is not very stable and can fail due to it doesn't know about the current state of the popup. It would be better if the wrapper methods would check for that first.

::: metrofirefox/tests/functional/testFormManager/testBasicFormCompletion.js
@@ +8,5 @@
> +
> +// Include the required modules
> +var { assert } = require("../../../../lib/assertions");
> +var tabs = require("../../../lib/ui/tabs");
> +var forms = require("../../../lib/ui/forms");

Keep the alphabetic order please.

@@ +18,5 @@
> +const INPUT_TEXT = "John";
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.tabs = new tabs.TabBrowser(aModule.controller);

This will overwrite the globally imported tabs module. You don't wanna do that.

@@ +21,5 @@
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.tabs = new tabs.TabBrowser(aModule.controller);
> +  aModule.autoFillPopup = new forms.AutoFillPopup(aModule.controller);
> +
> +  tabs.closeAllTabs(aModule.controller);

Why do you need the controller as parameter here?

@@ +40,5 @@
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +
> +  var inputField = new elementslib.ID(controller.tabs.activeTab, "ship_fname");
> +  assert.ok(inputField.exists(), "Name field has been found");

I don't think that we need this assert. sendKeys already checks that, right?

@@ +48,5 @@
> +                                        "SubmitButton");
> +  submitButton.tap();
> +  controller.waitForPageLoad();
> +
> +  // Open any page to see if form data is saved when reaccessing the test page

'Open another page' and 'accessing the test page again'

@@ +56,5 @@
> +  // Go back to the test page
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +
> +  autoFillPopup.waitForOpen(function() {

missing blank after function

@@ +60,5 @@
> +  autoFillPopup.waitForOpen(function() {
> +    inputField.sendKeys(INPUT_TEXT.substring(0, 2));
> +  });
> +
> +  autoFillPopup.waitForClose(function() {

same here.

@@ +84,5 @@
> +  // Wait for the clear to end
> +  assert.waitFor(function() {
> +    return clearHistoryEnd;
> +  }, "Form history has been cleaned");
> +}

This is part of the library, so please remove it now and finish the the lib instead. It's nothing which should be part of the test.

Comment 30

[Daniel Gherasim]

Attachment: metro_testBasicFormCompletion_v5.6.patch

(In reply to Henrik Skupin (:whimboo) from comment #29)
> I'm not that happy with forms.js given that auto-complete can also be used
> in chrome without a form. I think we might have to name the file popups.js?
> Then we can have it contain several kind of different popups. Also shouldn't
> this be in the base lib folder?
> 

Do you mean in metrofirefox/lib ? Because it's specific to metro.

> @@ +73,5 @@
> > +        elem = new findElement.ID(this._controller.window.document, "autofill-container");
> > +        break;
> > +      case "results":
> > +        var container = this.getElement({type: "container"}).getNode();
> > +        elem = new elementslib.Elem(container.querySelector("richlistbox"));
> 
> Do we really need the richlistbox element? I don't think so. The 'results'
> element should actually return a list of results. Right now this is unclear.
> 

Got this from the firefox desktop autocomplete library. 
Modified it as you asked.

> @@ +78,5 @@
> > +        break;
> > +      case "result":
> > +        var value = aSpec.value || 0;
> > +        elem = new elementslib.Elem(this.getElement({type: "results"}).
> > +                                    getNode().getItemAtIndex(value));
> 
> Can we use the nodeCollector here to retrieve the lines as an array similar
> to the autocomplete class for Firefox desktop?
> 

I actually got this from the autocomplete class for Firefox desktop.
I changed it now to use nodeCollector and retrieve the element at that index from the nodeCollector.elements.

Comment 31

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

(In reply to daniel.gherasim from comment #30)
> > I'm not that happy with forms.js given that auto-complete can also be used
> > in chrome without a form. I think we might have to name the file popups.js?
> > Then we can have it contain several kind of different popups. Also shouldn't
> > this be in the base lib folder?
> 
> Do you mean in metrofirefox/lib ? Because it's specific to metro.

If that only applies to Metro for the moment it would be the right location, yes. Just a note, if there are still open questions its better to wait for answers and not immediately upload new patches and set them also for review.

> > Do we really need the richlistbox element? I don't think so. The 'results'
> > element should actually return a list of results. Right now this is unclear.
> > 
> 
> Got this from the firefox desktop autocomplete library. 
> Modified it as you asked.

Heh, I would have loved to see a discussion first. Not sure if that was the right action happened here. You should have waited with the update and comment first with your findings. Not everything what I talk about should be promptly implemented. :)

Comment 32

[Andrei Eftimie]

Comment on attachment 8386603 [details] [diff] [review]
metro_testBasicFormCompletion_v5.6.patch

Review of attachment 8386603 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/lib/ui/popups.js
@@ +58,5 @@
> +   * @param {Object} aSpec.type
> +   *        General type information
> +   * @param {Object} [aSpec.subtype]
> +   *        Specific element or property
> +   * @param {Object} [aSpec.value]

nit: these should also have the default value set as the above `getElement` function.

@@ +67,5 @@
> +   * @returns {Object[]} Array of elements which have been found
> +   */
> +  getElements : function autoFillPopup_getElements(aSpec) {
> +    var elem = null;
> +    var type = aSpec.type || "";

You will need to have a default empty object set first. If aSpec is undefined you'll have a ReferenceError here.

@@ +103,5 @@
> +   *         Function that triggers the popup to open
> +   */
> +  open : function autoFillPopup_waitForOpen(aCallbackOpen) {
> +    var popupContainer = this.getElement({type: "container"});
> +    if(!!popupContainer.getNode().getAttribute("showing") !== true)

This will not work correctly. You are getting a string back.
> !!"" // false
> !!undefined // false
> !!"false" // true

Also please always wrap if clauses with brackets.

===

Also I think we should create a new method `isOpen` than we can use both here and in the `close` method.

But I'm not sure about the logic here. We should _always_ run the callback function.
And throw if something unexpected is happening (and we already do that in `_openClosePopupEvents`)

@@ +115,5 @@
> +   *         Function that triggers the popup to open
> +   */
> +  close : function autoFillPopup_waitForClose(aCallbackClose) {
> +    var popupContainer = this.getElement({type: "container"});
> +    if(!!popupContainer.getNode().getAttribute("hidden") !== true)

Same as above.

::: metrofirefox/tests/functional/testFormManager/testBasicFormCompletion.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +Cu.import("resource://gre/modules/FormHistory.jsm");

This is not used anymore in this file directly.

@@ +23,5 @@
> +  aModule.tabBrowser = new tabs.TabBrowser(aModule.controller);
> +  aModule.autoFillPopup = new popups.AutoFillPopup(aModule.controller);
> +
> +  tabBrowser.closeAllTabs();
> +

I'd remove this empty line.

@@ +29,5 @@
> +}
> +
> +function teardownModule(aModule) {
> +  tabBrowser.closeAllTabs();
> +

As well as this one.

Comment 33

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #31)
> (In reply to daniel.gherasim from comment #30)
> > > I'm not that happy with forms.js given that auto-complete can also be used
> > > in chrome without a form. I think we might have to name the file popups.js?
> > > Then we can have it contain several kind of different popups. Also shouldn't
> > > this be in the base lib folder?
> > 
> > Do you mean in metrofirefox/lib ? Because it's specific to metro.
> 
> If that only applies to Metro for the moment it would be the right location,
> yes. Just a note, if there are still open questions its better to wait for
> answers and not immediately upload new patches and set them also for review.
> 

I still don't understand why do we need to move it from metrofirefox/lib/ui tu metrofirefox/lib ?
Isn't it a UI library too ?

> > > Do we really need the richlistbox element? I don't think so. The 'results'
> > > element should actually return a list of results. Right now this is unclear.
> > > 
> > 
> > Got this from the firefox desktop autocomplete library. 
> > Modified it as you asked.
> 
> Heh, I would have loved to see a discussion first. Not sure if that was the
> right action happened here. You should have waited with the update and
> comment first with your findings. Not everything what I talk about should be
> promptly implemented. :)

I strongly believed that you were right about this so that's why I updated it.
The normal logic here is that "results" should return a list of elements (as the function name says: getElements), and not an element that contains a list, and you specified once that we don't have to copy code but to improve it. 
I would suggest to keep this solution if that's ok for you.

Comment 34

[Daniel Gherasim]

(In reply to Andrei Eftimie from comment #32)
> > +  open : function autoFillPopup_waitForOpen(aCallbackOpen) {
> > +    var popupContainer = this.getElement({type: "container"});
> > +    if(!!popupContainer.getNode().getAttribute("showing") !== true)
> > +      this._openClosePopupEvents(aCallbackOpen, "Autofill popoup has been opened");
> But I'm not sure about the logic here. We should _always_ run the callback
> function.
> And throw if something unexpected is happening (and we already do that in
> `_openClosePopupEvents`)

Yep, that's why I dind't check for it if it's opened from the first time.
One option I suggest is this:
// for opening: 
 if(!isOpen) {
    // run the callback & check for transition effect
    // _openClosePopupEvents(aCallbackOpen, "Autofill popoup has been opened");
 }
 else
   // just run the callback
   // aCallbackOpen()

Comment 35

[Andrei Eftimie]

(In reply to daniel.gherasim from comment #34)
> (In reply to Andrei Eftimie from comment #32)
> > > +  open : function autoFillPopup_waitForOpen(aCallbackOpen) {
> > > +    var popupContainer = this.getElement({type: "container"});
> > > +    if(!!popupContainer.getNode().getAttribute("showing") !== true)
> > > +      this._openClosePopupEvents(aCallbackOpen, "Autofill popoup has been opened");
> > But I'm not sure about the logic here. We should _always_ run the callback
> > function.
> > And throw if something unexpected is happening (and we already do that in
> > `_openClosePopupEvents`)
> 
> Yep, that's why I dind't check for it if it's opened from the first time.
> One option I suggest is this:
> // for opening: 
>  if(!isOpen) {
>     // run the callback & check for transition effect
>     // _openClosePopupEvents(aCallbackOpen, "Autofill popoup has been
> opened");
>  }
>  else
>    // just run the callback
>    // aCallbackOpen()

I'm not sure. Since we do call this method `open` we expect the code that runs it to really open the autocomplete popup. If it's already open we _should_ throw an error since we're in an unexpected state.

With what you are suggesting our code will run and we will not be noticed when something unexpected happens.

Comment 36

[Daniel Gherasim]

Attachment: metro_testBasicFormCompletion_v5.7.patch

Updated the patch now with the latest requests.

Comment 37

[Andrei Eftimie]

Comment on attachment 8387483 [details] [diff] [review]
metro_testBasicFormCompletion_v5.7.patch

Review of attachment 8387483 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few items, otherwise it looks good.
Thanks.

::: metrofirefox/lib/popups.js
@@ +94,5 @@
> +    }
> +
> +    return nodeCollector.elements;
> +  },
> +

Short jsdoc comment.

@@ +97,5 @@
> +  },
> +
> +  isOpen : function autoFillPopup_isOpen() {
> +    var popupContainer = this.getElement({type: "container"});
> +    if(!popupContainer.getNode().hidden) {

You can simply do:
> return !popupContainer.getNode().hidden

@@ +124,5 @@
> +   * @params {function} aCallbackClose
> +   *         Function that triggers the popup to open
> +   */
> +  close : function autoFillPopup_waitForClose(aCallbackClose) {
> +    if(this.isOpen())

Please have curly brackets even for 1 line if clauses.

Comment 38

[Daniel Gherasim]

Attachment: metro_testBasicFormCompletion_v5.8.patch

Updated.

Comment 39

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8388480 [details] [diff] [review]
metro_testBasicFormCompletion_v5.8.patch

Review of attachment 8388480 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 40

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

Comment on attachment 8388480 [details] [diff] [review]
metro_testBasicFormCompletion_v5.8.patch

Review of attachment 8388480 [details] [diff] [review]:
-----------------------------------------------------------------

Nearly done. Just some updates as you can see described inline. Once that is fixed you don't have to ask for review from me again.

::: metrofirefox/lib/popups.js
@@ +7,5 @@
> +var { assert } = require("../../lib/assertions");
> +var domUtils = require("../../lib/dom-utils");
> +
> +/**
> + * Constructor

Please re-read my last comment on that. This has to be '@constructor' and should be located right above @param.

@@ +83,5 @@
> +        nodeCollector.queryNodes("#autofill-popup");
> +        break;
> +      case "results":
> +        nodeCollector.root = this.getElement({type: "popup"}).getNode();
> +        nodeCollector.queryNodes("richlistitem");

Nice! That looks way better.

@@ +87,5 @@
> +        nodeCollector.queryNodes("richlistitem");
> +        break;
> +      case "result":
> +        var results = this.getElements({type: "results"});
> +        return [results[value]];

You can kill this case. Instead do `this.getElements({type: "results"})[index]` in your test code.

@@ +128,5 @@
> +   * @params {function} aCallbackClose
> +   *         Function that triggers the popup to open
> +   */
> +  close : function autoFillPopup_waitForClose(aCallbackClose) {
> +    if(this.isOpen()) {

nit: missing speace after if.

@@ +159,5 @@
> +    try {
> +      aCallback();
> +      assert.waitFor(function() {
> +        return transitionend;
> +      }, aMessage);

I would use an arrow function here: "() => transitioned"

Comment 41

[Daniel Gherasim]

Attachment: metro_testBasicFormCompletion_v6.patch

Patch is ready, but we still need the patch for bug 969428 to be landed before.

Comment 42

[Andrei Eftimie]

Comment on attachment 8391080 [details] [diff] [review]
metro_testBasicFormCompletion_v6.patch

Review of attachment 8391080 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
As you've said, we'll need bug 969428 to be able to run this test.

Comment 43

[Andrei Eftimie]

Comment on attachment 8391080 [details] [diff] [review]
metro_testBasicFormCompletion_v6.patch

The dependency has been fixed.

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/2ea1e3fd97d7 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/a0a80bb7c863 (mozilla-aurora)