Closed Bug 799149 Opened 12 years ago Closed 10 years ago

Add mozmill test to ensure that there are no conflicts in context menu access keys.

Categories

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

defect

Tracking

(firefox36 fixed)

RESOLVED FIXED
Tracking Status
firefox36 --- fixed

People

(Reporter: tetsuharu, Assigned: cosmin-malutan)

References

Details

(Whiteboard: [l10n][sprint])

Attachments

(1 file, 24 obsolete files)

12.64 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
As the result of fixing bug 565717, we need to add a new mozmill test to check no conflicts in context menu access keys.
Flags: in-moztrap?
Flags: in-moztrap? → in-qa-testsuite?
Flags: in-qa-testsuite? → in-qa-testsuite?(hskupin)
We haven't seen this request given that it has been filed in the wrong component. Moving it to our area so we can figure out what's necessary to do here.

Andreea, can you please figure out what would be necessary here? Thank you!
Component: Menus → Mozmill Tests
Flags: in-qa-testsuite?(hskupin)
Priority: -- → P2
Product: Firefox → Mozilla QA
QA Contact: hskupin
Version: Trunk → unspecified
Hm, talked to Mihaela in QA team, but apparently there's no moztrap testcase for this.

I have looked over bug 565717 and I believe our steps should be:

1. Open a page with input fields (normal and password) - could be our password manager one [1]
2. Type a word into the normal field, select it and open context menu
3. Check available options are as giving in the changeset [2]
4. Type a word into the password field, select it and open context menu
5. Check available options are as giving in the changeset [3]

Don't think it's required to test each option as well, please correct me if I'm wrong.

[1] http://www.mozqa.com/data/firefox/password_manager/login_form.html
[2] https://hg.mozilla.org/try/rev/2dfefb840b77#l3.41
[3] https://hg.mozilla.org/try/rev/2dfefb840b77#l3.59
So it seems to me that we need a full set of context menu checks for Firefox. That's something we haven't started yet at all. But having those checks for the contentareacontext menu for the browser window would be a good start.

There are tons of different paths we have to trap when doing those checks. Would be good to know which are the most important ones. Beside the default one for web pages, and links, I would take examples like those to prevent regressions.

Axel, what do you think?
Whiteboard: [l10n]
Whiteboard: [l10n] → [l10n] s=130318 u=failure c=l10n p=1
Whiteboard: [l10n] s=130318 u=failure c=l10n p=1 → [l10n] s=130318 u=new c=l10n p=1
I know litte about the context menu and how it works, so I can't say much about which scenarios imply other scenarios, to get a minimal test suite. I think that's more of a question for a Firefox guy, like Gavin.
Assignee: nobody → dpetrovici
Status: NEW → ASSIGNED
Gavin, can you please tell us what are the most important paths we need for checking the context menu access keys? Please see comment #3.
Flags: needinfo?(gavin.sharp)
(In reply to Henrik Skupin (:whimboo) [away 03/15 - 03/22] from comment #3)
> There are tons of different paths we have to trap when doing those checks.
> Would be good to know which are the most important ones. Beside the default
> one for web pages, and links, I would take examples like those to prevent
> regressions.

Those sound like good first steps. http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/test_contextmenu.html has a more comprehensive (though not complete) list of other combinations.
Flags: needinfo?(gavin.sharp)
Attached patch patch v1.0 (obsolete) — Splinter Review
This is just work in progress as I am not sure if I am doing this properly:
1) do we need to also check that the access keys work properly in this test?
2) does the check for the access keys is correctly done in this test
Attachment #726670 - Flags: feedback?(hskupin)
Attachment #726670 - Flags: feedback?(dave.hunt)
Comment on attachment 726670 [details] [diff] [review]
patch v1.0

Testing that the accesskey matches the value from the DTD isn't that useful, really. The goal here is to check that for specific invocations of the menu, there are no duplicate access keys. So just checking that all of the access keys are unique would be sufficient.
Attached patch patch v2.0 (obsolete) — Splinter Review
This is still partial work, but I have modified the previous patch to test that the access keys are unique inside the context menu for: input text field, password field, image link, link

These two tests use local pages. For some of the other tests (e.g. mailto link context menu) we can use mozqa.com. Can you please tell me if this is ok?

Also, should I add all the tests in a single patch or should I create patches for every file?
Attachment #726670 - Attachment is obsolete: true
Attachment #726670 - Flags: feedback?(hskupin)
Attachment #726670 - Flags: feedback?(dave.hunt)
Attachment #728181 - Flags: feedback?(hskupin)
Attachment #728181 - Flags: feedback?(dave.hunt)
Comment on attachment 728181 [details] [diff] [review]
patch v2.0

>diff --git a/lib/utils.js b/lib/utils.js

>+function checkContextMenuAccessKeys(aNodes) {

>+  arrayAccessKeys.filter(function(aItem, index, arrayAccessKeys){
>+    var tmpIndex = arrayAccessKeys.indexOf(aItem);
>+    assert.equal(index, tmpIndex, aItem + " access key position has been found");

I don't think this code is doing what you intend. Here's an alternative that I think would work:

var nodeArray = Array.slice(aNodes);
var seenKeys = {};
nodeArray.forEach(function (n) {
  let accessKey = n.accessKey;
  if (n.nodeName !== "menuitem" || n.hidden || !accessKey)
    return;
  if (accessKey in seenKeys)
    fail("menu item " + n.id + " duplicates access key from menu item " + seenKeys[accessKey]); 
  seenKeys[accessKey] = n.id;
});
Comment on attachment 728181 [details] [diff] [review]
patch v2.0

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

First, this type of test does not belong to the functional testrun but has to be a l10n test. See the whiteboard entry on that bug. So it has to look similar as the existent accesskey test for the preference dialog. Other comment you can find inline.

::: lib/utils.js
@@ +147,5 @@
> + * @param {Object} aNodes
> + *        Context menu items for which the access keys' uniqueness is evaluated
> + */
> +
> +function checkContextMenuAccessKeys(aNodes) {

Checks for access keys are unique for l10n tests. So any shared code will have to be added to the localization.js module.

Also why do we need a separate method here. I would like to see that we can add it to the DOMWalker code so it gets transparently called.
Attachment #728181 - Flags: feedback?(hskupin)
Attachment #728181 - Flags: feedback?(dave.hunt)
Attachment #728181 - Flags: feedback-
Attached patch patch v3.0 (obsolete) — Splinter Review
Modified patch based on feedback. 

Also, the prepareAccessKey had an issue: we were transforming all the access key's values to lower case. In case we have "t" and "T" access keys, the test reported that both are the same which was not correct. I have also changed this in this patch

There is also a bug regarding the content menu access keys for Nightly IT version:
- accessKey: "A" is found in: context-undo, spell-add-dictionaries-main 
- accessKey: "k" is found in: context-openlinkprivate, context-setDesktopBackground

I have manually checked and this issue reproduces.
Attachment #728181 - Attachment is obsolete: true
Attachment #730166 - Flags: feedback?(hskupin)
Attachment #730166 - Flags: feedback?(dave.hunt)
(In reply to Daniela Petrovici from comment #12)
> Also, the prepareAccessKey had an issue: we were transforming all the access
> key's values to lower case. In case we have "t" and "T" access keys, the
> test reported that both are the same which was not correct. I have also
> changed this in this patch

No, that's not correct. Please revert this change. It really doesn't matter if the value is lower or upper case. Both keys are treated as the same value. This conversion exists by purpose.
Attachment #730166 - Flags: feedback?(hskupin)
Attachment #730166 - Flags: feedback?(dave.hunt)
Attached patch patch v3.1 (obsolete) — Splinter Review
Reverted the change to prepareAccessKeys method. 

This means that there is another issue with Nightly IT version for accessKey: "t" found in: context-cut and context-inspect
Attachment #730166 - Attachment is obsolete: true
Attachment #730176 - Flags: feedback?(hskupin)
Attachment #730176 - Flags: feedback?(dave.hunt)
Comment on attachment 730176 [details] [diff] [review]
patch v3.1

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

Looks better now. Thank you for the update. See my inline comments for further details.

::: lib/localization.js
@@ +198,5 @@
> + * @param {Object} aNode
> + *        Node on which filtering is performed
> + * @returns {Number} Filter status of the given node
> + */
> +function filterContextMenuKeys(aNode) {

Why do you invent a new method here and don't stick the code in the already existent filterAccessKeys() method? Also this would apply to all kind of menu nodes not only context menus.

::: tests/l10n/manifest.ini
@@ +1,3 @@
>  [include:testAccessKeys/manifest.ini]
>  [include:testCropped/manifest.ini]
> +[include:testContextMenuAccessKeys/manifest.ini]

As you can see in the folder layout we have already two sub folders here. Given that this is about access keys, the test has to be folded into the testAccessKeys folder.

::: tests/l10n/testContextMenuAccessKeys/test1.js
@@ +17,5 @@
> +
> +const URL_LOGIN_FORM = "http://www.mozqa.com/data/firefox/password_manager/" +
> +                       "login_form.html";
> +const URL_MOZILLA_COMMUNITY = "http://www.mozqa.com/data/firefox/layout/" +
> +                              "mozilla_community.html"

All those test cases exist locally. Why are you using remote pages here? Also I would propose we create a new test page with all available HTML elements present for testing.

@@ +53,5 @@
> +  // all the other cases will work by right clicking on the element
> +  if (aType === "input") {
> +    controller.type(aElement, INPUT_FIELD_VALUE);
> +    controller.doubleClick(aElement);
> +  }

This doesn't belong into this method and should be moved to the appropriate place. Also why a double click? I don't see a difference between the visible entries for a text selection or not. Entries are only disabled. If the distinction is important we might need two separate tests then.

@@ +61,5 @@
> +                                         localization.filterContextMenuKeys,
> +                                         localization.prepareAccessKey,
> +                                         localization.checkAccessKeysResults);
> +
> +  var contextMenu = new elementslib.ID(controller.window.document, "contentAreaContextMenu");

Please make use of the MenuAPI of Mozmill here.
Attachment #730176 - Flags: feedback?(hskupin)
Attachment #730176 - Flags: feedback?(dave.hunt)
Attachment #730176 - Flags: feedback+
Marking as dependent per previous comment (we need to create an HTML page that contains all the elements we need in this test)
Depends on: 857551
(In reply to Henrik Skupin (:whimboo) from comment #15)
> Comment on attachment 730176 [details] [diff] [review]
> patch v3.1
> > +  var contextMenu = new elementslib.ID(controller.window.document, "contentAreaContextMenu");
> 
> Please make use of the MenuAPI of Mozmill here.
I have tried using the MenuAPI of Mozmill here and got the contextMenu element as described here: https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/test/test_menu.js#L5

If I am using it like this, the next line in this patch would need to use a private method: _menu. So the line would be:
var contextMenuNode = contextMenu._menu.getNode();

Do we still want to use Menu API if doing so would mean using a private method?

NOTE: I have also looked at how we get this context menu in other places in mozmill-tests and it seems we get it with ID - e.g. http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/utils.js#l151.
Attached patch patch v4.0 (obsolete) — Splinter Review
Based on discussion with Henrik on IRC, Menu API should not be used for this test.

Reports for passing runs:
MAC: http://mozmill-crowd.blargon7.com/#/l10n/report/8aa237803fd4b0e7348adc8bbecfc64d
Windows: http://mozmill-crowd.blargon7.com/#/l10n/report/8aa237803fd4b0e7348adc8bbecfc34e
Linux: http://mozmill-crowd.blargon7.com/#/l10n/report/8aa237803fd4b0e7348adc8bbecfc0e7
Attachment #730176 - Attachment is obsolete: true
Attachment #761364 - Flags: review?(andreea.matei)
Comment on attachment 761364 [details] [diff] [review]
patch v4.0

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

We usually put the elements in libraries, but giving the nature of this test, I'm not sure if it's worth it, if we're going to use that page in other tests as well. Henrik, what would you suggest?

Also, can we make this more compact? I see most checks use: 
getElement + checkContextMenu() call or
getElement + doubleClick() + checkContextMenu() call.
Could we make use of arrays to separate these cases and then go with a forEach through them?

::: tests/l10n/testAccessKeys/test2.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * This test tests the Context Menu for duplicated access keys.

This comment can be moved before the test function.

@@ +111,5 @@
> +  var testObject = new elementslib.ID(controller.tabs.activeTab, "test-object");
> +  checkContextMenu(testObject);
> +}
> +
> +function checkContextMenu(aElement) {

Wouldn't be best to have this in the utils lib?
Attachment #761364 - Flags: review?(andreea.matei) → review-
Attachment #761364 - Flags: review?(hskupin)
Comment on attachment 761364 [details] [diff] [review]
patch v4.0

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

::: tests/l10n/testAccessKeys/test2.js
@@ +11,5 @@
> +var domUtils = require("../../../lib/dom-utils");
> +var localization = require("../../../lib/localization");
> +var utils = require("../../../lib/utils");
> +
> +const TEST_DATA = "http://mozqa.com/data/firefox/layout/html_elements.html";

Why a remote location? This should be local.

@@ +26,5 @@
> +  var flashNotification = new elementslib.ID(controller.window.document,
> +                          "plugin-install-notification-icon");
> +  if (flashNotification.exists()) {
> +    controller.keypress(null, "VK_ESCAPE", {});
> +  }

Is there a preference we can set to disable the plugin finder service?

@@ +33,5 @@
> +  controller.doubleClick(textArea);
> +  checkContextMenu(textArea);
> +
> +  var canvas = new elementslib.ID(controller.tabs.activeTab, "test-canvas");
> +  checkContextMenu(canvas);

I would really love to see a data driven test as we have for the preferences dialog.

@@ +111,5 @@
> +  var testObject = new elementslib.ID(controller.tabs.activeTab, "test-object");
> +  checkContextMenu(testObject);
> +}
> +
> +function checkContextMenu(aElement) {

No but anything which would be necessary should be part of the localization library.

@@ +118,5 @@
> +  controller.rightClick(aElement);
> +  var domWalker = new domUtils.DOMWalker(controller,
> +                                         localization.filterAccessKeys,
> +                                         localization.prepareAccessKey,
> +                                         localization.checkAccessKeysResults);

I don't see why we would have to create a new instance of the DOMWalker for each check we have to perform. Please check how this works with the data-driven approach.
Attachment #761364 - Flags: review?(hskupin) → review-
Attached patch patch v5.0 (obsolete) — Splinter Review
Modified patch based on review

I have requested feedback again since I modified the test to be data driven per the previous comment and changed the domWalker to work for content windows, too.
Attachment #761364 - Attachment is obsolete: true
Attachment #769698 - Flags: feedback?(hskupin)
Attachment #769698 - Flags: feedback?(dave.hunt)
Comment on attachment 769698 [details] [diff] [review]
patch v5.0

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

This approach looks good to me. I've added some comments that should be addressed before requesting review.

::: tests/l10n/testAccessKeys/test2.js
@@ +28,5 @@
> +  var ids = [
> +    { getBy : GET_BY_ID,
> +      id : "test-textarea",
> +      preHook : doDoubleClickBefore,
> +      postHook : closeContextMenu,

The postHook is always closeContextMenu, so I don't see a need for this to be included here.

@@ +32,5 @@
> +      postHook : closeContextMenu,
> +      },
> +    { getBy : GET_BY_ID,
> +      id : "test-canvas",
> +      preHook : openContextMenu,

The openContextMenu is always called, and isn't really a preHook. We should call this directly in the test and only give preHook a value when it's needed.

@@ +162,5 @@
> +function closeContextMenu() {
> +  utils.closeContentAreaContextMenu(controller);
> +}
> +
> +function doDoubleClickBefore() {

I would drop 'before' from these methods and remove the openContextMenu call. It can be done directly from the test as all parameters require this.

@@ +169,5 @@
> +
> +  return openContextMenu(aElement);
> +}
> +
> +function doSelectBefore() {

As above.
Attachment #769698 - Flags: feedback?(hskupin)
Attachment #769698 - Flags: feedback?(dave.hunt)
Attachment #769698 - Flags: feedback+
Comment on attachment 769698 [details] [diff] [review]
patch v5.0

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

::: tests/l10n/testAccessKeys/test2.js
@@ +155,5 @@
> +                                         localization.prepareAccessKey,
> +                                         localization.checkAccessKeysResults,
> +                                         false);
> +
> +  domWalker.walk(ids);

As Daniella has pointed out on IRC, the postHook is called from within domWalker.walk, so if we're to reduce the amount of duplication setting the pre/post hooks, we could add them before we call domWalker.walk. We could set postHook to closeContextMenu and set preHook to openContextMenu if it's not already defined.

@@ +162,5 @@
> +function closeContextMenu() {
> +  utils.closeContentAreaContextMenu(controller);
> +}
> +
> +function doDoubleClickBefore() {

On reflection, perhaps rename these to doubleClickAndOpenContextMenu therefore being more descriptive to the actions taking place.
Comment on attachment 770787 [details] [diff] [review]
patch v6.0

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

I like these changes, thanks Daniela. I'll leave the review request for Henrik though as I'd like him to have another look at it before it lands.
Comment on attachment 770787 [details] [diff] [review]
patch v6.0

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

Looks good for me as well.
Attachment #770787 - Flags: review?(andreea.matei) → review+
Comment on attachment 770787 [details] [diff] [review]
patch v6.0

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

In general it looks way better as before. Thanks Daniela. But we still have a fair amount of things to do. Please see my inline comments and questions.

::: lib/dom-utils.js
@@ +21,5 @@
>   *        callback-method to process the results
>   *        [optional - default: undefined]
> + * @param {Boolean} isContentWindow
> + *        Type of window: chrome or content
> + *        [optional - default: false]

Hm, I get the feeling that we should better pass in the document and not the controller. With that change we can easily determine if we are in chrome or content. That would also make the additional added parameter obsolete here.

@@ +30,5 @@
>    this._controller = controller;
>    this._callbackFilter = callbackFilter;
>    this._callbackNodeTest = callbackNodeTest;
>    this._callbackResults = callbackResults;
> +  this._isContentWindow = isContentWindow || false;

Also I would use isChrome here given that this is the default.

@@ +137,5 @@
>  
> +    // If we have Chrome window
> +    if (!this._isContentWindow) {
> +      if (!root)
> +        root = this._controller.window.document.documentElement;

I don't see why we have to differentiate that. Both chrome and content documents have a documentElement property. And we always need a valid root been specified.

@@ +150,5 @@
> +    }
> +    else {
> +      if (ids) {
> +        this._prepareElements(ids);
> +      }

Can't we integrate the checks into the _walk() method?

@@ +257,5 @@
> +   *                          windowHandler - Window instance
> +   *                                          [only needed for some tests]
> +   */
> +  _prepareElements : function DOMWalker_prepareElements(ids) {
> +    var doc = this._controller.tabs.activeTab;

You want to use this.document here, right?

@@ +289,5 @@
> +          assert.equal(typeof idSet.postHook, "function",
> +                       "postHook is not a function");
> +          idSet.postHook.call(node);
> +        }
> +      }

We should generalize _prepareTargetWindows() so we can also cover content. Would that be possible? I don't like this duplication.

::: tests/l10n/testAccessKeys/test2.js
@@ +40,5 @@
> +    { id : "test-image-in-iframe" },
> +    { id : "test-plugin" },
> +    { id : "test-object" },
> +    { id : "test-textarea",
> +      preHook : doubleClickAndOpenContextMenu },

I wouldn't do all of that as preHook, but add another property to the id entry like 'action' which let us specify what to do with the element. Only opening the context menu should stay in the preHook section, and we might need a postHook to close the context menu again.

@@ +79,5 @@
> +    aId.getBy = GET_BY_ID;
> +    aId.postHook = closeContextMenu;
> +    if ( !aId.preHook ) {
> +      aId.preHook = openContextMenu;
> +    }

Even it would cause duplication, please put this directly into the map. Otherwise it's too confusing because of all the different areas you have to check. See my comment from above re postHook. This part is hard to find on first sight.
Attachment #770787 - Flags: review?(hskupin) → review+
Attachment #770787 - Flags: review+ → review-
Attached patch patch_v7.patch (obsolete) — Splinter Review
I added the changes required in comment #27, except for replacing controller with document. 

(In reply to Henrik Skupin (:whimboo) from comment #27)
> Hm, I get the feeling that we should better pass in the document and not the
> controller. With that change we can easily determine if we are in chrome or
> content. That would also make the additional added parameter obsolete here.
We can add document as parameter too but we still need controller, because sometimes we use it like in _processNode method (http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/dom-utils.js#l388).

Reports
http://mozmill-crowd.blargon7.com/#/l10n/report/5aa1ca5e7015e3740d269dc947b5d287
http://mozmill-crowd.blargon7.com/#/l10n/report/5aa1ca5e7015e3740d269dc947b5d10d
http://mozmill-crowd.blargon7.com/#/l10n/report/5aa1ca5e7015e3740d269dc947b5d15d
Assignee: daniela.p98911 → cosmin.malutan
Attachment #777670 - Flags: review?(hskupin)
Cosmin, when you have time please check if this still applies and add myself and Andrei as reviewers as well. Thanks.
Comment on attachment 777670 [details] [diff] [review]
patch_v7.patch

Hi Andreea, it still applies, I also added you and Andrei as reviewers.
Attachment #777670 - Flags: review?(andrei.eftimie)
Attachment #777670 - Flags: review?(andreea.matei)
Comment on attachment 777670 [details] [diff] [review]
patch_v7.patch

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

Some nits to fix. Also, please make sure to test it with latest nightly, it passed some time since Daniela worked on this.

::: data/layout/html_elements.html
@@ +45,5 @@
> +        <td>Mailto link</td>
> +      </tr>
> +      <tr>
> +        <td><a id="test-link" href="http://mozilla.com">ClickTheLink</a></td>
> +        <td><a id="test-mailto" href="mailto:noone@mozilla.com">MailToTink</a></td>

MailToLink?

::: lib/dom-utils.js
@@ +429,5 @@
> +      let domWalker = new DOMWalker(this._controller,
> +                                    this._callbackFilter,
> +                                    this._callbackNodeTest,
> +                                    this._callbackResults, false);
> +       domWalker.walk(null, contextMenu.getNode());

This is off intended to the left with one space.

::: tests/l10n/testAccessKeys/test2.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

We need "use strict;"
Attachment #777670 - Flags: review?(hskupin)
Attachment #777670 - Flags: review?(andrei.eftimie)
Attachment #777670 - Flags: review?(andreea.matei)
Attachment #777670 - Flags: review-
Comment on attachment 777670 [details] [diff] [review]
patch_v7.patch

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

::: lib/dom-utils.js
@@ +21,3 @@
>   *        callback-method to process the results
>   *        [optional - default: undefined]
> + * @param {Boolean} isChrome

That name also needs the ´a´ prefix.

@@ +85,5 @@
>    /**
> +   * Returns the document based on the type of window
> +   *
> +   * @returns Document
> +   * @type {Document}

nit: On a single line please and please specify the exact type

@@ +92,5 @@
> +    if (this._isChrome) {
> +      return this._controller.window.document;
> +    }
> +
> +    return this._controller.tabs.activeTab;

Something which would read better:

return this._isChrome ? this._controller.window.document
                      : this._controller.tabs.activeTab;

@@ +422,5 @@
>        }
>      }
>  
> +    // Gets the context menu and runs DOMWalker.walk() for its options
> +    else if (!this._isChrome) {

Why is this for content only? We also have context menus in chrome.

::: tests/l10n/testAccessKeys/test2.js
@@ +167,5 @@
> +  domWalker.walk(ids);
> +}
> +
> +function closeContextMenu() {
> +  utils.closeContentAreaContextMenu(controller);

Please use the controller.Menu API here.

@@ +174,5 @@
> +function openContextMenu() {
> +  // Some elements need time to focus before any tests are run on them
> +  var element = new elementslib.Elem(this);
> +  this.focus();
> +  controller.rightClick(element);

Please use the controller.Menu API here.

@@ +178,5 @@
> +  controller.rightClick(element);
> +}
> +
> +function doubleClick() {
> +  var element = new elementslib.Elem(this);

What is this?

@@ +183,5 @@
> +  controller.doubleClick(element);
> +}
> +
> +function select() {
> +  var element = new elementslib.Elem(this);

What is this?
Attached patch patch_v7.1 (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #32)
> > +    // Gets the context menu and runs DOMWalker.walk() for its options
> > +    else if (!this._isChrome) {
> 
> Why is this for content only? We also have context menus in chrome.
In this block I treat only content elements, chrome elements are treated in above

> > +function doubleClick() {
> > +  var element = new elementslib.Elem(this);
> 
> What is this?
This is the node element on which domwalker will call the callback

Reports:
http://mozmill-crowd.blargon7.com/#/l10n/report/8a42e4b8f277f9c0268b061eb8f540b9
http://mozmill-crowd.blargon7.com/#/l10n/report/8a42e4b8f277f9c0268b061eb8f53803
http://mozmill-crowd.blargon7.com/#/l10n/report/8a42e4b8f277f9c0268b061eb8f52f95
Attachment #770787 - Attachment is obsolete: true
Attachment #777670 - Attachment is obsolete: true
Attachment #825402 - Flags: review?(andrei.eftimie)
Attachment #825402 - Flags: review?(andreea.matei)
Comment on attachment 825402 [details] [diff] [review]
patch_v7.1

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

Please see my review comments inline. There are still bogus lines which have to be updated.

Generally please do not completely flip reviewers but keep the one who reviewed a patch on a bug at least once in the loop. Thanks.

::: lib/dom-utils.js
@@ +21,4 @@
>   *        callback-method to process the results
>   *        [optional - default: undefined]
> + * @param {Boolean} aIsChrome
> + *        True if the window is chrome

Please let it default to true, given that in most of our cases we are working with chrome.

@@ +29,5 @@
> +  this._controller = aController;
> +  this._callbackFilter = aCallbackFilter;
> +  this._callbackNodeTest = aCallbackNodeTest;
> +  this._callbackResults = aCallbackResults;
> +  this._aIsChrome = aIsChrome;

Why this change to _aIsChrome? Please revert.

@@ +84,5 @@
>  
>    /**
> +   * Returns the document based on the type of window
> +   *
> +   * @returns {Document}

It still misses the description.

@@ +418,5 @@
>        }
>      }
>  
> +    // Gets the context menu and runs DOMWalker.walk() for its options
> +    else if (!this._aIsChrome) {

This is not the right way of handling. The whole if construct is based on various kinds of node types. Here you mix it up to all types in content.

@@ +425,5 @@
> +      let domWalker = new DOMWalker(this._controller,
> +                                    this._callbackFilter,
> +                                    this._callbackNodeTest,
> +                                    this._callbackResults, false);
> +       domWalker.walk(null, contextMenu.getNode());

Why are you setting up a complete new domWalker instance? I don't think that this is necessary at all.

::: tests/l10n/testAccessKeys/test1.js
@@ +208,5 @@
>    var domWalker = new domUtils.DOMWalker(controller,
>                                           localization.filterAccessKeys,
>                                           localization.prepareAccessKey,
> +                                         localization.checkAccessKeysResults,
> +                                         true);

We can skip this change, once aIsChrome defaults to true.

::: tests/l10n/testAccessKeys/test2.js
@@ +150,5 @@
> +  return ids;
> +}
> +
> +/**
> + * This test tests the Context Menu for duplicated access keys.

'test tests' ready hard, please update. Also mention which context menu. We have dozen of them.

@@ +181,5 @@
> +
> +function openContextMenu() {
> +  // Some elements need time to focus before any tests are run on them
> +  var element = new elementslib.Elem(this);
> +  this.focus();

'need time to focus' does not apply to the above code. You are forcing the focus here. Also I still dislike the usage of 'this'. This callback has a parameter, so please use it! Same for all the other hooks.

@@ +183,5 @@
> +  // Some elements need time to focus before any tests are run on them
> +  var element = new elementslib.Elem(this);
> +  this.focus();
> +  controller.getMenu("#contentAreaContextMenu",
> +                     controller.window.document).open(element);

There is no need to specify the chrome document. It's used by default via the controller object.
Attachment #825402 - Flags: review?(andrei.eftimie)
Attachment #825402 - Flags: review?(andreea.matei)
Attachment #825402 - Flags: review-
Attached patch patch v7.2 (obsolete) — Splinter Review
Hi, Henrik, here is an updated patch, I changed the nits except for the last one:

> (In reply to Henrik Skupin (:whimboo) from comment #32)
> Also I still dislike the usage of 'this'. This callback has a
> parameter, so please use it! Same for all the other hooks.
I don't like it either but when a function is called with .call() the first parameter is the context (this),  so the parameter sent from dom-walker instance is actually the context here. To remove the .call() and send an actual parameter will imply to change all tests that use the dom-walker. 

http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/dom-utils.js#l250
>if ('preHook' in idSet) {
>  assert.equal(typeof idSet.preHook, "function",
>  "preHook is not a function");
>  idSet.preHook.call(node);
>}

Here is how is used in an already existing test, where this is the node parameter in the function above
http://hg.mozilla.org/qa/mozmill-tests/file/a09dd08a9e9d/tests/l10n/testAccessKeys/test1.js#l226
>function disableResetDialogCall() {
>  var aCommand = this.getAttribute("oncommand");
>  this.setAttribute("data-command-backup", aCommand);
>  this.setAttribute("oncommand", "gPrivacyPane.updateHistoryModePane();
>  gPrivacyPane.updateHistoryModePrefs();
>  gPrivacyPane.updatePrivacyMicroControls();");
>}
Attachment #825402 - Attachment is obsolete: true
Attachment #8333810 - Flags: review?(hskupin)
Attachment #8333810 - Flags: review?(andrei.eftimie)
Attachment #8333810 - Flags: review?(andreea.matei)
Comment on attachment 8333810 [details] [diff] [review]
patch v7.2

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

::: lib/dom-utils.js
@@ +21,3 @@
>   *        callback-method to process the results
>   *        [optional - default: undefined]
> + * @param {Boolean} [aIsChrome=true] - True if the window is chrome

Description bellow please.

@@ +353,5 @@
> +                                           "contentAreaContextMenu");
> +      this.walk(null, contextMenu.getNode());
> +
> +      // Because we are not in the chrome document we leave the function here
> +      return;

"Exit the function since it is not a chrome document"

::: tests/l10n/testAccessKeys/test2.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Include the required modules

"use strict"

@@ +171,5 @@
> +  controller.getMenu("#contentAreaContextMenu").close();
> +}
> +
> +function openAdobeFlashContextMenu() {
> +  //  Shockwave Flash -doesn't open an contentAreaContextMenu

nit: "a contextAreaContextMenu". And whitespace after "-".
Attachment #8333810 - Flags: review?(hskupin)
Attachment #8333810 - Flags: review?(andrei.eftimie)
Attachment #8333810 - Flags: review?(andreea.matei)
Attachment #8333810 - Flags: review-
Comment on attachment 8335182 [details] [diff] [review]
patch_v7.3

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

Please update the patch to reflect the new folder structure.
It doesn't apply anymore.
Attachment #8335182 - Flags: review?(andrei.eftimie)
Attachment #8335182 - Flags: review?(andreea.matei)
Attachment #8335182 - Flags: review-
Attached patch patch_v7.5 (obsolete) — Splinter Review
Sorry but I have gave the patch for beta:
Here is the patch for nightly, reports:
http://mozmill-crowd.blargon7.com/#/l10n/report/0bbed480c5898d9d02bca5d179be09e8
http://mozmill-crowd.blargon7.com/#/l10n/report/0bbed480c5898d9d02bca5d179be1844
http://mozmill-crowd.blargon7.com/#/l10n/report/0bbed480c5898d9d02bca5d179be0854
Attachment #8343626 - Attachment is obsolete: true
Attachment #8343626 - Flags: review?(andrei.eftimie)
Attachment #8343626 - Flags: review?(andreea.matei)
Attachment #8343645 - Flags: review?(andrei.eftimie)
Attachment #8343645 - Flags: review?(andreea.matei)
Comment on attachment 8343645 [details] [diff] [review]
patch_v7.5

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

Seems to work fine.

::: data/layout/html_elements.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +  <head>
> +    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">

This can be simplified to: <meta charset="utf-8">

@@ +9,5 @@
> +    </style>
> +  </head>
> +  <body>
> +    <h2>Element needed in order to test the context menu</h2>
> +    <h4> Text area</h4>

nit: leading whitespace

@@ +17,5 @@
> +    <canvas id="test-canvas" width="200" height="100" style="background-color: turquoise"></canvas>
> +
> +    <h4>Different types of inputs</h4>
> +    <input id="test-input-spellcheck" spellcheck="true" autofocus value="testInputWithSpellcheck"></input>
> +    <br />

nit: single tags don't _need_ to be closed (if you strongly want them closed I won't object)

::: firefox/tests/l10n/testAccessKeys/test2.js
@@ +145,5 @@
> +      getBy : GET_BY_ID,
> +      target : WINDOW_CURRENT,
> +      action : select,
> +      preHook : openContextMenu,
> +      postHook : closeContextMenu }

I'd like to make this declarations more DRY.

The only different thing is the `id` (and the `action`) parameter.

I propose to make a default object:
> var defaults = {
>   getBy : GET_BY_ID,
>   target : WINDOW_CURRENT,
>   preHook : openContextMenu,
>   postHook : closeContextMenu
> }

And extend that with the difference for each object:
> var ids = [
>   utils.extend(defaults, { id : "test-canvas" }),
>   utils.extend(defaults, { id : "test-input" }),
>   utils.extend(defaults, { id : "test-image" }),

Since there is no native way to extend objects properties we'll need our own (this would help in other places as well).
I've investigated a bit on how different js libraries achieve this.

Here is an adapted version[1]. It can extend multiple objects at once:
> function extend(obj) {
>   Array.prototype.slice.call(arguments, 1).map(function(source) {
>     if (source) {
>       for (var prop in source) {
>         obj[prop] = source[prop];
>       }
>     }
>   });
>   return obj;
> }

[1] This is basically underscore.js' implementation.
https://github.com/jashkenas/underscore/blob/6c102a76dbebe7b4da0c245a734483af6df644c9/underscore.js#L839-L848
I've looked at how different js libraries solved this problem, and this solution is by far the most elegant.
Underscore it MIT licensed, but I have no idea if we can use this as is or not.


**
We'd reduce this from 150LOC to less then 50LOC and we would be DRY
Attachment #8343645 - Flags: review?(andrei.eftimie)
Attachment #8343645 - Flags: review?(andreea.matei)
Attachment #8343645 - Flags: review-
Attached patch patch_v8.0 (obsolete) — Splinter Review
(In reply to Andrei Eftimie from comment #41)
> > +      getBy : GET_BY_ID,
> > +      target : WINDOW_CURRENT,
> > +      action : select,
> > +      preHook : openContextMenu,
> > +      postHook : closeContextMenu }
> 
> I'd like to make this declarations more DRY.
> The only different thing is the `id` (and the `action`) parameter.
> I propose to make a default object.
> And extend that with the difference for each object:
> > var ids = [
> >   utils.extend(defaults, { id : "test-canvas" }),
> >   utils.extend(defaults, { id : "test-input" }),
> >   utils.extend(defaults, { id : "test-image" }),
Hi I changed the method a bit so it will return a new object with the properties of all objects passed-in as parameters.

Reports:
http://mozmill-crowd.blargon7.com/#/l10n/report/0fc916b47806e5d11cba8af7dc39297d
http://mozmill-crowd.blargon7.com/#/l10n/report/0fc916b47806e5d11cba8af7dc3ad6b4
http://mozmill-crowd.blargon7.com/#/l10n/report/0fc916b47806e5d11cba8af7dc3c41c3
Attachment #8358392 - Flags: review?(hskupin)
Attachment #8358392 - Flags: review?(andrei.eftimie)
Attachment #8358392 - Flags: review?(andreea.matei)
Comment on attachment 8358392 [details] [diff] [review]
patch_v8.0

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

See inline an issue I have with the `action` hook and some nits.

I'm leaving the review flag for Henrik up since I want his take on the new extend method from utils.
I find it really handy for us to have this. There are numerous times I wanted to merge objects, and there is no native way in JS to do this.

::: firefox/lib/utils.js
@@ +253,5 @@
>    clipboard.copyString("");
>  }
>  
>  /**
> + * Returns an extended object of all objects pased-in as parameters

Please add a jsdoc comment for the @return value so we're in sync with the rest of the functions here.

::: firefox/tests/l10n/testAccessKeys/test2.js
@@ +17,5 @@
> +const WINDOW_CURRENT = domUtils.DOMWalker.WINDOW_CURRENT;
> +
> +const PREF_MISSING_FLASH = "plugins.notifyMissingFlash";
> +
> +const DEFAULT_HTML_ELEMENT = { getBy : GET_BY_ID,

nit: remove space between bracket and key

@@ +33,5 @@
> +}
> +
> +function htmlElementsInit() {
> +  var ids = [
> +    utils.extend(DEFAULT_HTML_ELEMENT, {id: "test-canvas"}),

nit: please be consistent with spacing: between the brackets and the key, between the key and the colon and between the value and the closing bracket

::: lib/dom-utils.js
@@ +255,5 @@
>        if (node) {
>          var idSet = ids[i];
>  
> +        // Action - what to do with the element
> +        if ('action' in idSet) {

I feel strongly against this.

That's why we have a hook, this is an unnecessary duplication of what a hook is.
If we need multiple hooks we should send along an array of functions as `preHook` and execute all of them in order.
Attachment #8358392 - Flags: review?(andrei.eftimie)
Attachment #8358392 - Flags: review?(andreea.matei)
Attachment #8358392 - Flags: review-
Comment on attachment 8399871 [details] [diff] [review]
patch_v8.1

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

Just a small improvement mentioned inline.

This looks good to me. r+ with the mentioned item.
Please ask a review from Henrik.

::: lib/dom-utils.js
@@ +29,5 @@
> +  this._controller = aController;
> +  this._callbackFilter = aCallbackFilter;
> +  this._callbackNodeTest = aCallbackNodeTest;
> +  this._callbackResults = aCallbackResults;
> +  this._IsChrome = (typeof aIsChrome !== "undefined") ? aIsChrome : true;

Lets also cast this as a boolean here.
Attachment #8399871 - Flags: review?(andrei.eftimie)
Attachment #8399871 - Flags: review?(andreea.matei)
Attachment #8399871 - Flags: review+
Attached patch patch_v8.2 (obsolete) — Splinter Review
Thanks for review Andrei!
Attachment #8399871 - Attachment is obsolete: true
Attachment #8399958 - Flags: review?(hskupin)
Attached patch patch v9 (obsolete) — Splinter Review
I had to update the patch because due to long delay it didn't applied anymore.

http://mozmill-crowd.blargon7.com/db/3dac09185ffafe61214f5a99be09a70f
Attachment #8399958 - Attachment is obsolete: true
Attachment #8399958 - Flags: review?(hskupin)
Attachment #8497481 - Flags: review?(andrei.eftimie)
Attachment #8497481 - Flags: review?(andreea.matei)
Comment on attachment 8497481 [details] [diff] [review]
patch v9

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

You missed to include the test...

::: lib/dom-utils.js
@@ +29,5 @@
>    this._controller = aController;
>    this._callbackFilter = aCallbackFilter;
>    this._callbackNodeTest = aCallbackNodeTest;
>    this._callbackResults = aCallbackResults;
> +  this._IsChrome = (typeof aIsChrome !== "undefined") ? !!aIsChrome : true;

nit: this was previously `_isChrome`, please don't capitalize the i

::: lib/utils.js
@@ +262,5 @@
> + */
> +function extend() {
> +  var object = {};
> +
> +  Array.prototype.slice.call(arguments, 0).map(function(aSource) {

nit: we can use =>
Attachment #8497481 - Flags: review?(andrei.eftimie)
Attachment #8497481 - Flags: review?(andreea.matei)
Attachment #8497481 - Flags: review-
Attached patch patch v10 (obsolete) — Splinter Review
Thanks for review, sorry for missing that. :D 

http://mozmill-crowd.blargon7.com/#/l10n/report/53ec3148fca67aea1b1c3528f9387172
Attachment #8497481 - Attachment is obsolete: true
Attachment #8498816 - Flags: review?(andrei.eftimie)
Attachment #8498816 - Flags: review?(andreea.matei)
Attached patch patch v10.1 (obsolete) — Splinter Review
I forgot about the nits.
http://mozmill-crowd.blargon7.com/#/l10n/report/53ec3148fca67aea1b1c3528f93a7f69
Attachment #8498821 - Flags: review?(andrei.eftimie)
Attachment #8498821 - Flags: review?(andreea.matei)
Attachment #8498816 - Attachment is obsolete: true
Attachment #8498816 - Flags: review?(andrei.eftimie)
Attachment #8498816 - Flags: review?(andreea.matei)
Comment on attachment 8498821 [details] [diff] [review]
patch v10.1

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

Just a few nits.

::: firefox/tests/l10n/testAccessKeys/test2.js
@@ +33,5 @@
> +}
> +
> +function htmlElementsInit() {
> +  var ids = [
> +    utils.extend(DEFAULT_HTML_ELEMENT, {id: "test-canvas"}),

This misses a whitespace after 'id'

::: lib/utils.js
@@ +257,5 @@
>  
> + /**
> + * Returns an extended object of all objects pased-in as parameters
> + *
> + * @return {Object} The extended object

Please use a lowercase for the types, everywhere.
Attachment #8498821 - Flags: review?(andrei.eftimie)
Attachment #8498821 - Flags: review?(andreea.matei)
Attachment #8498821 - Flags: review-
Attached patch patch v10.2 (obsolete) — Splinter Review
Done that, thanks Andreea :)
Attachment #8498821 - Attachment is obsolete: true
Attachment #8499454 - Flags: review?(andrei.eftimie)
Attachment #8499454 - Flags: review?(andreea.matei)
Comment on attachment 8499454 [details] [diff] [review]
patch v10.2

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

I'm missing the html_elements.html page... 
This one from testcase-data: http://www.mozqa.com/data/firefox/layout/html_elements.html is referenced locally, but we don't have it in the repository.

The test is passing even with no elements being run at all... this is concerning.
Please check what DOMWalker is doing as it doesn't return any failure...
Attachment #8499454 - Flags: review?(andrei.eftimie)
Attachment #8499454 - Flags: review?(andreea.matei)
Attachment #8499454 - Flags: review-
Attached patch patch v10.3 (obsolete) — Splinter Review
(In reply to Andrei Eftimie from comment #53)
> The test is passing even with no elements being run at all... this is
> concerning.
> Please check what DOMWalker is doing as it doesn't return any failure...

The page already includes other large remote items(video/audio/flash/xobject) so it will be pointless to add it in mozmill-tests to, I just used the correct link in TEST_DATA.

The reason no-one noticed we missed the items in the first place is because DOMWalker instead of making an assertion that the node exists it silently pass if the node doesn't exists(something we don't want form a QA state of mind) http://hg.mozilla.org/qa/mozmill-tests/file/f17ca5e4fae9/lib/dom-utils.js#l242 
I'll file a bug against DOMWalker, also when I addded an assertion there all other l10 tests failed so we surely miss other elements in other tests but that's out of the scope of this bug. 


http://mozmill-crowd.blargon7.com/#/l10n/report/41b4fea479d86c7288732f3ea863a3fb
Attachment #8499551 - Flags: review?(andrei.eftimie)
Attachment #8499551 - Flags: review?(andreea.matei)
Attachment #8499454 - Attachment is obsolete: true
Comment on attachment 8499551 [details] [diff] [review]
patch v10.3

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

::: firefox/tests/l10n/testAccessKeys/test2.js
@@ +30,5 @@
> +function teardownModule(aModule) {
> +  prefs.preferences.clearUserPref(PREF_MISSING_FLASH);
> +}
> +
> +function htmlElementsInit() {

Remove this as a helper function, as assign it directly to `ids` in the test.

@@ +48,5 @@
> +    utils.extend(DEFAULT_HTML_ELEMENT, {id : "test-plugin"}),
> +    utils.extend(DEFAULT_HTML_ELEMENT, {id : "test-object",
> +                                        preHook : openAdobeFlashContextMenu}),
> +    utils.extend(DEFAULT_HTML_ELEMENT, {id : "test-textarea",
> +                                        preHook : function () {

Please create a function (similar to the others).
Maybe call it something like `selectAndOpenContextMenu`.
Use that function as a prehook for this and all items which require a doubleClick before opening the context menu.

@@ +115,5 @@
> +
> +function openAdobeFlashContextMenu() {
> +  //  Shockwave Flash - doesn't open a contentAreaContextMenu
> +  var element = new elementslib.Elem(this);
> +  this.focus();

element.focus?

@@ +116,5 @@
> +function openAdobeFlashContextMenu() {
> +  //  Shockwave Flash - doesn't open a contentAreaContextMenu
> +  var element = new elementslib.Elem(this);
> +  this.focus();
> +  controller.rightClick(element);

element.rightClick()

@@ +124,5 @@
> +  var element = new elementslib.Elem(this);
> +  controller.getMenu("#contentAreaContextMenu").open(element);
> +}
> +
> +function doubleClick() {

This should be part of the new function as mentioned above.

@@ +126,5 @@
> +}
> +
> +function doubleClick() {
> +  var element = new elementslib.Elem(this);
> +  controller.doubleClick(element);

Use element.doubleClick()

@@ +129,5 @@
> +  var element = new elementslib.Elem(this);
> +  controller.doubleClick(element);
> +}
> +
> +function select() {

This is not used at all. Please remove.

::: lib/dom-utils.js
@@ +344,3 @@
>      var nodeToProcess = this._getNode(aIdSet);
>  
> +    if (!this._isChrome) {

So when we are not in a chrome document, we hardcode the use of contextMenu entries.
Please add a comment before the if clause to explain that we only test contextMenu items right now.
Attachment #8499551 - Flags: review?(andrei.eftimie)
Attachment #8499551 - Flags: review?(andreea.matei)
Attachment #8499551 - Flags: review-
Attached patch patch v11 (obsolete) — Splinter Review
Regarding DOMWalker issue I filed bug 1079210.

(In reply to Andrei Eftimie from comment #55)
> Comment on attachment 8499551 [details] [diff] [review]ook for this and all items which require a
> > +function openAdobeFlashContextMenu() {
> > +  //  Shockwave Flash - doesn't open a contentAreaContextMenu
> > +  var element = new elementslib.Elem(this);
> > +  this.focus();
> 
> element.focus?
This is the node sent as context by the DOMWalker, the MozElement ob jet doesn't have an focus method but DOM node.

Report:
http://mozmill-crowd.blargon7.com/#/l10n/report/78c2fb649de7649b6e328c2466373a50
Attachment #8499551 - Attachment is obsolete: true
Attachment #8501000 - Flags: review?(andrei.eftimie)
Whiteboard: [l10n] s=130318 u=new c=l10n p=1 → [l10n][sprint]
Attachment #8501000 - Flags: review?(andreea.matei)
Comment on attachment 8501000 [details] [diff] [review]
patch v11

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

This looks fine to me in general, but there are 2 issues right now that I would like addressed before we can land it. I'm going to give it r-, but because of the dependencies listed below.

1) we'll need to address bug 1081996 as the test fails with 90% rate for me on OSX (all l10n tests are failing for me)
If we won't fix this in mozmill (as it might take some time) we might find a way to fix it in mozmill-tests as a workaround for now
Have you seen this issue locally?
I don't see it affecting daily runs, but that might be because of:

2) bug 1079210 (I'll add this as a blocker). I can't confirm how many of the elements we test here are really getting tested, even If we screw up something this still reports all green
Attachment #8501000 - Flags: review?(andrei.eftimie)
Attachment #8501000 - Flags: review?(andreea.matei)
Attachment #8501000 - Flags: review-
Depends on: 1079210
(In reply to Andrei Eftimie from comment #57)
> 1) we'll need to address bug 1081996 as the test fails with 90% rate for me
> on OSX (all l10n tests are failing for me)
> If we won't fix this in mozmill (as it might take some time) we might find a
> way to fix it in mozmill-tests as a workaround for now
> Have you seen this issue locally?

The e10s prompt should not be a problem for mozmill anymore. This is already fixed and was part of the bustage release 2.0.8.
Comment on attachment 8501000 [details] [diff] [review]
patch v11

I just added a WIP patch in bug 1079210, you can apply it to check that it doesn't affect my test.
http://mozmill-crowd.blargon7.com/#/l10n/report/02345d0791a18ffc72a4b042ab592dbb

I'll mark the bug as blocked by 1082077, you can review it then.
Thanks
Attachment #8501000 - Flags: review?(andrei.eftimie)
Attachment #8501000 - Flags: review?(andreea.matei)
Attachment #8501000 - Flags: review-
Depends on: 1082077
Comment on attachment 8501000 [details] [diff] [review]
patch v11

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

Indeed this works fine, used an expect call like bug 1079210, and everything looks good.
I've also gotten better results in regards to bug 1081996 (this is landed but there's no mozmill 2.0.9 out yet). I still see the prompt, but it doesn't interfere with the l10n test anymore.
Given that we haven't see interference in CI testruns regarding this, it might not be a big issue right now.
Attachment #8501000 - Flags: review?(hskupin)
Attachment #8501000 - Flags: review?(andrei.eftimie)
Attachment #8501000 - Flags: review?(andreea.matei)
Attachment #8501000 - Flags: review+
Comment on attachment 8501000 [details] [diff] [review]
patch v11

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

::: firefox/tests/l10n/testAccessKeys/manifest.ini
@@ +1,4 @@
>  [parent:../manifest.ini]
>  
>  [test1.js]
> +[test2.js]

Please file a new bug so we can name the tests correctly. We can make all of them the new style of restart tests now.

::: firefox/tests/l10n/testAccessKeys/test2.js
@@ +19,5 @@
> +
> +const DEFAULT_HTML_ELEMENT = {getBy : GET_BY_ID,
> +                              target : WINDOW_CURRENT,
> +                              preHook : openContextMenu,
> +                              postHook : closeContextMenu};

Please format that as we usually do. See the coding style: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests/Mozmill_Style_Guide#Code_Blocks

@@ +31,5 @@
> +  prefs.preferences.clearUserPref(PREF_MISSING_FLASH);
> +}
> +
> +/**
> + * Tests the content area Context Menu(right-click) for duplicated access keys.

nit: context menu

@@ +52,5 @@
> +    utils.extend(DEFAULT_HTML_ELEMENT, {id : "test-video-in-iframe"}),
> +    utils.extend(DEFAULT_HTML_ELEMENT, {id : "test-image-in-iframe"}),
> +    utils.extend(DEFAULT_HTML_ELEMENT, {id : "test-plugin"}),
> +    utils.extend(DEFAULT_HTML_ELEMENT, {id : "test-object",
> +                                        preHook : openAdobeFlashContextMenu}),

Please fix the indentation in all those cases.

@@ +80,5 @@
> +  domWalker.walk(ids);
> +}
> +
> +function closeContextMenu() {
> +  controller.getMenu("#contentAreaContextMenu").close();

See below.

@@ +86,5 @@
> +
> +function openAdobeFlashContextMenu() {
> +  //  Shockwave Flash - doesn't open a contentAreaContextMenu
> +  var element = new elementslib.Elem(this);
> +  this.focus();

See below in regards of `this`.

@@ +92,5 @@
> +}
> +
> +function openContextMenu() {
> +  var element = new elementslib.Elem(this);
> +  controller.getMenu("#contentAreaContextMenu").open(element);

What is `this` here? Don't we pass anything into this method? Usually `this` is the function scope when the method is used as a closure. Lets add a jsdoc anyway to all of those callbacks.

Also I would suggest that you cache the menu. I don't see the need that we have to request it each time for open and close.

@@ +97,5 @@
> +}
> +
> +function selectAndOpenContextMenu() {
> +  var element = new elementslib.Elem(this);
> +  element.doubleClick();

Why do we need a double click? Can't we do that via .focus?

@@ +98,5 @@
> +
> +function selectAndOpenContextMenu() {
> +  var element = new elementslib.Elem(this);
> +  element.doubleClick();
> +  openContextMenu.call(this)

nit: missing semicolon.

::: lib/dom-utils.js
@@ +29,5 @@
>    this._controller = aController;
>    this._callbackFilter = aCallbackFilter;
>    this._callbackNodeTest = aCallbackNodeTest;
>    this._callbackResults = aCallbackResults;
> +  this._isChrome = (typeof aIsChrome !== "undefined") ? !!aIsChrome : true;

As given by the jsdoc aIsChrome should already be a boolean. No need for the double exclamation marks.

@@ +82,5 @@
>      return this._controller;
>    },
>  
>    /**
> +   * Returns the document based on the type of window in which we are

"Returns the document the code operates on"

@@ +84,5 @@
>  
>    /**
> +   * Returns the document based on the type of window in which we are
> +   *
> +   * @returns {document} - Either the current chrome or the current tab document.

Which type is document exactly? 

nit: no dash necessary, kill the second 'the current', and replace tab with content.

@@ +344,3 @@
>      var nodeToProcess = this._getNode(aIdSet);
>  
> +    // For content space only checks context menu items

nit: s/space/scope/, and 'only, check'

@@ +344,4 @@
>      var nodeToProcess = this._getNode(aIdSet);
>  
> +    // For content space only checks context menu items
> +    if (!this._isChrome) {

So what if we have content elements but don't want to check the context menu. How would you have to specify that in the aIDs array? Right now we would check that menu all the time, right?

@@ +345,5 @@
>  
> +    // For content space only checks context menu items
> +    if (!this._isChrome) {
> +      var contextMenu = new elementslib.ID(this._controller.window.document,
> +                                           "contentAreaContextMenu");

Shall we better cache the context menu?

@@ +346,5 @@
> +    // For content space only checks context menu items
> +    if (!this._isChrome) {
> +      var contextMenu = new elementslib.ID(this._controller.window.document,
> +                                           "contentAreaContextMenu");
> +      this.walk(null, contextMenu.getNode());

We don't need a wait callback here? Can we be sure the context menu is open?

::: lib/utils.js
@@ +255,5 @@
>    clipboard.copyString("");
>  }
>  
> + /**
> + * Returns an extended object of all objects pased-in as parameters

I would say "Create a combined object from properties of all passed-in objects".

@@ +257,5 @@
>  
> + /**
> + * Returns an extended object of all objects pased-in as parameters
> + *
> + * @return {object} The extended object

nit: @returns

@@ +259,5 @@
> + * Returns an extended object of all objects pased-in as parameters
> + *
> + * @return {object} The extended object
> + */
> +function extend() {

Maybe combineObjects?

@@ +262,5 @@
> + */
> +function extend() {
> +  var object = {};
> +
> +  Array.prototype.slice.call(arguments, 0).map((aSource) => {

nit: no need for brackets around aSource
Attachment #8501000 - Flags: review?(hskupin) → review-
Attached patch patch v12.0 (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #61)
> >  [test1.js]
> > +[test2.js]
> 
> Please file a new bug so we can name the tests correctly. We can make all of
> them the new style of restart tests now.
Bug 1087936

> > +function openContextMenu() {
> > +  var element = new elementslib.Elem(this);
> > +  controller.getMenu("#contentAreaContextMenu").open(element);
> 
> What is `this` here? Don't we pass anything into this method? Usually `this`
> is the function scope when the method is used as a closure. Lets add a jsdoc
> anyway to all of those callbacks.
I added a comment in bug regarding this, also I explained this in comment 33, comment 35 and comment 56, it's ugly but if we change it we will have to change all tests that uses DOMWalker, and that's out of scope of this bug. 


> > +
> > +function selectAndOpenContextMenu() {
> > +  var element = new elementslib.Elem(this);
> > +  element.doubleClick();
> 
> Why do we need a double click? Can't we do that via .focus?
No, focus does something completely different, we use this so we can select a word(like it does when you duble-click on a word in page), and it opens the context menu for that word not for the page itself. 


> > +  this._isChrome = (typeof aIsChrome !== "undefined") ? !!aIsChrome : true;
> 
> As given by the jsdoc aIsChrome should already be a boolean. No need for the
> double exclamation marks.
I reverted this, there is no need but also there is no harm in this, is a matter of taste, Andrei suggested this in comment 45.

> > +    // For content space only checks context menu items
> > +    if (!this._isChrome) {
> 
> So what if we have content elements but don't want to check the context
> menu. How would you have to specify that in the aIDs array? Right now we
> would check that menu all the time, right?
Yes, this is haw we handle the other specific situations menulist/pref/tab, if at some point we would like to walk something else specifically in content we can add a different condition then, that's why I don't catch the context menu before we get in to this condition.

> @@ +346,5 @@
> > +    // For content space only checks context menu items
> > +    if (!this._isChrome) {
> > +      var contextMenu = new elementslib.ID(this._controller.window.document,
> > +                                           "contentAreaContextMenu");
> > +      this.walk(null, contextMenu.getNode());
> 
> We don't need a wait callback here? Can we be sure the context menu is open?
No we use the Menu class and it waits for menu to open itself:
https://github.com/mozilla/mozmill/blob/8cad8438b6f6237f449add334f97d731448baff7/mozmill/mozmill/extension/resource/driver/controller.js#L108 



http://mozmill-crowd.blargon7.com/#/l10n/report/5560c4beef382ba39c25005635222336
Attachment #8501000 - Attachment is obsolete: true
Attachment #8510258 - Flags: review?(andrei.eftimie)
Attachment #8510258 - Flags: review?(andreea.matei)
Comment on attachment 8510258 [details] [diff] [review]
patch v12.0

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

I see the changes were implemented.
Attachment #8510258 - Flags: review?(hskupin)
Attachment #8510258 - Flags: review?(andrei.eftimie)
Attachment #8510258 - Flags: review?(andreea.matei)
Attachment #8510258 - Flags: review+
Comment on attachment 8510258 [details] [diff] [review]
patch v12.0

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

r=me with the inline comments fixed.

::: firefox/tests/l10n/testAccessKeys/test2.js
@@ +34,5 @@
> +  prefs.preferences.clearUserPref(PREF_MISSING_FLASH);
> +}
> +
> +/**
> + * Tests the content area context menu(right-click) for duplicated access keys.

nit: missing space before the brackets, whereby I think context menu should be self-explanatory.

@@ +100,5 @@
> +  domWalker.walk(ids);
> +}
> +
> +// All functions below will be called from DOMWalker, which does that
> +// with function.call(nodeElement) so "this" is the element being processed

Please file another bug so we can get this fixed. It's an easy one, and shouldn't need that much work. But it would drastically help for clarity here. Thanks.

nit: 'is the currently processed DOM node'.

@@ +117,5 @@
> +  var element = new elementslib.Elem(this);
> +  contextMenu.open(element);
> +}
> +
> +function selectAndOpenContextMenu() {

So please rename this callback to `selectTextAndOpenContextMenu` then. Otherwise it's unclear what select means, as you have seen by my last review already.

::: lib/dom-utils.js
@@ +348,5 @@
> +      var contextMenu = new elementslib.ID(this._controller.window.document,
> +                                           "contentAreaContextMenu");
> +      this.walk(null, contextMenu.getNode());
> +
> +      // Exit the function since it is not a chrome document

nit: Return early, because all remaining checks are for chrome scope.

::: lib/utils.js
@@ +562,2 @@
>  exports.checkSearchField = checkSearchField;
>  exports.createURI = createURI;

Please fix the ordering of the 3 lines above.
Attachment #8510258 - Flags: review?(hskupin) → review+
Attached patch patch v12.1 (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #64)
> Please file another bug so we can get this fixed. It's an easy one, and
> shouldn't need that much work. But it would drastically help for clarity
> here. Thanks.
Done, bug 1094745

Report: http://mozmill-crowd.blargon7.com/#/l10n/report/1e5e44bea8f3e17708194a929a97e948
Attachment #8510258 - Attachment is obsolete: true
Attachment #8518097 - Flags: review?(andrei.eftimie)
Comment on attachment 8518097 [details] [diff] [review]
patch v12.1

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

A couple updates, otherwise looks good.

::: firefox/tests/l10n/testAccessKeys/test2.js
@@ +107,5 @@
> +}
> +
> +function openAdobeFlashContextMenu() {
> +  //  Shockwave Flash - doesn't open a contentAreaContextMenu
> +  var element = new elementslib.Elem(this);

Please use findElement throughout the test.

@@ +109,5 @@
> +function openAdobeFlashContextMenu() {
> +  //  Shockwave Flash - doesn't open a contentAreaContextMenu
> +  var element = new elementslib.Elem(this);
> +  this.focus();
> +  element.rightClick(element);

Shouldn't this be `element.rightClick();` ?

::: lib/dom-utils.js
@@ +345,5 @@
>  
> +    // For content scope only, check context menu items
> +    if (!this._isChrome) {
> +      var contextMenu = new elementslib.ID(this._controller.window.document,
> +                                           "contentAreaContextMenu");

Please use findElement
Attachment #8518097 - Flags: review?(andrei.eftimie) → review-
Attached patch patch v12.2Splinter Review
Thanks for review Andrei.
I fixed those nits ;)
Attachment #8518097 - Attachment is obsolete: true
Attachment #8518703 - Flags: review?(andrei.eftimie)
Attachment #8518703 - Flags: review?(andreea.matei)
Comment on attachment 8518703 [details] [diff] [review]
patch v12.2

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

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/4d5e955f36d7 (default)
Attachment #8518703 - Flags: review?(andrei.eftimie)
Attachment #8518703 - Flags: review?(andreea.matei)
Attachment #8518703 - Flags: review+
Attachment #8518703 - Flags: checkin+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.