587819 - DOM walker for A11y and L10n tests

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Adrian Kalla [:adriank]]

This bug should cover the implementation of a common shared module for accessibility and localization tests.

Comment 1

[Adrian Kalla [:adriank]]

Attachment: DOMWalker v1

mostly finished DOMWalker.

What's left is to remove some of the sleep-calls.
Looking at other Mozmill preferences tests, it's probably unavoidable to have sleep calls for opening prefpanes.

Comment 2

[Adrian Kalla [:adriank]]

ah, forgot. Modal dialog support is missing at the moment, so it won't work on Windows in combination with the pref-tests

Comment 3

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

Comment on attachment 479049 [details] [diff] [review]
DOMWalker v1

Here some comments in short term, which we talked about in our meeting today.

>var MODULE_NAME = 'DOMUtilitiesAPI';

Should be integrated into the DOMUtilsAPI, now that this module has been landed.

>var jumlib = {}; 
>Components.utils.import("resource://mozmill/modules/jum.js", jumlib);

That's not needed in the API.

>const FILTER_ACCEPT = 1;
>const FILTER_REJECT = 2;
>const FILTER_SKIP = 3;

Please move the constants into the class, so they can be accessed as static members.

>function DOMWalker(controller)

Please use the class as wrapper and define an internal function for the recursive usage. Most of the parameters for the wrapper can be already set by the constructor.

>        if (idSet.target == "dropdown") {

This shouldn't operate on a value from an user defined list but on the nodes element name. This allows the maximum of flexibility. To identify a special object use attribute and value in the ids object. 

>          var nodeToClickOn = cont.window.document.getElementById(idSet.id);
>          
>          if (nodeToClickOn.label != idSet.title) {
[..]
>            var root = cont.window.document.documentElement;
>            
>            this.walk(cont, root, callback, idSet.subContent, true);
>          } else if (nodeToClickOn.selected && idSet.subContent.length > 0) {
>            this.openNew(cont, callback, idSet.subContent);
>          }

The decision how to handle the target element (window, prefpane...) should be separated out of the if condition. A dropdown could also open a new window.


>    for (var i = 0; i < nodes.length; i++) {

Instead of "for" you could use Array.forEach() which makes it a bit better readable.

>        if (result != undefined && result.length > 0) {
>          results = results.concat(result);

Array.concat doesn't handle empty arrays itself? I would tend to say we do not need the if clause here. Also name the variable so it reflects it is an array.

>        result = this.walk(cont, nodes[i], callback, ids, false);

Do you always have to pass in the whole ids object? I don't think so. Just use the currently processed entry, now that we have a hierarchy.

>        if (result != undefined && result.length > 0) {
>          results = results.concat(result)

*snip*

>      } else if (nodeStatus == FILTER_SKIP) {
>        result = this.walk(cont, nodes[i], callback, ids, false);
>        if (result != undefined && result.length > 0) {
>          results = results.concat(result)

Move the call to walk and concat out of the if clauses. All conditions are using it.

>    // Go here only, if not launched by a self-recursion
>    if (main) {
>      if (resultsCheck != null) {
>        resultsCheck(results, cont);
>      }
>        
>      if (ids != null) {
>        this.openNew(cont, callback, ids)
>      }
>    }

We should be able to remove it by using a wrapper.

Comment 4

[Adrian Kalla [:adriank]]

(In reply to comment #3)
> >var MODULE_NAME = 'DOMUtilitiesAPI';
> 
> Should be integrated into the DOMUtilsAPI, now that this module has been
> landed.

ok

> >var jumlib = {}; 
> >Components.utils.import("resource://mozmill/modules/jum.js", jumlib);
> 
> That's not needed in the API.

right

> >const FILTER_ACCEPT = 1;
> >const FILTER_REJECT = 2;
> >const FILTER_SKIP = 3;
> 
> Please move the constants into the class, so they can be accessed as static
> members.

ok

> >function DOMWalker(controller)
> 
> Please use the class as wrapper and define an internal function for the
> recursive usage. Most of the parameters for the wrapper can be already set by
> the constructor.

ok

> >        if (idSet.target == "dropdown") {
> 
> This shouldn't operate on a value from an user defined list but on the nodes
> element name. This allows the maximum of flexibility. To identify a special
> object use attribute and value in the ids object. 
> 
> >          var nodeToClickOn = cont.window.document.getElementById(idSet.id);
> >          
> >          if (nodeToClickOn.label != idSet.title) {
> [..]
> >            var root = cont.window.document.documentElement;
> >            
> >            this.walk(cont, root, callback, idSet.subContent, true);
> >          } else if (nodeToClickOn.selected && idSet.subContent.length > 0) {
> >            this.openNew(cont, callback, idSet.subContent);
> >          }
> 
> The decision how to handle the target element (window, prefpane...) should be
> separated out of the if condition. A dropdown could also open a new window.

A problem that I see here ,is handling modal windows, as I have to activate the modaldialog watcher before I do anything. So I don't see a possibility to don't check the target before doing anything.

> >    for (var i = 0; i < nodes.length; i++) {
> 
> Instead of "for" you could use Array.forEach() which makes it a bit better
> readable.

Imho introducing another callback of a callback will make it much less readable than it is now, especially, because I'd have to pass all the parameters to the callback method.
 
> >        if (result != undefined && result.length > 0) {
> >          results = results.concat(result);
> 
> Array.concat doesn't handle empty arrays itself? I would tend to say we do not
> need the if clause here. Also name the variable so it reflects it is an array.

results -> resultsArray && result -> resultArray 

> >        result = this.walk(cont, nodes[i], callback, ids, false);
> 
> Do you always have to pass in the whole ids object? I don't think so. Just use
> the currently processed entry, now that we have a hierarchy.

After introducing the wrapper method, ids in walk() will be obsolete anyway.

> >      } else if (nodeStatus == FILTER_SKIP) {
> >        result = this.walk(cont, nodes[i], callback, ids, false);
> >        if (result != undefined && result.length > 0) {
> >          results = results.concat(result)
> 
> Move the call to walk and concat out of the if clauses. All conditions are
> using it.

No, not all. There is no else-clause. FILTER_REJECT's are ommited that way, because their children have to be completely ignored.

> >    // Go here only, if not launched by a self-recursion
> >    if (main) {
> >      if (resultsCheck != null) {
> >        resultsCheck(results, cont);
> >      }
> >        
> >      if (ids != null) {
> >        this.openNew(cont, callback, ids)
> >      }
> >    }
> 
> We should be able to remove it by using a wrapper.

Yep, will be done.

Comment 5

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

(In reply to comment #4)
> > The decision how to handle the target element (window, prefpane...) should be
> > separated out of the if condition. A dropdown could also open a new window.
> 
> A problem that I see here ,is handling modal windows, as I have to activate the
> modaldialog watcher before I do anything. So I don't see a possibility to don't
> check the target before doing anything.

You know the target from the ids list right at the beginning of the function. So in case of a modal window, you will have to start the modal window watcher. After that handle the specific node and trigger the action.

> Imho introducing another callback of a callback will make it much less readable
> than it is now, especially, because I'd have to pass all the parameters to the
> callback method.

Huh, why that? It's just:

nodes.forEach(function(node) {
  var status = filter(node);
[..]
      
  } else if (status == FILTER_SKIP) {
    result = this.walk(cont, node, callback, ids, false);
    if (result != undefined && result.length > 0) {
      results = results.concat(result)
    }
  }
}, this);

IMO it's still much better readable.

> > >        if (result != undefined && result.length > 0) {
> > >          results = results.concat(result);
> > 
> > Array.concat doesn't handle empty arrays itself? I would tend to say we do not
> > need the if clause here. Also name the variable so it reflects it is an array.
> 
> results -> resultsArray && result -> resultArray 

I can't get what you mean with this line.

> > Do you always have to pass in the whole ids object? I don't think so. Just use
> > the currently processed entry, now that we have a hierarchy.
>
> After introducing the wrapper method, ids in walk() will be obsolete anyway.

Only for the node collector. But when you spawn a new instance of the DOM walker you will still have to pass in a new array with whitelisted nodes. 

> > >      } else if (nodeStatus == FILTER_SKIP) {
> > >        result = this.walk(cont, nodes[i], callback, ids, false);
> > >        if (result != undefined && result.length > 0) {
> > >          results = results.concat(result)
> > 
> > Move the call to walk and concat out of the if clauses. All conditions are
> > using it.
> 
> No, not all. There is no else-clause. FILTER_REJECT's are ommited that way,
> because their children have to be completely ignored.

Declare result as an empty array before the if block. If nothing gets pushed into the result array, the final concat will not add any entries to the results array.

Comment 6

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

Any updates Adrian? Would be great to have the next review as soon as possible.

Comment 7

[Adrian Kalla [:adriank]]

Attachment: DOMWalker v2

(hopefully) addressed all review comments as well as comments from last meeting

Comment 8

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

Comment on attachment 480416 [details] [diff] [review]
DOMWalker v2

While reviewing the code I will also make notes about styles now, which I haven't done the last times...

> var MODULE_NAME = 'DOMUtilsAPI';
> 
>+var RELATIVE_ROOT = '.';
>+var MODULE_REQUIRES = ['ModalDialogAPI', 'UtilsAPI'];
>+
>+const gDelay = 1000;
>+const gTimeout = 5000;

All of those lines have to be constants and only have capital letters.

>+ * @param {Function} callbackFilter
>+ *        callback-method to filter nodes
>+ * @param {Function} callbackNodeTest
>+ *        callback-method to test accepted nodes
>+ * @param {Function} callbackResults
>+ *        callback-method to process the results

Please mark optional arguments as such. See the new AddonsAPI in the default branch how to do that.

>+function DOMWalker(controller, callbackFilter, callbackNodeTest,
>+                   callbackResults)
>+{

nit: Don't put the bracket in its own line.

>+  this.FILTER_ACCEPT = 1;
>+  this.FILTER_REJECT = 2;
>+  this.FILTER_SKIP = 3;

These have to be static members. That means don't set them inside the constructor. You will need access to them without having to create a new instance.

>+  this.CURRENT_WINDOW = "currentWindow";
>+  this.NEW_WINDOW = "newWindow";

Same here and those entries will need a prefix. Just flip the parts. What about non-modal and modal new windows?

>+  this._callbackFilter = callbackFilter;
>+  this._callbackNodeTest = callbackNodeTest;
>+  this._callbackResults = callbackResults;
>+  this._controller = controller;

nit: please use the order as given by the parameter list

>+  this._modalDialogAPI = collector.getModule('ModalDialogAPI');
>+  this._utilsAPI = collector.getModule('UtilsAPI');

nit: capitalization

>+  set callbackFilter(callback) {
>+    this._callbackFilter = callback;
>+  },

Do we have needs to re-set the callback filters after a DOMWalker has been instantiated?

>+  /**
>+   * Sets the MozMill controller
>+   */
>+  set controller(controller) {
>+    this._controller = controller;
>+  },

Same here. I don't think that we will support this setter.

>+  /**
>+   * The main DOMWalker function.
>+   *
>+   * @param [ids] Array contating informations on the elements to open while

I miss the type of the parameter.

>+   * @param {Node} root (optional) Node to start testing from

When it is optional, what's the default value?

>+   * @returns [results] An array with gathered all results from testing
>+   *  a given element

Use @returns and @type to exactly specify what this function returns.

>+  walk : function DOMWalker_walkWrapper(ids, root) {

You forgot to update the internal function name.

>+    let callbackResults = this._callbackResults;

Why you need to do that?

>+    var resultsArray = this._walk(root);

Those are 'collectedNodes', aren't they?

>+    if (!(callbackResults == undefined || callbackResults == null)) {

This should only test for type of function.

>+      callbackResults(resultsArray, this._controller);

We should use the controller always as the first parameter.

>+    if (!(ids == undefined || ids == null)) {

Why you don't simply test for existence?

>+  /**
>+   * Main entry point to open new elements like windows, tabpanels, prefpanes,
>+   * dialogs
>+   * 
>+   * @param [ids] IDs-array with the informations needed to open the
>+   * windows/dialogs 

Missing parameter type.

>+  _openNew : function DOMWalker_openNew(ids) {

This function should really be named like '_prepareTargetWindow' or such. 

>+    for (var i = 0; i < ids.length; i++) {
>+      var switchingElement = doc.getElementById(ids[i].id);

Lately I was working on the nodeCollector class (http://hg.mozilla.org/qa/mozmill-tests/diff/default/shared-modules/testDOMUtilsAPI.js) which make use of the querySelector. Use this class so we can also retrieve nodes not only by id but by any class and attribute.

Also both ways will return a DOM node, which should be named as such. 'Element' should only be used for elementslib instances.

>+      if (switchingElement != undefined && switchingElement != null) {

You will always have an object instance or null. So a simply 'if (node)' should be sufficient.

>+        /**
>+         * Decide if what we want to open is a new window or if it will be
>+         * opened in the current window.
>+         */

Please use block level comments only on function headers.

>+        if (idSet.target == this.CURRENT_WINDOW) {

As said above, access to the constant has to happen not via this current instance.

>+        } else if (idSet.target == this.NEW_WINDOW) {
>+          // Modal windows have to be able to access that informations
>+          var modalInfos = {ids : idSet.subContent,
>+                            callbackFilter :  this._callbackFilter,
>+                            callbackNodeTest : this._callbackNodeTest,
>+                            callbackResults : this._callbackResults}
>+          persisted.modalInfos = modalInfos;

You should initialize this object once in the constructor and simply update the ids member when necessary. Think about storing the callbacks directly in there. No need to carry two references around. Also please check the indentation for objects. Therefore see the AddonsAPI. In general I would suggest 'persisted.callbacks = { ... }' and 'persisted.nodeList'. 

>+          // Modal windows are used only on Windows
>+          if (mozmill.isWindows) {
>+            var md = new this._modalDialogAPI.modalDialog(this._windowHelper);
>+            md.start(200);
>+          }

That's not correct. Also on Linux we have modal dialogs. Also don't create a modal dialog instance for all new windows. There is no way to get rid of the observer for now. There should really be a separate constant for it. Currently it seems that the DOMWalker is still bound a bit too much to the preferences window.

>+          // On non-Windows systems we can manage the new window more easily
>+          if (!mozmill.isWindows) {
>+            var wCont = this._utilsAPI.handleWindow('title', idSet.title,
>+                                                    false, true);
>+            this._windowHelper(wCont);

Same here. Also on Windows we have non-modal windows.

>+   * @param {Node} switchingElement Node that holds the information which way 
>+   * to open the new window/dialog

I would prefer a name like activeNode.

>+  _switchElement : function DOMWalker_switchElement(switchingElement, idSet) {

Can you please rename '_switchElement' to '_processNode', because that's really what happens here, right?

>+    /**
>+     * Opens a new window/dialog through a menulist and runs DOMWalker.walk()
>+     * for it.
>+     * 
>+     * If the wanted window/dialog is already selected, just run this function
>+     * recursively for it's descendants.
>+     */

Block comments only for function headers.

>+    if (switchingElement.localName == "menulist") {
>+      var nodeToClickOn = doc.getElementById(idSet.id);
>+      if (nodeToClickOn.label != idSet.title) {

As talked we should default to the value and not the label. I would still prefer to see it customizable, so it will work for any possible property/attribute. Call the variable dropDownNode...

>+        var dropDown = new elementslib.ID(doc, idSet.id);
>+        this._controller.waitForElement(dropDown, gTimeout);

Use elementslib.Elem with the element you already have retrieved above.

>+        // controller.select doesn't work with our filters on non-Windows OS's
>+        // No idea why, so lets do it manually for now
>+        if (!mozmill.isWindows) {
>+          for (j = 0; j < idSet.title.length; j++) {
>+            this._controller.keypress(dropDown, idSet.title[j], {});
>+            this._controller.sleep(100);
>+          };
>+          this._controller.keypress(dropDown, "VK_RETURN", {});
>+        } else {

I don't see a problem with our existing tests for the pref pane. Can you please elaborate this a bit more detailed? You have a simple minimized test?

>+          this._controller.select(dropDown, null, idSet.title);
>+          this._controller.waitFor(function()
>+                                   {
>+                                      return nodeToClickOn.label == idSet.title;
>+                                   }

There is no need for wait for the selected element. It's more important to wait for the target to be active or selected. 

>+        // No known generic way to check, if a panes *content* did load
>+        this._controller.sleep(gTimeout);
>
>+        if (idSet.target == this.CURRENT_WINDOW)
>+          this.walk(idSet.subContent);

Huh? You should really differentiate between different node targets. Not all nodes with the type menulist will change a pane in the current window. And using a timeout is definitely the wrong way to wait for a selection or activation.

>+    else if (switchingElement.localName == "prefpane") {
>+      var prefDialog = idSet.dialog;

Please name the member .windowHandler. That's much clearer. 

>+      if (prefDialog.paneId != idSet.id) {
>+        prefDialog.paneId = idSet.id;
>+        
>+        // No known generic way to check, if a panes *content* did load
>+        this._controller.sleep(gDelay);

in 'set paneId' we wait until the new pane has been selected. So the content should also be ready. Why we need to wait?

>+        if (idSet.target == this.CURRENT_WINDOW)
>+          this.walk(idSet.subContent);

Can we make .target optional? In this case it will always be true.

>+    else if (switchingElement.localName == "tab") {
>+      var nodeToClickOn = doc.getElementById(idSet.id);
>+      
>+      if (nodeToClickOn.selected != true) {
>+        this._controller.click(new elementslib.ID(doc, idSet.id));
>+        this._controller.sleep(gDelay);

You already have the node above. Use elementslib.Elem. I would also like to see that we can get rid of the sleep call.

>+        if (idSet.target == this.CURRENT_WINDOW)
>+          this.walk(idSet.subContent);

snip, as above.

>+    else if (switchingElement.localName == "button") {
>+      this._controller.click(new elementslib.ID(doc, idSet.id));
>+      this._controller.sleep(gDelay);

snip, same comment for sleep.

>+      if (idSet.target == this.CURRENT_WINDOW)
>+        this.walk(idSet.subContent);

What about the case when a button opens a new window?

>+    }
>+    
>+    else {
>+      this._controller.assert(function() { return false; },
>+                              "Unknown node.localName: " +
>+                              switchingElement.localName);  
>+    }

Please fix the indentation for all lines.

>+   * @param {Node} root (optional) Node to start testing from

nit: See the AddonAPI how to format this correctly.

>+  _walk : function DOMWalker_walk(root) {
>+    if (root == undefined || root == null)
>+      root = this._controller.window.document.documentElement;
>+    var nodes = root.childNodes;

Just test for 'if (!root)'

>+    var resultsArray = [];

Name it testResults please.

>+    callbackFilter = this._callbackFilter;
>+    callbackNodeTest = this._callbackNodeTest;

why this is necessary?

>+    for (var i = 0; i < nodes.length; i++) {
[..]
>+      
>+      var resultArray = [];

Name it results. No need to say that we use an array.

>+        resultArray = this._walk(nodes[i]);
>+      } else if (nodeStatus == this.FILTER_SKIP) {
>+        resultArray = this._walk(nodes[i]);
>+      }
>+      resultsArray = resultsArray.concat(resultArray);
>+    }

A switch condition would be preferred. Only use the break statement after FILTER_SKIP, so FILTER_ACCEPT can also use the FILTER_SKIP code.

Comment 9

[Adrian Kalla [:adriank]]

(In reply to comment #8)
> Comment on attachment 480416 [details] [diff] [review]
> DOMWalker v2
> 
> While reviewing the code I will also make notes about styles now, which I
> haven't done the last times...
> 
> > var MODULE_NAME = 'DOMUtilsAPI';
> > 
> >+var RELATIVE_ROOT = '.';
> >+var MODULE_REQUIRES = ['ModalDialogAPI', 'UtilsAPI'];
> >+
> >+const gDelay = 1000;
> >+const gTimeout = 5000;
> 
> All of those lines have to be constants and only have capital letters.

gDelay and gTimeout too?

> >+ * @param {Function} callbackFilter
> >+ *        callback-method to filter nodes
> >+ * @param {Function} callbackNodeTest
> >+ *        callback-method to test accepted nodes
> >+ * @param {Function} callbackResults
> >+ *        callback-method to process the results
> 
> Please mark optional arguments as such. See the new AddonsAPI in the default
> branch how to do that.

ok
 
> >+function DOMWalker(controller, callbackFilter, callbackNodeTest,
> >+                   callbackResults)
> >+{
> 
> nit: Don't put the bracket in its own line.

ok
 
> >+  this.FILTER_ACCEPT = 1;
> >+  this.FILTER_REJECT = 2;
> >+  this.FILTER_SKIP = 3;
> 
> These have to be static members. That means don't set them inside the
> constructor. You will need access to them without having to create a new
> instance.

ok

> >+  this.CURRENT_WINDOW = "currentWindow";
> >+  this.NEW_WINDOW = "newWindow";
> 
> Same here and those entries will need a prefix. Just flip the parts. What about
> non-modal and modal new windows?

see later below
 
> >+  this._callbackFilter = callbackFilter;
> >+  this._callbackNodeTest = callbackNodeTest;
> >+  this._callbackResults = callbackResults;
> >+  this._controller = controller;
> 
> nit: please use the order as given by the parameter list

ok

> >+  this._modalDialogAPI = collector.getModule('ModalDialogAPI');
> >+  this._utilsAPI = collector.getModule('UtilsAPI');
> 
> nit: capitalization
 
ok

> >+  set callbackFilter(callback) {
> >+    this._callbackFilter = callback;
> >+  },
> 
> Do we have needs to re-set the callback filters after a DOMWalker has been
> instantiated?

Not for my existing tests, but some tests may need to switch to another filter for some specific window parts. Because of that it may make sense to have that here.
 
> >+  /**
> >+   * Sets the MozMill controller
> >+   */
> >+  set controller(controller) {
> >+    this._controller = controller;
> >+  },
> 
> Same here. I don't think that we will support this setter.

Right, setting another controller is highly unlikely.
 
> >+  /**
> >+   * The main DOMWalker function.
> >+   *
> >+   * @param [ids] Array contating informations on the elements to open while
> 
> I miss the type of the parameter.

That's why "ids" is another brackets (array). But yeah, I think I did use that style somewhere else... I'll change that.
 
> >+   * @param {Node} root (optional) Node to start testing from
> 
> When it is optional, what's the default value?

ok, I'll put that into the comment...
 
> >+   * @returns [results] An array with gathered all results from testing
> >+   *  a given element
> 
> Use @returns and @type to exactly specify what this function returns.

ok

> >+  walk : function DOMWalker_walkWrapper(ids, root) {
> 
> You forgot to update the internal function name.
 
right

> >+    let callbackResults = this._callbackResults;
> 
> Why you need to do that?

No idea why, but doing "this._callbackResults.callbackResults()" always fails and "callbackResults.callbackResults()" works...

> >+    var resultsArray = this._walk(root);
> 
> Those are 'collectedNodes', aren't they?

No, the aren't. It depends on what the array returned by callbackNodeTest() contains.

> >+    if (!(callbackResults == undefined || callbackResults == null)) {
> 
> This should only test for type of function.

right

> >+      callbackResults(resultsArray, this._controller);
> 
> We should use the controller always as the first parameter.

ok

> >+    if (!(ids == undefined || ids == null)) {
> 
> Why you don't simply test for existence?

looks like because of copy&paste...

> >+  /**
> >+   * Main entry point to open new elements like windows, tabpanels, prefpanes,
> >+   * dialogs
> >+   * 
> >+   * @param [ids] IDs-array with the informations needed to open the
> >+   * windows/dialogs 
> 
> Missing parameter type.

ok

> >+  _openNew : function DOMWalker_openNew(ids) {
> 
> This function should really be named like '_prepareTargetWindow' or such. 

ok

> >+    for (var i = 0; i < ids.length; i++) {
> >+      var switchingElement = doc.getElementById(ids[i].id);
> 
> Lately I was working on the nodeCollector class
> (http://hg.mozilla.org/qa/mozmill-tests/diff/default/shared-modules/testDOMUtilsAPI.js)
> which make use of the querySelector. Use this class so we can also retrieve
> nodes not only by id but by any class and attribute.

I'd really like to move that to a later follow up - it would introduce too much noise at the moment.
 
> Also both ways will return a DOM node, which should be named as such. 'Element'
> should only be used for elementslib instances.

ok

> >+      if (switchingElement != undefined && switchingElement != null) {
> 
> You will always have an object instance or null. So a simply 'if (node)' should
> be sufficient.

right

> >+        /**
> >+         * Decide if what we want to open is a new window or if it will be
> >+         * opened in the current window.
> >+         */
> 
> Please use block level comments only on function headers.

ok

> >+        if (idSet.target == this.CURRENT_WINDOW) {
> 
> As said above, access to the constant has to happen not via this current
> instance.

ok

> >+        } else if (idSet.target == this.NEW_WINDOW) {
> >+          // Modal windows have to be able to access that informations
> >+          var modalInfos = {ids : idSet.subContent,
> >+                            callbackFilter :  this._callbackFilter,
> >+                            callbackNodeTest : this._callbackNodeTest,
> >+                            callbackResults : this._callbackResults}
> >+          persisted.modalInfos = modalInfos;
> 
> You should initialize this object once in the constructor and simply update the
> ids member when necessary. Think about storing the callbacks directly in there.
> No need to carry two references around. Also please check the indentation for
> objects. Therefore see the AddonsAPI. In general I would suggest
> 'persisted.callbacks = { ... }' and 'persisted.nodeList'.

Storing the ids in the constructor is a really bad idea, as we don't start a new DOMWalker instance for window-elements like prefpanes. If a recursion run changes the ids to it's ids.subContent, then other siblings will use later that subContent instead of the original ids array...

Beside that, all the callbacks are always in the constructor. The above code prepares the elements to be processed to _windowHelper, where a new DOMWalker instance is being created each time.

 
> >+          // Modal windows are used only on Windows
> >+          if (mozmill.isWindows) {
> >+            var md = new this._modalDialogAPI.modalDialog(this._windowHelper);
> >+            md.start(200);
> >+          }
> 
> That's not correct. Also on Linux we have modal dialogs. Also don't create a
> modal dialog instance for all new windows. There is no way to get rid of the
> observer for now. There should really be a separate constant for it. Currently
> it seems that the DOMWalker is still bound a bit too much to the preferences
> window.
> 
> >+          // On non-Windows systems we can manage the new window more easily
> >+          if (!mozmill.isWindows) {
> >+            var wCont = this._utilsAPI.handleWindow('title', idSet.title,
> >+                                                    false, true);
> >+            this._windowHelper(wCont);
> 
> Same here. Also on Windows we have non-modal windows.

ok, will add modalWindow too

> >+   * @param {Node} switchingElement Node that holds the information which way 
> >+   * to open the new window/dialog
> 
> I would prefer a name like activeNode.

ok

> >+  _switchElement : function DOMWalker_switchElement(switchingElement, idSet) {
> 
> Can you please rename '_switchElement' to '_processNode', because that's really
> what happens here, right?

Even more happens here -> the new window/dialog etc. is being opened here. So not sure if "processNode" is the right name... Maybe "switchDialog"?


> >+    /**
> >+     * Opens a new window/dialog through a menulist and runs DOMWalker.walk()
> >+     * for it.
> >+     * 
> >+     * If the wanted window/dialog is already selected, just run this function
> >+     * recursively for it's descendants.
> >+     */
> 
> Block comments only for function headers.

ok

> >+    if (switchingElement.localName == "menulist") {
> >+      var nodeToClickOn = doc.getElementById(idSet.id);
> >+      if (nodeToClickOn.label != idSet.title) {
> 
> As talked we should default to the value and not the label. I would still
> prefer to see it customizable, so it will work for any possible
> property/attribute. Call the variable dropDownNode...

As long as controller.select() does not work, I need the label.
 
> >+        var dropDown = new elementslib.ID(doc, idSet.id);
> >+        this._controller.waitForElement(dropDown, gTimeout);
> 
> Use elementslib.Elem with the element you already have retrieved above.

ok

> >+        // controller.select doesn't work with our filters on non-Windows OS's
> >+        // No idea why, so lets do it manually for now
> >+        if (!mozmill.isWindows) {
> >+          for (j = 0; j < idSet.title.length; j++) {
> >+            this._controller.keypress(dropDown, idSet.title[j], {});
> >+            this._controller.sleep(100);
> >+          };
> >+          this._controller.keypress(dropDown, "VK_RETURN", {});
> >+        } else {
> 
> I don't see a problem with our existing tests for the pref pane. Can you please
> elaborate this a bit more detailed? You have a simple minimized test?

I don't know why it happens - I spend a whole night on this problem and couldn't find a solution. As said, the below code works on Windows, but not on Mac.

I'll try to do a minimized test case later.
 
> >+          this._controller.select(dropDown, null, idSet.title);
> >+          this._controller.waitFor(function()
> >+                                   {
> >+                                      return nodeToClickOn.label == idSet.title;
> >+                                   }
> 
> There is no need for wait for the selected element. It's more important to wait
> for the target to be active or selected. 

Let me repeat the below comment: "No known generic way to check, if a panes *content* did load". It could mean *content* of anything too, like content of a part of a prefpane. If you know a way, please tell me it.
 
> >+        // No known generic way to check, if a panes *content* did load
> >+        this._controller.sleep(gTimeout);
> >
> >+        if (idSet.target == this.CURRENT_WINDOW)
> >+          this.walk(idSet.subContent);
> 
> Huh? You should really differentiate between different node targets. Not all
> nodes with the type menulist will change a pane in the current window.

That's *why* it is in an if clause. In case of new Windows the _windowHelper start's DOMWalker.walk() - it can't be done here.

> And
> using a timeout is definitely the wrong way to wait for a selection or
> activation.

Then tell me how. I couldn't find *any* generic element, attribute that would tell me, that the *content* did load.

> >+    else if (switchingElement.localName == "prefpane") {
> >+      var prefDialog = idSet.dialog;
> 
> Please name the member .windowHandler. That's much clearer. 

ok

> >+      if (prefDialog.paneId != idSet.id) {
> >+        prefDialog.paneId = idSet.id;
> >+        
> >+        // No known generic way to check, if a panes *content* did load
> >+        this._controller.sleep(gDelay);
> 
> in 'set paneId' we wait until the new pane has been selected. So the content
> should also be ready. Why we need to wait?

Yes, we are waiting there for the *selection*. But it does not say *anything* if all nodes were loaded *and* that the content is being displayed.
The content of most of the panes is ready around 900ms *after the pane has been marked as selected*. On my virtual machine, it takes up to 2000ms to load the content beside the "Applications pane" - that one needs up to 4500ms, so I think to extend the sleep call to 5 seconds.

I spend on finding a way to find out if everything was displayed more than too much time and didn't find a way beside sleep - calls. Even the existing Mozmill tests use sleep there: http://hg.mozilla.org/qa/mozmill-tests/file/62551d70805a/firefox/testPreferences/testSwitchPanes.js#l78 .
One thing is clear - the nodes seem to be there, as my tests do find bugs, but if I take screenshots *without* those delays, I see empty gray panes instead of the contet.

> >+        if (idSet.target == this.CURRENT_WINDOW)
> >+          this.walk(idSet.subContent);
> 
> Can we make .target optional? In this case it will always be true.

It won't be always true, like I said. If the target is *not* the current window, like in case of new windows or new modal windows, we don't go in there.
 
> >+    else if (switchingElement.localName == "tab") {
> >+      var nodeToClickOn = doc.getElementById(idSet.id);
> >+      
> >+      if (nodeToClickOn.selected != true) {
> >+        this._controller.click(new elementslib.ID(doc, idSet.id));
> >+        this._controller.sleep(gDelay);
> 
> You already have the node above. 

elementslib.ID does not return me a Node, and I need a Node to test for selected.

> Use elementslib.Elem. 

That should go in a follow-up.

> I would also like to see
> that we can get rid of the sleep call.

Then please tell me a way to see, if everything was *displayed*, as nothing I found waits for the content to be displayed but just to the tab be "selected".

> >+        if (idSet.target == this.CURRENT_WINDOW)
> >+          this.walk(idSet.subContent);
> 
> snip, as above.

as above

> >+    else if (switchingElement.localName == "button") {
> >+      this._controller.click(new elementslib.ID(doc, idSet.id));
> >+      this._controller.sleep(gDelay);
> 
> snip, same comment for sleep.

above

> >+      if (idSet.target == this.CURRENT_WINDOW)
> >+        this.walk(idSet.subContent);
> 
> What about the case when a button opens a new window?

see above and see the _openNew method, where this method is being used for *new* windows too.

> >+    }
> >+    
> >+    else {
> >+      this._controller.assert(function() { return false; },
> >+                              "Unknown node.localName: " +
> >+                              switchingElement.localName);  
> >+    }
> 
> Please fix the indentation for all lines.

Does the content have to begin under the first "("? A link to a style guide for Mozmill tests would be nice too.

> >+   * @param {Node} root (optional) Node to start testing from
> 
> nit: See the AddonAPI how to format this correctly.
 
ok. See above too.

> >+  _walk : function DOMWalker_walk(root) {
> >+    if (root == undefined || root == null)
> >+      root = this._controller.window.document.documentElement;
> >+    var nodes = root.childNodes;
> 
> Just test for 'if (!root)'

ok

> 
> >+    callbackFilter = this._callbackFilter;
> >+    callbackNodeTest = this._callbackNodeTest;
> 
> why this is necessary?

see the comment about "callbackResults"

> >+    var resultsArray = [];
> 
> Name it testResults please. 
>
> >+    for (var i = 0; i < nodes.length; i++) {
> [..]
> >+      
> >+      var resultArray = [];
> 
> Name it results. No need to say that we use an array.

I changed this because you've had told me to do it ("Also name the variable so it reflects it is an array."). So what should I do?

> >+        resultArray = this._walk(nodes[i]);
> >+      } else if (nodeStatus == this.FILTER_SKIP) {
> >+        resultArray = this._walk(nodes[i]);
> >+      }
> >+      resultsArray = resultsArray.concat(resultArray);
> >+    }
> 
> A switch condition would be preferred. 

Maybe I do something wrong when using switch, but changing it to switch causes a break here...

> Only use the break statement after
> FILTER_SKIP, so FILTER_ACCEPT can also use the FILTER_SKIP code.

...maybe I did always forget the break statement at all...

Comment 10

[Adrian Kalla [:adriank]]

>No idea why, but doing "this._callbackResults.callbackResults()" always fails and "callbackResults.callbackResults()" works...

should be "this._callbackResults()" and "callbackResults()"

Comment 11

[Adrian Kalla [:adriank]]

Attachment: DOMWalker v3 (WIP)

Not asking for review, as there are still open questions here - a summary of the open ones below:

(In reply to comment #8)
> >+  set callbackFilter(callback) {
> >+    this._callbackFilter = callback;
> >+  },
> 
> Do we have needs to re-set the callback filters after a DOMWalker has been
> instantiated?

Not for my existing tests, but some tests may need to switch to another filter
for some specific window parts. Because of that it may make sense to have that
here.


> >+    let callbackResults = this._callbackResults;
> 
> Why you need to do that?

No idea why, but doing "this._callbackResults()" always fails
and "callbackResults()" works...

> >+    var resultsArray = this._walk(root);
> 
> Those are 'collectedNodes', aren't they?

No, the aren't. It depends on what the array returned by callbackNodeTest()
contains.


> >+    for (var i = 0; i < ids.length; i++) {
> >+      var switchingElement = doc.getElementById(ids[i].id);
> 
> Lately I was working on the nodeCollector class
> (http://hg.mozilla.org/qa/mozmill-tests/diff/default/shared-modules/testDOMUtilsAPI.js)
> which make use of the querySelector. Use this class so we can also retrieve
> nodes not only by id but by any class and attribute.

I'd really like to move that to a later follow up - it would introduce too much
noise at the moment.


> >+        } else if (idSet.target == this.NEW_WINDOW) {
> >+          // Modal windows have to be able to access that informations
> >+          var modalInfos = {ids : idSet.subContent,
> >+                            callbackFilter :  this._callbackFilter,
> >+                            callbackNodeTest : this._callbackNodeTest,
> >+                            callbackResults : this._callbackResults}
> >+          persisted.modalInfos = modalInfos;
> 
> You should initialize this object once in the constructor and simply update the
> ids member when necessary. Think about storing the callbacks directly in there.
> No need to carry two references around. Also please check the indentation for
> objects. Therefore see the AddonsAPI. In general I would suggest
> 'persisted.callbacks = { ... }' and 'persisted.nodeList'.

Storing the ids in the constructor is a really bad idea, as we don't start a
new DOMWalker instance for window-elements like prefpanes. If a recursion run
changes the ids to it's ids.subContent, then other siblings will use later that
subContent instead of the original ids array...

Beside that, all the callbacks are always in the constructor. The above code
prepares the elements to be processed to _windowHelper, where a new DOMWalker
instance is being created each time.


> >+    if (switchingElement.localName == "menulist") {
> >+      var nodeToClickOn = doc.getElementById(idSet.id);
> >+      if (nodeToClickOn.label != idSet.title) {
> 
> As talked we should default to the value and not the label. I would still
> prefer to see it customizable, so it will work for any possible
> property/attribute. Call the variable dropDownNode...

As long as controller.select() does not work, I need the label.

> >+        var dropDown = new elementslib.ID(doc, idSet.id);
> >+        this._controller.waitForElement(dropDown, gTimeout);
> 
> Use elementslib.Elem with the element you already have retrieved above.

best for a follow-up

> >+        // controller.select doesn't work with our filters on non-Windows OS's
> >+        // No idea why, so lets do it manually for now
> >+        if (!mozmill.isWindows) {
> >+          for (j = 0; j < idSet.title.length; j++) {
> >+            this._controller.keypress(dropDown, idSet.title[j], {});
> >+            this._controller.sleep(100);
> >+          };
> >+          this._controller.keypress(dropDown, "VK_RETURN", {});
> >+        } else {
> 
> I don't see a problem with our existing tests for the pref pane. Can you please
> elaborate this a bit more detailed? You have a simple minimized test?

I don't know why it happens - I spend a whole night on this problem and
couldn't find a solution. As said, the below code works on Windows, but not on
Mac.

I'll try to do a minimized test case later.

> >+          this._controller.select(dropDown, null, idSet.title);
> >+          this._controller.waitFor(function()
> >+                                   {
> >+                                      return nodeToClickOn.label == idSet.title;
> >+                                   }
> 
> There is no need for wait for the selected element. It's more important to wait
> for the target to be active or selected. 

Let me repeat the below comment: "No known generic way to check, if a panes
*content* did load". It could mean *content* of anything too, like content of a
part of a prefpane. If you know a way, please tell me it.

> >+        // No known generic way to check, if a panes *content* did load
> >+        this._controller.sleep(gTimeout);
> >
> >+        if (idSet.target == this.CURRENT_WINDOW)
> >+          this.walk(idSet.subContent);
> 
> Huh? You should really differentiate between different node targets. Not all
> nodes with the type menulist will change a pane in the current window.

That's *why* it is in an if clause. In case of new Windows the _windowHelper
start's DOMWalker.walk() - it can't be done here.

> And
> using a timeout is definitely the wrong way to wait for a selection or
> activation.

Then tell me how. I couldn't find *any* generic element, attribute that would
tell me, that the *content* did load.


> >+      if (prefDialog.paneId != idSet.id) {
> >+        prefDialog.paneId = idSet.id;
> >+        
> >+        // No known generic way to check, if a panes *content* did load
> >+        this._controller.sleep(gDelay);
> 
> in 'set paneId' we wait until the new pane has been selected. So the content
> should also be ready. Why we need to wait?

Yes, we are waiting there for the *selection*. But it does not say *anything*
if all nodes were loaded *and* that the content is being displayed.
The content of most of the panes is ready around 900ms *after the pane has been
marked as selected*. On my virtual machine, it takes up to 2000ms to load the
content beside the "Applications pane" - that one needs up to 4500ms, so I
think to extend the sleep call to 5 seconds.

I spend on finding a way to find out if everything was displayed more than too
much time and didn't find a way beside sleep - calls. Even the existing Mozmill
tests use sleep there:
http://hg.mozilla.org/qa/mozmill-tests/file/62551d70805a/firefox/testPreferences/testSwitchPanes.js#l78
.
One thing is clear - the nodes seem to be there, as my tests do find bugs, but
if I take screenshots *without* those delays, I see empty gray panes instead of
the contet.

> >+        if (idSet.target == this.CURRENT_WINDOW)
> >+          this.walk(idSet.subContent);
> 
> Can we make .target optional? In this case it will always be true.

It won't be always true, like I said. If the target is *not* the current
window, like in case of new windows or new modal windows, we don't go in there.

> >+    else if (switchingElement.localName == "tab") {
> >+      var nodeToClickOn = doc.getElementById(idSet.id);
> >+      
> >+      if (nodeToClickOn.selected != true) {
> >+        this._controller.click(new elementslib.ID(doc, idSet.id));
> >+        this._controller.sleep(gDelay);
> 
> You already have the node above. 

elementslib.ID does not return me a Node, and I need a Node to test for
selected.

> Use elementslib.Elem. 

That should go in a follow-up.

> I would also like to see
> that we can get rid of the sleep call.

Then please tell me a way to see, if everything was *displayed*, as nothing I
found waits for the content to be displayed but just to the tab be "selected".

> >+        if (idSet.target == this.CURRENT_WINDOW)
> >+          this.walk(idSet.subContent);
> 
> snip, as above.

as above

> >+    else if (switchingElement.localName == "button") {
> >+      this._controller.click(new elementslib.ID(doc, idSet.id));
> >+      this._controller.sleep(gDelay);
> 
> snip, same comment for sleep.

above

> >+      if (idSet.target == this.CURRENT_WINDOW)
> >+        this.walk(idSet.subContent);
> 
> What about the case when a button opens a new window?

see above and see the _openNew method, where this method is being used for
*new* windows too.

> >+    }
> >+    
> >+    else {
> >+      this._controller.assert(function() { return false; },
> >+                              "Unknown node.localName: " +
> >+                              switchingElement.localName);  
> >+    }
> 
> Please fix the indentation for all lines.

Does the content have to begin under the first "("? A link to a style guide for
Mozmill tests would be nice too.


> >+    callbackFilter = this._callbackFilter;
> >+    callbackNodeTest = this._callbackNodeTest;
> 
> why this is necessary?

see the comment about "callbackResults"

> >+    var resultsArray = [];
> 
> Name it testResults please. 
>
> >+    for (var i = 0; i < nodes.length; i++) {
> [..]
> >+      
> >+      var resultArray = [];
> 
> Name it results. No need to say that we use an array.

I changed this because you've had told me to do it ("Also name the variable so
it reflects it is an array."). So what should I do?

Comment 12

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

(In reply to comment #9)
> gDelay and gTimeout too?

Sure.

> > >+  set callbackFilter(callback) {
> > >+    this._callbackFilter = callback;
> > >+  },
> > 
> > Do we have needs to re-set the callback filters after a DOMWalker has been
> > instantiated?
> 
> Not for my existing tests, but some tests may need to switch to another filter
> for some specific window parts. Because of that it may make sense to have that
> here.

Wouldn't that also require a new data list which needs to be pushed to the DOMWalker. I don't see a member which would allow to use a different callback. And once walk has been called there is no way for the consumer to do anything before the work has been completed. 

> > >+    let callbackResults = this._callbackResults;
> > 
> > Why you need to do that?
> 
> No idea why, but doing "this._callbackResults.callbackResults()" always fails
> and "callbackResults.callbackResults()" works...

What fails? Which error do you get?

> > >+    var resultsArray = this._walk(root);
> > 
> > Those are 'collectedNodes', aren't they?
> 
> No, the aren't. It depends on what the array returned by callbackNodeTest()
> contains.

So name it nodeTestResults or something like that. It would bind the name better to what's in there.

> > Lately I was working on the nodeCollector class
> > (http://hg.mozilla.org/qa/mozmill-tests/diff/default/shared-modules/testDOMUtilsAPI.js)
> > which make use of the querySelector. Use this class so we can also retrieve
> > nodes not only by id but by any class and attribute.
> 
> I'd really like to move that to a later follow up - it would introduce too much
> noise at the moment.

It's something we would need for the final version. So please file a bug and mark it dependent. 

> > >+        } else if (idSet.target == this.NEW_WINDOW) {
> > >+          // Modal windows have to be able to access that informations
> > >+          var modalInfos = {ids : idSet.subContent,
> > >+                            callbackFilter :  this._callbackFilter,
> > >+                            callbackNodeTest : this._callbackNodeTest,
> > >+                            callbackResults : this._callbackResults}
> > >+          persisted.modalInfos = modalInfos;
> > 
> > You should initialize this object once in the constructor and simply update the
> > ids member when necessary. Think about storing the callbacks directly in there.
> > No need to carry two references around. Also please check the indentation for
> > objects. Therefore see the AddonsAPI. In general I would suggest
> > 'persisted.callbacks = { ... }' and 'persisted.nodeList'.
> 
> Storing the ids in the constructor is a really bad idea, as we don't start a
> new DOMWalker instance for window-elements like prefpanes. If a recursion run
> changes the ids to it's ids.subContent, then other siblings will use later that
> subContent instead of the original ids array...

I have never told that you should put the assignment of the ids member into the constructor. See: "... and simply update the ids member when necessary".

> Beside that, all the callbacks are always in the constructor. The above code
> prepares the elements to be processed to _windowHelper, where a new DOMWalker
> instance is being created each time.

Which would then override the already stored values in the persisted object, as long as we do not use a guid for the variable name. So ok, that slipped through my fingers. Leave it as it is. But please make sure to delete this object at the end.

> > Can you please rename '_switchElement' to '_processNode', because that's really
> > what happens here, right?
> 
> Even more happens here -> the new window/dialog etc. is being opened here. So
> not sure if "processNode" is the right name... Maybe "switchDialog"?

Which is a possible target state. But finally you have a node, you check which type it is, and operate on it to start another walk.

> > >+    if (switchingElement.localName == "menulist") {
> > >+      var nodeToClickOn = doc.getElementById(idSet.id);
> > >+      if (nodeToClickOn.label != idSet.title) {
> > 
> > As talked we should default to the value and not the label. I would still
> > prefer to see it customizable, so it will work for any possible
> > property/attribute. Call the variable dropDownNode...
> 
> As long as controller.select() does not work, I need the label.

Why? Even without controller.select() you can use value.

> > I don't see a problem with our existing tests for the pref pane. Can you please
> > elaborate this a bit more detailed? You have a simple minimized test?
> 
> I don't know why it happens - I spend a whole night on this problem and
> couldn't find a solution. As said, the below code works on Windows, but not on
> Mac.
> 
> I'll try to do a minimized test case later.

Thanks. Please file this as a separate bug for Mozmill.

> > >+          this._controller.select(dropDown, null, idSet.title);
> > >+          this._controller.waitFor(function()
> > >+                                   {
> > >+                                      return nodeToClickOn.label == idSet.title;
> > >+                                   }
> > 
> > There is no need for wait for the selected element. It's more important to wait
> > for the target to be active or selected. 
> 
> Let me repeat the below comment: "No known generic way to check, if a panes
> *content* did load". It could mean *content* of anything too, like content of a
> part of a prefpane. If you know a way, please tell me it.

Once selectedPanel or selectedIndex is updated for a deck panel the content should have been loaded. I'm also using this method in the AddonsAPI for the selected panel on the right side (list view vs. details view). But the problem is that the walker is not smart enough yet to know, which node it has to watch for a change. The connection between the current node and the target whatever is absolutely something we should have. But we can also move this out to another bug.

> > >+        if (idSet.target == this.CURRENT_WINDOW)
> > >+          this.walk(idSet.subContent);
> > 
> > Huh? You should really differentiate between different node targets. Not all
> > nodes with the type menulist will change a pane in the current window.
> 
> That's *why* it is in an if clause. In case of new Windows the _windowHelper
> start's DOMWalker.walk() - it can't be done here.

So in the "else" case the previously created modal window watcher or window helper instance kicks in? If that's the case here, please add a note to the comment.

> > And
> > using a timeout is definitely the wrong way to wait for a selection or
> > activation.
> 
> Then tell me how. I couldn't find *any* generic element, attribute that would
> tell me, that the *content* did load.

As said above.

> > >+      if (prefDialog.paneId != idSet.id) {
> > >+        prefDialog.paneId = idSet.id;
> > >+        
> > >+        // No known generic way to check, if a panes *content* did load
> > >+        this._controller.sleep(gDelay);
> > 
> > in 'set paneId' we wait until the new pane has been selected. So the content
> > should also be ready. Why we need to wait?
> 
> Yes, we are waiting there for the *selection*. But it does not say *anything*
> if all nodes were loaded *and* that the content is being displayed.
> The content of most of the panes is ready around 900ms *after the pane has been
> marked as selected*. On my virtual machine, it takes up to 2000ms to load the
> content beside the "Applications pane" - that one needs up to 4500ms, so I
> think to extend the sleep call to 5 seconds.

There should be a notification. Can you please get in contact with Neil Deakin? He should be able to help you.

> > >+    else if (switchingElement.localName == "tab") {
> > >+      var nodeToClickOn = doc.getElementById(idSet.id);
> > >+      
> > >+      if (nodeToClickOn.selected != true) {
> > >+        this._controller.click(new elementslib.ID(doc, idSet.id));
> > >+        this._controller.sleep(gDelay);
> > 
> > You already have the node above. 
> 
> elementslib.ID does not return me a Node, and I need a Node to test for
> selected.

Right, but for controller.click use elementslib.Elem(nodeToClickOn).

> > Use elementslib.Elem. 
> 
> That should go in a follow-up.

Nope. Please fix that in-place.

> > I would also like to see
> > that we can get rid of the sleep call.
> 
> Then please tell me a way to see, if everything was *displayed*, as nothing I
> found waits for the content to be displayed but just to the tab be "selected".

Same as above. I hope Neil can help.

> > >+    }
> > >+    
> > >+    else {
> > >+      this._controller.assert(function() { return false; },
> > >+                              "Unknown node.localName: " +
> > >+                              switchingElement.localName);  
> > >+    }
> > 
> > Please fix the indentation for all lines.
> 
> Does the content have to begin under the first "("? A link to a style guide for
> Mozmill tests would be nice too.

We are working on a style guide at the moment and it should be ready seen. For now check e.g. http://hg.mozilla.org/qa/mozmill-tests/file/62551d70805a/firefox/testCookies/testRemoveCookie.js

> > >+    var resultsArray = [];
> > 
> > Name it testResults please. 
> >
> > >+    for (var i = 0; i < nodes.length; i++) {
> > [..]
> > >+      
> > >+      var resultArray = [];
> > 
> > Name it results. No need to say that we use an array.
> 
> I changed this because you've had told me to do it ("Also name the variable so
> it reflects it is an array."). So what should I do?

In that case I referred to "result" which was an array but not visible as such. But please don't put "Array" in the name.

> Maybe I do something wrong when using switch, but changing it to switch causes
> a break here...
> 
> > Only use the break statement after
> > FILTER_SKIP, so FILTER_ACCEPT can also use the FILTER_SKIP code.
> 
> ...maybe I did always forget the break statement at all...

What do you mean?

Comment 13

[Adrian Kalla [:adriank]]

(In reply to comment #12)
> Wouldn't that also require a new data list which needs to be pushed to the
> DOMWalker. I don't see a member which would allow to use a different callback.
> And once walk has been called there is no way for the consumer to do anything
> before the work has been completed. 

right. I did remove all the set methods now.

> > No idea why, but doing "this._callbackResults.callbackResults()" always fails
> > and "callbackResults.callbackResults()" works...
> 
> What fails? Which error do you get?

I don't get any error. The test just stops and the teardown method jumps in...
 
> So name it nodeTestResults or something like that. It would bind the name
> better to what's in there.

done

> > > Lately I was working on the nodeCollector class
> > > (http://hg.mozilla.org/qa/mozmill-tests/diff/default/shared-modules/testDOMUtilsAPI.js)
> > > which make use of the querySelector. Use this class so we can also retrieve
> > > nodes not only by id but by any class and attribute.
> > 
> > I'd really like to move that to a later follow up - it would introduce too much
> > noise at the moment.
> 
> It's something we would need for the final version. So please file a bug and
> mark it dependent. 

ok

> I have never told that you should put the assignment of the ids member into the
> constructor. See: "... and simply update the ids member when necessary".

Looks like I misinterpreted it.

> Which would then override the already stored values in the persisted object, as
> long as we do not use a guid for the variable name. So ok, that slipped through
> my fingers. Leave it as it is. But please make sure to delete this object at
> the end.

ok

> > > Can you please rename '_switchElement' to '_processNode', because that's really
> > > what happens here, right?
> > 
> > Even more happens here -> the new window/dialog etc. is being opened here. So
> > not sure if "processNode" is the right name... Maybe "switchDialog"?
> 
> Which is a possible target state. But finally you have a node, you check which
> type it is, and operate on it to start another walk.

ok, done

> > As long as controller.select() does not work, I need the label.
> 
> Why? Even without controller.select() you can use value.

Looks like I have to find out how that works.

> Once selectedPanel or selectedIndex is updated for a deck panel the content
> should have been loaded. I'm also using this method in the AddonsAPI for the
> selected panel on the right side (list view vs. details view). But the problem
> is that the walker is not smart enough yet to know, which node it has to watch
> for a change. The connection between the current node and the target whatever
> is absolutely something we should have. But we can also move this out to
> another bug.
 
Right. I did use "content load", and I did mean "content display" - the content is there, as I find errors, but the pain's content is definitely not displayed right after selection.

> So in the "else" case the previously created modal window watcher or window
> helper instance kicks in? If that's the case here, please add a note to the
> comment.

Yes, that's the case


> There should be a notification. Can you please get in contact with Neil Deakin?
> He should be able to help you.

what IRC nick does he use?

> > elementslib.ID does not return me a Node, and I need a Node to test for
> > selected.
> 
> Right, but for controller.click use elementslib.Elem(nodeToClickOn).

done

> > > Use elementslib.Elem. 
> > 
> > That should go in a follow-up.
> 
> Nope. Please fix that in-place.

ok, done

> > > >+    }
> > > >+    
> > > >+    else {
> > > >+      this._controller.assert(function() { return false; },
> > > >+                              "Unknown node.localName: " +
> > > >+                              switchingElement.localName);  
> > > >+    }
> > > 
> > > Please fix the indentation for all lines.
> > 
> > Does the content have to begin under the first "("? A link to a style guide for
> > Mozmill tests would be nice too.
> 
> We are working on a style guide at the moment and it should be ready seen. For
> now check e.g.
> http://hg.mozilla.org/qa/mozmill-tests/file/62551d70805a/firefox/testCookies/testRemoveCookie.js

Ok, thx.
Now I remember why I used such indentation there: it was there for a comment to be written - done now.

> > > >+    var resultsArray = [];
> > > 
> > > Name it testResults please. 
> > >
> > > >+    for (var i = 0; i < nodes.length; i++) {
> > > [..]
> > > >+      
> > > >+      var resultArray = [];
> > > 
> > > Name it results. No need to say that we use an array.
> > 
> > I changed this because you've had told me to do it ("Also name the variable so
> > it reflects it is an array."). So what should I do?
> 
> In that case I referred to "result" which was an array but not visible as such.
> But please don't put "Array" in the name.

ok

> > Maybe I do something wrong when using switch, but changing it to switch causes
> > a break here...
> > 
> > > Only use the break statement after
> > > FILTER_SKIP, so FILTER_ACCEPT can also use the FILTER_SKIP code.
> > 
> > ...maybe I did always forget the break statement at all...
> 
> What do you mean?

I mean with that, that last time I tried to use "switch", I forgot about putting "break" for each case, and was wondering, why it didn't work as expected ;)

Comment 14

[Adrian Kalla [:adriank]]

Attachment: DOMWalker v4

What's left:
-"ids" related stuff, that'll be part of a follow-up bug, that will have to be resolved before pushing this to the mozmill-tests repository
-controller.select problem, at least on Mac (selecting the right pane works, but access key tests in that pane don't find any problems, even if duplicated access keys are in there)
-prefpane switching problem: after a prefpane has been loaded, an animated fade-in takes place, which results in having grayed-out screenshots. Because of that, a 1 second sleep call is in place for now

Henrik, if you could review the patch please - but omit the described parts, for now.

Comment 15

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

(In reply to comment #14)
> -"ids" related stuff, that'll be part of a follow-up bug, that will have to be
> resolved before pushing this to the mozmill-tests repository

Please finally file this bug and others and add those to the dependency list. I have mentioned it a couple of times. Review will happen when I exactly know what will happen in the other bugs.

> -prefpane switching problem: after a prefpane has been loaded, an animated
> fade-in takes place, which results in having grayed-out screenshots. Because of
> that, a 1 second sleep call is in place for now

Fade-in means a change of the opacity. Isn't there an event we can listen for?

Comment 16

[Adrian Kalla [:adriank]]

Attachment: DOMWalker v5

all review comments should have been addressed by this patch, if I didn't forget something

Comment 17

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

Comment on attachment 488730 [details] [diff] [review]
DOMWalker v5

>  * Contributor(s):
>+ *   Adrian Kalla <[e-mail]>
>  *   Henrik Skupin <[e-mail]>

Don't put yourself in-front of existing contributors. That gives the impression you are the original author of this module.

>+const GDELAY = 100;
>+const GTIMEOUT = 5000;

Remove the 'G' in both constants. We are not using Hungarian notation in our repository.

>+function DOMWalker(controller, callbackFilter, callbackNodeTest,
>+                   callbackResults, ids, root) {

It would be really helpful for me when you can explain why the root parameter has been added to the constructor. Not sure why it is necessary here and in the walk method. Same for ids.

>+  if (!root)
>+    root = controller.window.document.documentElement;
>+  this._root = root;

If you want to initialize it depending on the parameter do a '= root || controller.window.document.documentElement'.

>+   * @param {array of objects} ids
>+   *        Contains informations on the elements to open while
>+   *        Object-elements:  attribute     - attribute-name of the attribute
>+   *                                          containing the identification
>+   *                                          information for the opener-element
>+   *                          subContent    - array of ids of the opener-elements
>+   *                                          in the window with the above id

"above id" should probably be the attribute.

>+   *                          target        - information, where the new
>+   *                                          elements will be opened
>+   *                                          [1|2|4]
>+   *                          title         - title of the opened dialog/window
>+   *                          waitFunction  - The function used as an argument
>+   *                                          for MozmillController.waitFor to 
>+   *                                          wait before starting the walk.
>+   *                                          [optional - default: no waiting] 
>+   *                          windowHandler - Window instance
>+   *                                          [only needed for some tests]

Same here. A lot of code has been moved around without giving any information why. This makes reviewing the code really hard.

>+   * @param {Function} waitFunction
>+   *        The function used as an argument for MozmillController.waitFor to 
>+   *        wait before starting the walk.
>+   *        [optional - default: no waiting] 

Why once more? And why we have to wait?

>+   *        Object-elements:  attribute     - attribute-name of the attribute
[..]
>+   * 
>+   * @returns Node
>+   * @type {Node}
>+   */
>+  _getNode : function DOMWalker_getNode(idSet) {

Same comments as above for his comment.

>+    var doc = this._controller.window.document;

Why you don't use this._root?

>+    // QuerySelector seems to be unusuale for id's in this case:
>+    // https://developer.mozilla.org/En/Code_snippets/QuerySelector
>+    if (idSet.attribute == "id") {
>+      return doc.getElementById(idSet[idSet.attribute]);
>+    } else if (idSet.attribute == "querySelector") {
>+      return doc.querySelector(idSet[idSet.attribute]);

Can you give me an example of the updated isSet array vs. object? Right now I don't understand how this code works.

>+    } else {
>+      this._controller.assert(function() { return false; },
>+                              "Not supported attribute: " + idSet.attribute);  

Don't use the controller.assert method to force an exception. Raise it on your own by using "new Error".

>+  _prepareTargetWindows : function DOMWalker_prepareTargetWindows(ids) {
>+    var doc = this._controller.window.document;

Same here. why not this.__root?

>+          // Get the new non-modal window controller
>+          var wCont = utils.handleWindow('title', idSet.title,
>+                                                  false, true);

Why don't you use 'controller'? Also please align the parameter by the functions opening bracket.

>+        this._controller.assert(function() { return false; },
>+                                "ERROR: Node " + ids[i][ids[i].attribute] +
>+                                " does not exist!");  

As commented above.

>+  _processNode: function DOMWalker_processNode(activeNode, idSet) {
>+    var doc = this._controller.window.document;

this._root?

>+      if (nodeToProcess.label != idSet.title) {

I still haven't gotten an answer why you are using label instead of value.

>+          for (j = 0; j < idSet.title.length; j++) {

missing variable declaration.

>+          this._controller.select(dropDown, null, idSet.title);
>+          this._controller.waitFor(function() {
>+                                    return nodeToProcess.label == idSet.title;
>+                                   }
>+          );

See the following link (pre-final) how to specify a message and to format this correctly:
https://wiki.mozilla.org/QA/Mozmill_Test_Automation/Test_Modules_Refactor

>+        // Wait for the pane's content to load and to be fully displayed
>+        this._controller.waitFor(function() {
>+                                  return nodeToProcess.loaded &&
>+                                  nodeToProcess.style.opacity == 1;
>+                                }
>+        );

nit: format and message.

>+  _walk : function DOMWalker__walk(root) {
>+    if (!root)
>+      root = this._controller.window.document.documentElement;

root shouldn't be optional for this function.

>+    var nodes = root.childNodes;

Just to be sure, we never have to check the root element?

>+      switch (nodeStatus) {
>+        case DOMWalker.FILTER_ACCEPT:
>+          nodeTestResults = this._callbackNodeTest(nodes[i]);
>+          collectedResults = collectedResults.concat(nodeTestResults);
>+        case DOMWalker.FILTER_SKIP:

Please comment why there is no break statement. This causes confusion.

>+  _modalWindowHelper: function DOMWalker_modalWindowHelper(controller) {
[..]
>+    domWalker.walk(persisted.modalInfos.ids,
>+                   controller.window.document.documentElement,
>+                   persisted.modalInfos.waitFunction);

Still not clear for what this wait function is used for.

Also I miss an answer for:

>> -prefpane switching problem: after a prefpane has been loaded, an animated
>> fade-in takes place, which results in having grayed-out screenshots. Because of
>> that, a 1 second sleep call is in place for now
>
> Fade-in means a change of the opacity. Isn't there an event we can listen for?

Comment 18

[Adrian Kalla [:adriank]]

Attachment: DOMWalker v6

(In reply to comment #17)
> Comment on attachment 488730 [details] [diff] [review]
> DOMWalker v5
> 
> >  * Contributor(s):
> >+ *   Adrian Kalla <[e-mail]>
> >  *   Henrik Skupin <[e-mail]>
> 
> Don't put yourself in-front of existing contributors. That gives the impression
> you are the original author of this module.

sorry - I thought it was an alphabetically sorted list...

> >+const GDELAY = 100;
> >+const GTIMEOUT = 5000;
> 
> Remove the 'G' in both constants. We are not using Hungarian notation in our
> repository.

done. Didn't hear anything about the "Hungarian notation" until now, so something new learned ;)

> >+function DOMWalker(controller, callbackFilter, callbackNodeTest,
> >+                   callbackResults, ids, root) {
> 
> It would be really helpful for me when you can explain why the root parameter
> has been added to the constructor. Not sure why it is necessary here and in the
> walk method. Same for ids.

I had to check that first too and... it is a left-over from my experiments with nodeCollector, which is not needed anyway, as "querySelector" was used at the end.

> >+  if (!root)
> >+    root = controller.window.document.documentElement;
> >+  this._root = root;
> 
> If you want to initialize it depending on the parameter do a '= root ||
> controller.window.document.documentElement'.

not needed anymore - see above

> >+   * @param {array of objects} ids
> >+   *        Contains informations on the elements to open while
> >+   *        Object-elements:  attribute     - attribute-name of the attribute
> >+   *                                          containing the identification
> >+   *                                          information for the opener-element
> >+   *                          subContent    - array of ids of the opener-elements
> >+   *                                          in the window with the above id
> 
> "above id" should probably be the attribute.

oops, didn't catch that when changing the descriptions

> >+   *                          target        - information, where the new
> >+   *                                          elements will be opened
> >+   *                                          [1|2|4]
> >+   *                          title         - title of the opened dialog/window
> >+   *                          waitFunction  - The function used as an argument
> >+   *                                          for MozmillController.waitFor to 
> >+   *                                          wait before starting the walk.
> >+   *                                          [optional - default: no waiting] 
> >+   *                          windowHandler - Window instance
> >+   *                                          [only needed for some tests]
> 
> Same here. A lot of code has been moved around without giving any information
> why. This makes reviewing the code really hard.

I didn't rembember that it was that much, but I may be wrong ;)
Most of the moving was the result of adding support for the querySelector, e.g. "_getNode" is a result of this - that's the reason why I wanted to have it in a follow-up patch. But that'd have made only sense, if done one after another, and not at the same time...

> >+   * @param {Function} waitFunction
> >+   *        The function used as an argument for MozmillController.waitFor to 
> >+   *        wait before starting the walk.
> >+   *        [optional - default: no waiting] 
> 
> Why once more? And why we have to wait?

This was done as a result of one of our latest 1-1's. From my notes: "have the ability to wait for a node specified in a test". It maybe usefull for some non-standard situations.
 
> >+   *        Object-elements:  attribute     - attribute-name of the attribute
> [..]
> >+   * 
> >+   * @returns Node
> >+   * @type {Node}
> >+   */
> >+  _getNode : function DOMWalker_getNode(idSet) {
> 
> Same comments as above for his comment.

see above

> >+    var doc = this._controller.window.document;
> 
> Why you don't use this._root?

as this._root shouldn't have existed in the first place - and is removed now
 
> >+    // QuerySelector seems to be unusuale for id's in this case:
> >+    // https://developer.mozilla.org/En/Code_snippets/QuerySelector
> >+    if (idSet.attribute == "id") {
> >+      return doc.getElementById(idSet[idSet.attribute]);
> >+    } else if (idSet.attribute == "querySelector") {
> >+      return doc.querySelector(idSet[idSet.attribute]);
> 
> Can you give me an example of the updated isSet array vs. object? Right now I
> don't understand how this code works.

e.g.:

[
  {
    attribute : "id",
    id : "popupPolicyButton",
    target : WINDOW_NEW,
    title : utils.getProperty(properties, "popuppermissionstitle")
  },
  {
    attribute : "querySelector",
    querySelector : 'button[accesskey='+utils.getEntity(dtds, "exceptions.accesskey")+'][label='+utils.getEntity(dtds, "exceptions.label")+']',
    target : WINDOW_NEW,
    title : utils.getProperty(properties, "imagepermissionstitle")
  }
]
 
> >+    } else {
> >+      this._controller.assert(function() { return false; },
> >+                              "Not supported attribute: " + idSet.attribute);  
> 
> Don't use the controller.assert method to force an exception. Raise it on your
> own by using "new Error".

done

> >+  _prepareTargetWindows : function DOMWalker_prepareTargetWindows(ids) {
> >+    var doc = this._controller.window.document;
> 
> Same here. why not this.__root?

see above

> >+          // Get the new non-modal window controller
> >+          var wCont = utils.handleWindow('title', idSet.title,
> >+                                                  false, true);
> 
> Why don't you use 'controller'?

Because I don't want to overwrite the controller of the parent window.

> Also please align the parameter by the
> functions opening bracket.

ok

> >+        this._controller.assert(function() { return false; },
> >+                                "ERROR: Node " + ids[i][ids[i].attribute] +
> >+                                " does not exist!");  
> 
> As commented above.

done

> >+  _processNode: function DOMWalker_processNode(activeNode, idSet) {
> >+    var doc = this._controller.window.document;
> 
> this._root?

see above

> >+      if (nodeToProcess.label != idSet.title) {
> 
> I still haven't gotten an answer why you are using label instead of value.

The answer is bug 606024. And I didn't find any way to use the value for the controller.keypress() workaround...

> >+          for (j = 0; j < idSet.title.length; j++) {
> 
> missing variable declaration.

oops, corrected

> >+          this._controller.select(dropDown, null, idSet.title);
> >+          this._controller.waitFor(function() {
> >+                                    return nodeToProcess.label == idSet.title;
> >+                                   }
> >+          );
> 
> See the following link (pre-final) how to specify a message and to format this
> correctly:
> https://wiki.mozilla.org/QA/Mozmill_Test_Automation/Test_Modules_Refactor

done and message added

> >+        // Wait for the pane's content to load and to be fully displayed
> >+        this._controller.waitFor(function() {
> >+                                  return nodeToProcess.loaded &&
> >+                                  nodeToProcess.style.opacity == 1;
> >+                                }
> >+        );
> 
> nit: format and message.

done and message added

> >+  _walk : function DOMWalker__walk(root) {
> >+    if (!root)
> >+      root = this._controller.window.document.documentElement;
> 
> root shouldn't be optional for this function.

In most cases probably this._controller.window.document.documentElement will be used, so I'd really like to have it as the default one.

> >+    var nodes = root.childNodes;
> 
> Just to be sure, we never have to check the root element?

IIRC, every node should have the "childNodes" attribute and when there are no children, it shuold be an empty array - and in such case, the for-loop won't be entered. But I'm not 100% sure here...

> 
> >+      switch (nodeStatus) {
> >+        case DOMWalker.FILTER_ACCEPT:
> >+          nodeTestResults = this._callbackNodeTest(nodes[i]);
> >+          collectedResults = collectedResults.concat(nodeTestResults);
> >+        case DOMWalker.FILTER_SKIP:
> 
> Please comment why there is no break statement. This causes confusion.

done

> >+  _modalWindowHelper: function DOMWalker_modalWindowHelper(controller) {
> [..]
> >+    domWalker.walk(persisted.modalInfos.ids,
> >+                   controller.window.document.documentElement,
> >+                   persisted.modalInfos.waitFunction);
> 
> Still not clear for what this wait function is used for.

see above

> Also I miss an answer for:
> 
> >> -prefpane switching problem: after a prefpane has been loaded, an animated
> >> fade-in takes place, which results in having grayed-out screenshots. Because of
> >> that, a 1 second sleep call is in place for now
> >
> > Fade-in means a change of the opacity. Isn't there an event we can listen for?

opacity was the answer that I've got from Neil too. The problem was solved here

> >+        // Wait for the pane's content to load and to be fully displayed
> >+        this._controller.waitFor(function() {
> >+                                  return nodeToProcess.loaded &&
> >+                                  nodeToProcess.style.opacity == 1;
> >+                                }
> >+        );

by waiting for "nodeToProcess.style.opacity == 1".

Comment 19

[Adrian Kalla [:adriank]]

Attachment: DOMWalker v6 - right file

oops, attached the dom-utils file, not the patch. Corrected now.

Comment 20

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

Comment on attachment 490493 [details] [diff] [review]
DOMWalker v6 - right file

>+const DELAY = 100;
>+const TIMEOUT = 5000;

If you really only have to use these two values you can completely remove those from the patch. Those are now the default settings for all waitFor* methods.

> I didn't rembember that it was that much, but I may be wrong ;)
> Most of the moving was the result of adding support for the querySelector, e.g.
> "_getNode" is a result of this - that's the reason why I wanted to have it in a
> follow-up patch. But that'd have made only sense, if done one after another,
> and not at the same time...

Combined with the renaming of the file, I was not able to check the inter-diff to the last version attached on this bug. 

> > >+   * @param {Function} waitFunction
> > >+   *        The function used as an argument for MozmillController.waitFor to 
> > >+   *        wait before starting the walk.
> > >+   *        [optional - default: no waiting] 
> > 
> > Why once more? And why we have to wait?
> 
> This was done as a result of one of our latest 1-1's. From my notes: "have the
> ability to wait for a node specified in a test". It maybe usefull for some
> non-standard situations.

Right. But shouldn't it be enough to specify the node or a list of nodes to wait for instead of passing in callback? Could you imagine a case when a callback would be necessary?

>   {
>     attribute : "querySelector",
>     querySelector : 'button[accesskey='+utils.getEntity(dtds,
> "exceptions.accesskey")+'][label='+utils.getEntity(dtds,
> "exceptions.label")+']',
>     target : WINDOW_NEW,
>     title : utils.getProperty(properties, "imagepermissionstitle")
>   }

Using attribute to specify how an element has to be retrieved can cause a lot of confusion. What you really want is something like getBy, i.e. "byID", "bySelector". Those should also be constants.

+      } else {
>+        throw new Error("Node " + ids[i][ids[i].attribute] + " does not exist!");
>+      }

To be consistent the element details should be displayed at the end of the string like "Node does not exist: %s".

>+          this._controller.waitFor(function() {
>+            return nodeToProcess.label == idSet.title;
>+          }, "The menu item " + idSet.title + " was not selected in time.", TIMEOUT);

See above.

>+        this._controller.waitFor(function() {
>+          return nodeToProcess.loaded && nodeToProcess.style.opacity == 1;
>+        }, "The pane " + idSet.id + " did not load in time.", TIMEOUT);

Dito. Also please use the same message as you have above. We shouldn't differentiate between selected and loaded for test results, at least not in this case.

> > >+          // Get the new non-modal window controller
> > >+          var wCont = utils.handleWindow('title', idSet.title,
> > >+                                                  false, true);
> > 
> > Why don't you use 'controller'?
> 
> Because I don't want to overwrite the controller of the parent window.

The class doesn't have a controller property. So it shouldn't conflict with any other controller in this scope.

> > I still haven't gotten an answer why you are using label instead of value.
> 
> The answer is bug 606024. And I didn't find any way to use the value for the
> controller.keypress() workaround...

Please file a bug and add the dependency to this bug and bug 606024 so we can change this once bug 606024 has been solved.

> > >+  _walk : function DOMWalker__walk(root) {
> > >+    if (!root)
> > >+      root = this._controller.window.document.documentElement;
> > 
> > root shouldn't be optional for this function.
> 
> In most cases probably this._controller.window.document.documentElement will be
> used, so I'd really like to have it as the default one.

It's an internal function of the DOMWalker and not used by any other code. Make sure that when you call this function an parameter is always passed in. It shouldn't be optional.

> > >+    var nodes = root.childNodes;
> > 
> > Just to be sure, we never have to check the root element?
> 
> IIRC, every node should have the "childNodes" attribute and when there are no
> children, it shuold be an empty array - and in such case, the for-loop won't be
> entered. But I'm not 100% sure here...

Well, what I meant was, do we ever have to run a check against the root element itself or will that never has to happen? Currently the node gets silently ignored.

>+    persisted.modalInfos = undefined;

Forgot about this the last time. Please use 'delete' here.

> > >> fade-in takes place, which results in having grayed-out screenshots. Because of
> > >> that, a 1 second sleep call is in place for now
> > >
> > > Fade-in means a change of the opacity. Isn't there an event we can listen for?
> 
> opacity was the answer that I've got from Neil too. The problem was solved here

Ok, thanks.

Looks like that we are close to the final state now. I would really be interested to have it ready this week. Would that be possible for you Adrian?

Comment 21

[Adrian Kalla [:adriank]]

Attachment: DOMWalker v7

(In reply to comment #20)
> Comment on attachment 490493 [details] [diff] [review]
> DOMWalker v6 - right file
> 
> >+const DELAY = 100;
> >+const TIMEOUT = 5000;
> 
> If you really only have to use these two values you can completely remove those
> from the patch. Those are now the default settings for all waitFor* methods.

ok, done.
The DELAY was used in the workaround for bug 606024, so I did put the 100 there directly.

> > > Why once more? And why we have to wait?
> > 
> > This was done as a result of one of our latest 1-1's. From my notes: "have the
> > ability to wait for a node specified in a test". It maybe usefull for some
> > non-standard situations.
> 
> Right. But shouldn't it be enough to specify the node or a list of nodes to
> wait for instead of passing in callback? Could you imagine a case when a
> callback would be necessary?

I think it is better to have such a general way of specifying what to wait for than by specifying a node or list of nodes. Also, the problem starts with the question: node or list of nodes? Then we would also need a list of attributes to wait for and their expected values. That's why I think it'll be easier to specify a callback.

> >   {
> >     attribute : "querySelector",
> >     querySelector : 'button[accesskey='+utils.getEntity(dtds,
> > "exceptions.accesskey")+'][label='+utils.getEntity(dtds,
> > "exceptions.label")+']',
> >     target : WINDOW_NEW,
> >     title : utils.getProperty(properties, "imagepermissionstitle")
> >   }
> 
> Using attribute to specify how an element has to be retrieved can cause a lot
> of confusion. What you really want is something like getBy, i.e. "byID",
> "bySelector". Those should also be constants.

ok, done. As that are constants, I named them: GET_BY_ID and GET_BY_SELECTOR,

> +      } else {
> >+        throw new Error("Node " + ids[i][ids[i].attribute] + " does not exist!");
> >+      }
> 
> To be consistent the element details should be displayed at the end of the
> string like "Node does not exist: %s".

done

> >+          this._controller.waitFor(function() {
> >+            return nodeToProcess.label == idSet.title;
> >+          }, "The menu item " + idSet.title + " was not selected in time.", TIMEOUT);
> 
> See above.

done

> >+        this._controller.waitFor(function() {
> >+          return nodeToProcess.loaded && nodeToProcess.style.opacity == 1;
> >+        }, "The pane " + idSet.id + " did not load in time.", TIMEOUT);
> 
> Dito. Also please use the same message as you have above. We shouldn't
> differentiate between selected and loaded for test results, at least not in
> this case.

done

> > > >+          // Get the new non-modal window controller
> > > >+          var wCont = utils.handleWindow('title', idSet.title,
> > > >+                                                  false, true);
> > > 
> > > Why don't you use 'controller'?
> > 
> > Because I don't want to overwrite the controller of the parent window.
> 
> The class doesn't have a controller property. So it shouldn't conflict with any
> other controller in this scope.

Right. Changed to controller.

> > > I still haven't gotten an answer why you are using label instead of value.
> > 
> > The answer is bug 606024. And I didn't find any way to use the value for the
> > controller.keypress() workaround...
> 
> Please file a bug and add the dependency to this bug and bug 606024 so we can
> change this once bug 606024 has been solved.

Bug 612687
 
> > > >+  _walk : function DOMWalker__walk(root) {
> > > >+    if (!root)
> > > >+      root = this._controller.window.document.documentElement;
> > > 
> > > root shouldn't be optional for this function.
> > 
> > In most cases probably this._controller.window.document.documentElement will be
> > used, so I'd really like to have it as the default one.
> 
> It's an internal function of the DOMWalker and not used by any other code. Make
> sure that when you call this function an parameter is always passed in. It
> shouldn't be optional.

ok - I moved that if to the main "walk" method.

> > > >+    var nodes = root.childNodes;
> > > 
> > > Just to be sure, we never have to check the root element?
> > 
> > IIRC, every node should have the "childNodes" attribute and when there are no
> > children, it shuold be an empty array - and in such case, the for-loop won't be
> > entered. But I'm not 100% sure here...
> 
> Well, what I meant was, do we ever have to run a check against the root element
> itself or will that never has to happen? Currently the node gets silently
> ignored.

ok - added a check to see, if root.childNodes exists. An error will be thrown if not, as something must be wrong then.

> >+    persisted.modalInfos = undefined;
> 
> Forgot about this the last time. Please use 'delete' here.

done

> > opacity was the answer that I've got from Neil too. The problem was solved here
> 
> Ok, thanks.

oops - nearly forgot to add "|| nodeToProcess.style.opacity == null", as right after opening the preferences window, the value isn't set (thx Neil)

> Looks like that we are close to the final state now. I would really be
> interested to have it ready this week. Would that be possible for you Adrian?

I hope it will :)

Comment 22

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

Comment on attachment 490982 [details] [diff] [review]
DOMWalker v7

>+  walk : function DOMWalker_walk(ids, root, waitFunction) {
>+    if (typeof waitFunction == 'function')
>+      this._controller.waitFor(waitFunction());

So what about ids.waitFunction here? Is that one forwarded to the walk function of sub windows?

>+    // QuerySelector seems to be unusuale for id's in this case:
>+    // https://developer.mozilla.org/En/Code_snippets/QuerySelector
>+    if (idSet.getBy == DOMWalker.GET_BY_ID) {
>+      return doc.getElementById(idSet[idSet.getBy]);
>+    } else if (idSet.getBy == DOMWalker.GET_BY_SELECTOR) {
>+      return doc.querySelector(idSet[idSet.getBy]);
>+    } else {
>+      throw new Error("Not supported getBy-attribute: " + idSet.getBy);
>+    }

Ideally that should have been a switch statement but we can change this later.

>+    delete(persisted.modalInfos);

delete is an operator and not a function. Just remove the braces.

>+        // Wait for the pane's content to load and to be fully displayed
>+        this._controller.waitFor(function() {
>+          return nodeToProcess.loaded &&
>+          (nodeToProcess.style.opacity == 1 || nodeToProcess.style.opacity == null);
>+        }, "The pane did not load in time: " + idSet.id);
> 
> oops - nearly forgot to add "|| nodeToProcess.style.opacity == null", as right
> after opening the preferences window, the value isn't set (thx Neil)

So why do we need this check in the callback function then? We have to wait until the pane has been loaded and as what I got from your statements, it's done when opacity is set to 1. So why checking for null?

The last item here is the reason for a r- because it's not clear for me. Also please send me a complete patch so I can test the complete functionality.

Comment 23

[Adrian Kalla [:adriank]]

Attachment: DOMWalker v8

(In reply to comment #22)
> Comment on attachment 490982 [details] [diff] [review]
> DOMWalker v7
> 
> >+  walk : function DOMWalker_walk(ids, root, waitFunction) {
> >+    if (typeof waitFunction == 'function')
> >+      this._controller.waitFor(waitFunction());
> 
> So what about ids.waitFunction here? Is that one forwarded to the walk function
> of sub windows?

ids is the array of sub-windows here. The waitFunction here is intended for the main window.

> >+    // QuerySelector seems to be unusuale for id's in this case:
> >+    // https://developer.mozilla.org/En/Code_snippets/QuerySelector
> >+    if (idSet.getBy == DOMWalker.GET_BY_ID) {
> >+      return doc.getElementById(idSet[idSet.getBy]);
> >+    } else if (idSet.getBy == DOMWalker.GET_BY_SELECTOR) {
> >+      return doc.querySelector(idSet[idSet.getBy]);
> >+    } else {
> >+      throw new Error("Not supported getBy-attribute: " + idSet.getBy);
> >+    }
> 
> Ideally that should have been a switch statement but we can change this later.

changed it now to not forget it. In _prepareTargetWindows I also changed the if..else to switch.

> >+    delete(persisted.modalInfos);
> 
> delete is an operator and not a function. Just remove the braces.

didn't know that... and didn't get an error here either... But fixed now.

> >+        // Wait for the pane's content to load and to be fully displayed
> >+        this._controller.waitFor(function() {
> >+          return nodeToProcess.loaded &&
> >+          (nodeToProcess.style.opacity == 1 || nodeToProcess.style.opacity == null);
> >+        }, "The pane did not load in time: " + idSet.id);
> > 
> > oops - nearly forgot to add "|| nodeToProcess.style.opacity == null", as right
> > after opening the preferences window, the value isn't set (thx Neil)
> 
> So why do we need this check in the callback function then? We have to wait
> until the pane has been loaded and as what I got from your statements, it's
> done when opacity is set to 1. So why checking for null?

The problem here is, that directly after opening the preferences window nodeToProcess.style.opacity has no value, so is null. In the current tests, when the preferences window is opened before starting the DOMWalker, this doesn't matter. But if the preferences window will get opened from the DOMWalker itself in a test, nodeToProcess.style.opacity will be null, so the waitFor would time out.
That's the reason, why nodeToProcess.style.opacity == null is needed here.

I also added a check for Mac here - as the opacity thing happens only on Mac.
 
> The last item here is the reason for a r- because it's not clear for me. Also
> please send me a complete patch so I can test the complete functionality.

ok

Comment 24

[Adrian Kalla [:adriank]]

Attachment: DOMWalker v9

added the screenshotPath attribute to DOMWalker

Comment 25

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

Comment on attachment 491813 [details] [diff] [review]
DOMWalker v9

>+ * @param {string} screenshotPath
>+ *        Path to save screenshots under
>+ */
>+function DOMWalker(controller, callbackFilter, callbackNodeTest,
>+                   callbackResults, screenshotPath) {

As talked and agreed in our last 1-1 we didn't wanted to make this change now. Further the DOMWalker should never receive such an argument.

(In reply to comment #23)
> The problem here is, that directly after opening the preferences window
> nodeToProcess.style.opacity has no value, so is null. In the current tests,
> when the preferences window is opened before starting the DOMWalker, this
> doesn't matter. But if the preferences window will get opened from the
> DOMWalker itself in a test, nodeToProcess.style.opacity will be null, so the
> waitFor would time out.
> That's the reason, why nodeToProcess.style.opacity == null is needed here.

I thought that opacity == null means the preferences dialog hasn't been completely loaded yet and we have to wait for '1'? It confuses me now.

> I also added a check for Mac here - as the opacity thing happens only on Mac.

With all those changes to this code lately, I wonder how many other changes will have to be made here. Is this now the final check?

Comment 26

[Adrian Kalla [:adriank]]

(In reply to comment #25)
> Comment on attachment 491813 [details] [diff] [review]
> DOMWalker v9
> 
> >+ * @param {string} screenshotPath
> >+ *        Path to save screenshots under
> >+ */
> >+function DOMWalker(controller, callbackFilter, callbackNodeTest,
> >+                   callbackResults, screenshotPath) {
> 
> As talked and agreed in our last 1-1 we didn't wanted to make this change now.

To be honest, I understood this differently - only if it would be too much hassle, then as a follow up (that's what I had in my notes too). As this didn't require many changes, it is better to do it here and now then changing later APIs...

> Further the DOMWalker should never receive such an argument.

The other option would be to pass it to walk() and then to _prepareTargetWindows(), which seemed to be the worse option to me.

> (In reply to comment #23)
> > The problem here is, that directly after opening the preferences window
> > nodeToProcess.style.opacity has no value, so is null. In the current tests,
> > when the preferences window is opened before starting the DOMWalker, this
> > doesn't matter. But if the preferences window will get opened from the
> > DOMWalker itself in a test, nodeToProcess.style.opacity will be null, so the
> > waitFor would time out.
> > That's the reason, why nodeToProcess.style.opacity == null is needed here.
> 
> I thought that opacity == null means the preferences dialog hasn't been
> completely loaded yet and we have to wait for '1'? It confuses me now.

you are talking about opacity == 0, not opacity == null. opacity == null happens only on the start of the preferences window - and remains null *until* a switch to another pane.

> > I also added a check for Mac here - as the opacity thing happens only on Mac.
> 
> With all those changes to this code lately, I wonder how many other changes
> will have to be made here. Is this now the final check?

I think that's it. I wasn't able to test it on Vista or Win7, as I have just XP installed, so I cannot assure that it works there. But under normal circumstances, it should.

Comment 27

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

(In reply to comment #26)
> > As talked and agreed in our last 1-1 we didn't wanted to make this change now.
> 
> To be honest, I understood this differently - only if it would be too much
> hassle, then as a follow up (that's what I had in my notes too). As this didn't
> require many changes, it is better to do it here and now then changing later
> APIs...

So we have a conflict here. While moving the screenshot functions into its own module is fine, all the new code which has been introduced by the latest patch to the DOMWalker, breaks the matter of this class. I don't want to see such bogus workarounds in the code.

If you want to follow this route, I have another option for you. Given that the tests will be run by the command line, we can have a '--screenshot" option for the automation script. This option can be passed via the persisted object. Depending on it the callback can decide itself if a screenshot has to be taken or not. The location will be included.

> > Further the DOMWalker should never receive such an argument.
> 
> The other option would be to pass it to walk() and then to
> _prepareTargetWindows(), which seemed to be the worse option to me.

Those are still members of the DOMWalker.

> > > I also added a check for Mac here - as the opacity thing happens only on Mac.
> > 
> > With all those changes to this code lately, I wonder how many other changes
> > will have to be made here. Is this now the final check?
> 
> I think that's it. I wasn't able to test it on Vista or Win7, as I have just XP
> installed, so I cannot assure that it works there. But under normal
> circumstances, it should.

Ok, so lets leave it as it is. I will check on all platforms, so I can report back.

Comment 28

[Adrian Kalla [:adriank]]

Attachment: DOMWalker v10

screenshot stuff removed

Comment 29

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

Comment on attachment 491873 [details] [diff] [review]
DOMWalker v10

>+        // Workaround for bug 606024. Works only on Mac, somehow
>+        // Until it isn't fixed, no errors will get caught on Linux and Windows
>+        // here.
>+        if (mozmill.isMac) {
>+          for (var j = 0; j < idSet.title.length; j++) {
>+            this._controller.keypress(dropDown, idSet.title[j], {});
>+            this._controller.sleep(100);
>+          };
>+          this._controller.keypress(dropDown, "VK_RETURN", {});
>+        } else {
>+          this._controller.select(dropDown, null, idSet.title);
>+          this._controller.waitFor(function() {
>+            return nodeToProcess.label == idSet.title;
>+          }, "The menu item did not load in time: " + idSet.title);
>+        }

Can you please tell me which type of error you are referring to? I'm aware of the select problem, but what's up with Windows and Linux?

Comment 30

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

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 31

[Adrian Kalla [:adriank]]

(In reply to comment #29)
> Can you please tell me which type of error you are referring to? I'm aware of
> the select problem, but what's up with Windows and Linux?

I mean with that, what the select bug causes: there won't be any L10n errors/bugs found where it is being used. A good example is the Polish Fx 4.0b6: in the privacy pane in the custom settings on Mac duplicated access keys will be found. On Windows and Linux not, even if they are there too.

I honestly don't know, why the Mac workaround does not work on this two platforms, as it should. But at least does using select() not break the tests completely.

Comment 32

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

On Linux and Windows you are not using the workaround from OS X. Does select really work as expected on those platforms?

Comment 33

[Adrian Kalla [:adriank]]

(In reply to comment #32)
> On Linux and Windows you are not using the workaround from OS X. Does select
> really work as expected on those platforms?

I'm not using it there, as the workaround is not working there.
And select does NOT work like expected anywhere...

Comment 34

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

keypress should always work. It doesn't depend on which platform you are. So it could only be a timing issue. How have you debugged this problem? And btw. haven't hear before from this problem.

Comment 35

[Adrian Kalla [:adriank]]

Attachment: DOMWalker v11

(In reply to comment #34)
> keypress should always work. It doesn't depend on which platform you are. So it
> could only be a timing issue. How have you debugged this problem? And btw.
> haven't hear before from this problem.

The problem was quite strange. Looks like my waitFor check which I did have in there while debugging this in the first place was causing Firefox to think, that we didn't stop typing... Putting a sleep call of at least 1 sec seems to solve the problem on Windows and Linux. Another option could be raising the check-interval for the waitFor from 100ms to 1000ms, but I think it is not possible to specify it manually in 1.5.1 anymore, or is it?

Comment 36

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

Comment on attachment 492855 [details] [diff] [review]
DOMWalker v11

>+        for (var j = 0; j < idSet.title.length; j++) {
>+          this._controller.keypress(dropDown, idSet.title[j], {});
>+          this._controller.sleep(50);
>+        };

As our experience shows using a value of 50ms can be too short. As long as we have to sleep here, please use 100ms.

>+        // The app needs to know, that we stopped typing. To do that 
>+        // on Mac it is enough to hit return. On Linux & Windows that would
>+        // save and exit the pref window. As The below waitFor interval lets
>+        // the app think, we didn't stop typing, we need to sleep first.
>+        if (mozmill.isMac) {
>+          this._controller.keypress(dropDown, "VK_RETURN", {});
>+        } else {
>+          this._controller.sleep(1000);
>+        }

So what you should do instead, is simply to synthesize a mouse click at the select node. This will open the popup, allows the keypresses, and will not close the dialog when pressing return. The above code will still leave the popup in a visible state which could cause problems.

Comment 37

[Adrian Kalla [:adriank]]

Attachment: DOMWalker v12 [checked-in]

going back to controller.select(), as it actually does work properly (I'll explain the reason in Bug 606024).

I'll open two follow-up bugs:
1) to use the value instead of label for controller.select()
2) to properly test the "custom" dialog of the Privacy preferences pane (the L10n tests will need one additional entry in the ids-sections)

Comment 38

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

Comment on attachment 493362 [details] [diff] [review]
DOMWalker v12 [checked-in]

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

Lets check what has to be done for the backports.

Comment 39

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

Adrian, could you please give me feedback about the backports? When can we expect patches for 1.9.2 and 1.9.1? Would be kinda helpful when we could check-in those in the next days.

Comment 40

[Adrian Kalla [:adriank]]

Attachment: DOMWalker for 1.9.2

DOMWalker backport to 1.9.2

Comment 41

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

Comment on attachment 494072 [details] [diff] [review]
DOMWalker for 1.9.2

Please fix the order of the function and your class. All have to be in the same order across branches.

Comment 42

[Adrian Kalla [:adriank]]

Attachment: DOMWalker for 1.9.2 v2 [checked-in]

(In reply to comment #41)
> Comment on attachment 494072 [details] [diff] [review]
> DOMWalker for 1.9.2
> 
> Please fix the order of the function and your class. All have to be in the same
> order across branches.

oops. Looks like merge played me here... Fixed.

Comment 43

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

Comment on attachment 494356 [details] [diff] [review]
DOMWalker for 1.9.2 v2 [checked-in]

Landed on 1.9.2:
http://hg.mozilla.org/qa/mozmill-tests/rev/ba528df40c5a

Comment 44

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

Adrian, please take bug 615493 into account for the 1.9.1 backport.

Comment 45

[Adrian Kalla [:adriank]]

since Bug 621013 did land now, we can close this bug (there won't be a 1.9.1 backport of this shared-module).