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)
Mozilla QA Graveyard
Mozmill Tests
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.
Updated•12 years ago
|
Flags: in-moztrap?
Updated•12 years ago
|
Flags: in-moztrap? → in-qa-testsuite?
Updated•11 years ago
|
Flags: in-qa-testsuite? → in-qa-testsuite?(hskupin)
Comment 1•11 years ago
|
||
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
Comment 2•11 years ago
|
||
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
Comment 3•11 years ago
|
||
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]
Updated•11 years ago
|
Whiteboard: [l10n] → [l10n] s=130318 u=failure c=l10n p=1
Updated•11 years ago
|
Whiteboard: [l10n] s=130318 u=failure c=l10n p=1 → [l10n] s=130318 u=new c=l10n p=1
Comment 4•11 years ago
|
||
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.
Updated•11 years ago
|
Assignee: nobody → dpetrovici
Status: NEW → ASSIGNED
Comment 5•11 years ago
|
||
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)
Comment 6•11 years ago
|
||
(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)
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
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.
Comment 9•11 years ago
|
||
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 10•11 years ago
|
||
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 11•11 years ago
|
||
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-
Comment 12•11 years ago
|
||
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)
Comment 13•11 years ago
|
||
(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.
Updated•11 years ago
|
Attachment #730166 -
Flags: feedback?(hskupin)
Attachment #730166 -
Flags: feedback?(dave.hunt)
Comment 14•11 years ago
|
||
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 15•11 years ago
|
||
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+
Comment 16•11 years ago
|
||
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
Comment 17•11 years ago
|
||
(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.
Comment 18•11 years ago
|
||
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 19•11 years ago
|
||
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-
Updated•11 years ago
|
Attachment #761364 -
Flags: review?(hskupin)
Comment 20•11 years ago
|
||
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-
Comment 21•11 years ago
|
||
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 22•11 years ago
|
||
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 23•11 years ago
|
||
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 24•11 years ago
|
||
Modified patch based on feedback. Reports are: Linux: http://mozmill-crowd.blargon7.com/#/l10n/report/a1b02004612785c13cf7c6bf1ea54f95 MAC: http://mozmill-crowd.blargon7.com/#/l10n/report/a1b02004612785c13cf7c6bf1ea588b8 Windows: http://mozmill-crowd.blargon7.com/#/l10n/report/a1b02004612785c13cf7c6bf1ea5acf2
Attachment #769698 -
Attachment is obsolete: true
Attachment #770787 -
Flags: review?(hskupin)
Attachment #770787 -
Flags: review?(andreea.matei)
Comment 25•11 years ago
|
||
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 26•11 years ago
|
||
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 27•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #770787 -
Flags: review+ → review-
Assignee | ||
Comment 28•11 years ago
|
||
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)
Comment 29•11 years ago
|
||
Cosmin, when you have time please check if this still applies and add myself and Andrei as reviewers as well. Thanks.
Assignee | ||
Comment 30•11 years ago
|
||
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 31•11 years ago
|
||
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 32•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #777670 -
Flags: review-
Assignee | ||
Comment 33•11 years ago
|
||
(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 34•11 years ago
|
||
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-
Assignee | ||
Comment 35•11 years ago
|
||
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 36•11 years ago
|
||
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-
Assignee | ||
Comment 37•11 years ago
|
||
Hi, I updated the patch, and made reports. Reports: http://mozmill-crowd.blargon7.com/#/l10n/report/2f1ca72938985fc4b989b7efccc52ab2 http://mozmill-crowd.blargon7.com/#/l10n/report/2f1ca72938985fc4b989b7efccc533bd http://mozmill-crowd.blargon7.com/#/l10n/report/2f1ca72938985fc4b989b7efccc523af
Attachment #8333810 -
Attachment is obsolete: true
Attachment #8335182 -
Flags: review?(andrei.eftimie)
Attachment #8335182 -
Flags: review?(andreea.matei)
Comment 38•10 years ago
|
||
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-
Assignee | ||
Comment 39•10 years ago
|
||
Hi Andrei, I updated the patch due to new directory structure. Reports: http://mozmill-crowd.blargon7.com/#/l10n/report/0bbed480c5898d9d02bca5d179b39ecb http://mozmill-crowd.blargon7.com/#/l10n/report/0bbed480c5898d9d02bca5d179b402a4 http://mozmill-crowd.blargon7.com/#/l10n/report/0bbed480c5898d9d02bca5d179b3ab56
Attachment #8335182 -
Attachment is obsolete: true
Attachment #8343626 -
Flags: review?(andrei.eftimie)
Attachment #8343626 -
Flags: review?(andreea.matei)
Assignee | ||
Comment 40•10 years ago
|
||
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 41•10 years ago
|
||
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-
Assignee | ||
Comment 42•10 years ago
|
||
(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 43•10 years ago
|
||
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-
Assignee | ||
Comment 44•10 years ago
|
||
Thanks for review Andrei, I made the changes. http://mozmill-crowd.blargon7.com/#/l10n/report/eeab4d15c588c5e6f00ab97aa525bfa5 http://mozmill-crowd.blargon7.com/#/l10n/report/eeab4d15c588c5e6f00ab97aa525c136 http://mozmill-crowd.blargon7.com/#/l10n/report/eeab4d15c588c5e6f00ab97aa525ce88
Attachment #8343645 -
Attachment is obsolete: true
Attachment #8358392 -
Attachment is obsolete: true
Attachment #8358392 -
Flags: review?(hskupin)
Attachment #8399871 -
Flags: review?(andrei.eftimie)
Attachment #8399871 -
Flags: review?(andreea.matei)
Comment 45•10 years ago
|
||
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+
Assignee | ||
Comment 46•10 years ago
|
||
Thanks for review Andrei!
Attachment #8399871 -
Attachment is obsolete: true
Attachment #8399958 -
Flags: review?(hskupin)
Assignee | ||
Comment 47•10 years ago
|
||
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 48•10 years ago
|
||
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-
Assignee | ||
Comment 49•10 years ago
|
||
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)
Assignee | ||
Comment 50•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8498816 -
Attachment is obsolete: true
Attachment #8498816 -
Flags: review?(andrei.eftimie)
Attachment #8498816 -
Flags: review?(andreea.matei)
Comment 51•10 years ago
|
||
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-
Assignee | ||
Comment 52•10 years ago
|
||
Done that, thanks Andreea :)
Attachment #8498821 -
Attachment is obsolete: true
Attachment #8499454 -
Flags: review?(andrei.eftimie)
Attachment #8499454 -
Flags: review?(andreea.matei)
Comment 53•10 years ago
|
||
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-
Assignee | ||
Comment 54•10 years ago
|
||
(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)
Assignee | ||
Updated•10 years ago
|
Attachment #8499454 -
Attachment is obsolete: true
Comment 55•10 years ago
|
||
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-
Assignee | ||
Comment 56•10 years ago
|
||
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)
Updated•10 years ago
|
Whiteboard: [l10n] s=130318 u=new c=l10n p=1 → [l10n][sprint]
Assignee | ||
Updated•10 years ago
|
Attachment #8501000 -
Flags: review?(andreea.matei)
Comment 57•10 years ago
|
||
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-
Comment 58•10 years ago
|
||
(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.
Assignee | ||
Comment 59•10 years ago
|
||
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-
Comment 60•10 years ago
|
||
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 61•10 years ago
|
||
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-
Assignee | ||
Comment 62•10 years ago
|
||
(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 63•10 years ago
|
||
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 64•10 years ago
|
||
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+
Assignee | ||
Comment 65•10 years ago
|
||
(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 66•10 years ago
|
||
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-
Assignee | ||
Comment 67•10 years ago
|
||
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 68•10 years ago
|
||
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+
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox36:
--- → fixed
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•