If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Focus inside the select method is not set properly on the menulist

RESOLVED FIXED

Status

Testing Graveyard
Mozmill
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: Daniela Petrovici, Assigned: AndreeaMatei)

Tracking

Details

(Whiteboard: [mozmill-2.0][mozmill-1.5.22+][ateamtrack: p=mozmill q=2013q3 m=2])

Attachments

(4 attachments, 8 obsolete attachments)

1.82 KB, text/plain
Details
3.98 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
1.84 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
11.26 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
When selecting an item from the menulist on MAC OS, the focus is not set properly on the element (see comment 44 in bug 865640). This happens both in Mozmill 1.5.21 and 2.0. 

The select code in Mozmill 2.0 will be changed in bug 860662, but this issue still reproduces with the patch from that bug.
(Reporter)

Updated

4 years ago
Whiteboard: [mozmill 2.0?]
(Reporter)

Comment 1

4 years ago
Created attachment 748706 [details]
testcase

This is the minimized testcase for mozmill 2.0
(Reporter)

Comment 2

4 years ago
This issue is depended on bug 860662 since the fix Andreea did in 2.0 will fix this issue to.

Do we want the fix from the above bug in mozmill 1.5 too?
Blocks: 865640
Depends on: 860662
(Reporter)

Comment 3

4 years ago
Actually the code in bug 860662 does not fully fix the issue with the menu list so removing the dependency
No longer depends on: 860662
Does the attached testcase also fail with Mozmill 1.5? I wonder why we haven't seen this before with 1.5. What's different with the menulist in the add-on manager? That's interesting.
(Reporter)

Comment 5

4 years ago
Created attachment 749779 [details]
testcase that is failing on both mozmill 1.5 and 2.0

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Does the attached testcase also fail with Mozmill 1.5? 
The previous testcase did not fail in 1.5 mozmill. I have added the test case that is failing in both mozmill 1.5 and 2.0 now.

> I wonder why we haven't seen this before with 1.5. What's different with the menulist in the add-on manager? That's interesting.
I think that we haven't seen this before in 1.5 because we are not selecting the option by label only in any test. I have looked in our repo and we are only using controller.select with the value, not label.
Attachment #748706 - Attachment is obsolete: true

Updated

4 years ago
Whiteboard: [mozmill 2.0?] → [mozmill 2.0?][sprint2013-32]

Updated

4 years ago
Assignee: nobody → andrei.eftimie
(Reporter)

Updated

4 years ago
Assignee: andrei.eftimie → dpetrovici
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Whiteboard: [mozmill 2.0?][sprint2013-32] → [mozmill 2.0?][sprint2013-32][mozmill 1.5?]
Whiteboard: [mozmill 2.0?][sprint2013-32][mozmill 1.5?] → [mozmill-2.0?][mozmill-1.5?][sprint2013-32]
(Reporter)

Comment 6

4 years ago
Created attachment 757465 [details]
simplified test case

This is a more complete test case (simulating the deactivation and activation of the plugin). I will also add a patch for all the changes I made in mozmill in a moment.

The investigation is below:
Used Andreea's patch for select() method and Andrei's patch for EventUtils.js
When running the test with no other changes in mozmill - error:

    "message": "this.element.getElementsByTagName(...)[0] is undefined", 
    "fileName": "resource://mozmill/driver/mozelement.js", 
    "name": "TypeError", 
    "lineNumber": 616
line is: menuitems = this.element.getElementsByTagName("menupopup")[0].getElementsByTagName("menuitem");
This works if we add "xul:menupop" as tagName

After changing the above, new error is:
    "message": "No item selected for element Elem: [object XULElement]", 
    "fileName": "resource://mozmill/driver/mozelement.js", 
    "name": "Error", 
    "lineNumber": 673
The problem is the getElementsByTagName("menuitem") in above line
  - xul:menupop -> xul:hbox -> xul:menuitem
So changing the above line to: 
menuitems = this.element.getElementsByTagName("menupopup")[0].getElementsByTagName("xul:menuitem")

fixes this issue. So, the remaining issue is:

    "message": "Value has been selected", 
    "fileName": "resource://mozmill/stdlib/utils.js", 
    "name": "TimeoutError", 
    "lineNumber": 243
The problem is that the value is still not selected
So, I tried: 
   EventUtils.synthesizeMouse(this.element.selectedItem, 1, 1, {}, ownerDoc.defaultView); 
instead of 
   EventUtils.synthesizeMouse(this.element, 1, 1, {}, ownerDoc.defaultView);

and this fixes the test (testDeactivate), but does not work for the second one (testActivate). I tested on MAC OS 10.7.5
Attachment #749779 - Attachment is obsolete: true
Flags: needinfo?(hskupin)
(Reporter)

Comment 7

4 years ago
Created attachment 757466 [details] [diff] [review]
patch with dumps and changes to mozmill
Comment on attachment 757466 [details] [diff] [review]
patch with dumps and changes to mozmill

If still applicable please update a possible patch and flag for feedback, once you have taken my upcoming comment into account.
Attachment #757466 - Attachment is obsolete: true
(In reply to Daniela Petrovici from comment #6)
> This works if we add "xul:menupop" as tagName

This might be necessary because we have an in-content chrome window. Because of that the 'xul:' namespace is in use. But we cannot simply change this by adding the namespace because this will most likely fail in other cases e.g. when we have a real chrome window like the preferences dialog. Have you tried to run its tests with your changes applied? I believe that those will fail.

That means we have to detect which of content we show and have to add the XUL namespace as prefix when trying to find elements by tag name.
Flags: needinfo?(hskupin)
(Reporter)

Comment 10

4 years ago
Created attachment 762017 [details] [diff] [review]
patch v1.0

Added patch with feedback per comment #8, but this patch does not fix the issue.

I took into account comment #9 and it seems that we do not need to change the tag. We only need to use getElementByTagNameNS and add the correct namespace. This will not break the mutt tests although it will only solve a part of the issue for the simplified TC.

The simplified TC still fails. I tried to use focus before clicking on the element and on the item, but this doesn't fix it.

Part of the issue is still solved by using EventUtils.synthesizeMouse(this.element.selectedItem, 1, 1, {}, ownerDoc.defaultView);
This is not the correct approach though since it breaks both the mutt tests and the next test for activating the plugin.

Do you have any suggestions on how to proceed with the investigation?
Attachment #762017 - Flags: feedback?(hskupin)
Comment on attachment 762017 [details] [diff] [review]
patch v1.0

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

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +629,5 @@
>          // Unwrap the XUL element's XPCNativeWrapper
>          this.element = utils.unwrapNode(this.element);
>          // Get the list of menuitems
> +        menuitems = this.element.getElementsByTagNameNS(NAMESPACE_XUL, "menupopup")[0].
> +                    getElementsByTagNameNS(NAMESPACE_XUL, "menuitem");

I'm concerned with this change. I would be more inclined to take it if we would have tests for any combination where an element could live in. If all those cases would still work, I'm happy to take it. So please create a testcase for those situations.
Attachment #762017 - Flags: feedback?(hskupin) → feedback-
(In reply to Daniela Petrovici from comment #10)
> Part of the issue is still solved by using
> EventUtils.synthesizeMouse(this.element.selectedItem, 1, 1, {},
> ownerDoc.defaultView);

Which part of the issue is solved by using this call?

> This is not the correct approach though since it breaks both the mutt tests
> and the next test for activating the plugin.

What's happening and why is it not the right approach? Can you please clarify?
(Reporter)

Comment 13

4 years ago
(In reply to Henrik Skupin (:whimboo) from comment #12)
> (In reply to Daniela Petrovici from comment #10)
> > Part of the issue is still solved by using
> > EventUtils.synthesizeMouse(this.element.selectedItem, 1, 1, {},
> > ownerDoc.defaultView);
> 
> Which part of the issue is solved by using this call?
testDeactivate from the simplified test case passes when using this call. So, this will fix the disable plugin test, but not the re-enable.

> > This is not the correct approach though since it breaks both the mutt tests
> > and the next test for activating the plugin.
> 
> What's happening and why is it not the right approach? Can you please
> clarify?
I was referring to the fact that using this.element.selectItem instead of this.element in synthesizeMouse fixes the plugin disabling, but it breaks the mutt test (which fails to select the element now) and also it is not fixing the enabling of the plugin.
(In reply to Daniela Petrovici from comment #13)
> > What's happening and why is it not the right approach? Can you please
> > clarify?
> I was referring to the fact that using this.element.selectItem instead of
> this.element in synthesizeMouse fixes the plugin disabling, but it breaks
> the mutt test (which fails to select the element now) and also it is not
> fixing the enabling of the plugin.

We should indeed click on the selected item when choosing it from the list. Clicking on the main menu list could cause strange behavior. That's what you do manually too, right? Seems like there has to be done some more investigation here. Also keep in mind that you can always check which element has the focus via document.activeElement.
(Reporter)

Comment 15

4 years ago
(In reply to Henrik Skupin (:whimboo) from comment #14)
> We should indeed click on the selected item when choosing it from the list.
> Clicking on the main menu list could cause strange behavior. That's what you
> do manually too, right? 
We are first clicking on the menulist to open it then select an item from the list. The change I made was related to opening the menulist by using its selectedItem. I think that if we try to open the main menulist by clicking on the selectedItem, it will fail in cases where we do not have a selected item.

> Seems like there has to be done some more
> investigation here. Also keep in mind that you can always check which
> element has the focus via document.activeElement.
I will continue the investigation.
(Reporter)

Comment 16

4 years ago
Created attachment 771570 [details] [diff] [review]
patch v2.0

The problem was the offset which was set in synthesizeMouse and was not correct. Changing synthesize mouse to click which uses mouseEvent and calculates the offsetX and offsetY works properly.

Per comment #11, I have also added in the existing mutt test for menulist the case where the menulist is xul:menulist and the patch works with all three types of menulists where an item can reside.
Attachment #762017 - Attachment is obsolete: true
Attachment #771570 - Flags: feedback?(hskupin)
Comment on attachment 771570 [details] [diff] [review]
patch v2.0

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

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +768,4 @@
>  
>          // Click the item
>          try {
> +          this.element.click();

We do not want to use the internal click() command. This is not triggering the event from the outside. So please revert this change.

::: mutt/mutt/tests/js/elementslib/menulist.js
@@ +64,5 @@
> +  controller.window.document.addEventListener("ViewChanged",
> +                                               onViewChanged, false);
> +
> +  var plugin = new elementslib.ID(controller.window.document, "category-plugin");
> +  controller.click(plugin);

I'm afraid of all that usage of the addons manager here. Why cannot we use https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/content/test/chrome_elements.xul for that? This are also chrome elements put into content space.
Attachment #771570 - Flags: feedback?(hskupin) → feedback-
(In reply to Henrik Skupin (:whimboo) from comment #17)
> >          // Click the item
> >          try {
> > +          this.element.click();
> 
> We do not want to use the internal click() command. This is not triggering
> the event from the outside. So please revert this change.
> 
Because EventUtils.synthesizeMouse, is not working on properly on mac, we have to use EventUtils.synthesizeMouseAtCenter so it will pass the simplified test case, and will unlock Bug 865640
> ::: mutt/mutt/tests/js/elementslib/menulist.js
> @@ +64,5 @@
> > +  controller.window.document.addEventListener("ViewChanged",
> > +                                               onViewChanged, false);
> > +
> > +  var plugin = new elementslib.ID(controller.window.document, "category-plugin");
> > +  controller.click(plugin);
> 
> I'm afraid of all that usage of the addons manager here. Why cannot we use
> https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/
> content/test/chrome_elements.xul for that? This are also chrome elements put
> into content space.
If I change the last test to use https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/content/test/chrome_elements.xul, it will be exactly as testChromeSelect, so my question is, should I change the testXULMenuList, to use the xul file that you proposed, live it as it is or remove it from patch?
Flags: needinfo?(hskupin)
Assignee: daniela.p98911 → cosmin.malutan
(In reply to Cosmin Malutan from comment #18)
> Because EventUtils.synthesizeMouse, is not working on properly on mac, we
> have to use EventUtils.synthesizeMouseAtCenter so it will pass the
> simplified test case, and will unlock Bug 865640

Why does it not work correctly only on Mac? What is the problem? You might want to be clearer. There shouldn't be really a difference between both methods except that the latter clicks into the middle of an element. Which x/y coordinates have you used for testing?

But wait. Looks like I missed that this is actually for Mozmill 2.0 and element.click() is actually executing MozMillElement.click(). So that would be fine. Sorry.

> If I change the last test to use
> https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/
> content/test/chrome_elements.xul, it will be exactly as testChromeSelect, so
> my question is, should I change the testXULMenuList, to use the xul file
> that you proposed, live it as it is or remove it from patch?

Well, you might want to check if testChromeSelect satisfies the needs for a test on this bug. If it doesn't we might want to leave in the current test which uses the addon manager, so we can release rc5 soon. We could find a better test later.
Flags: needinfo?(hskupin)
(Assignee)

Updated

4 years ago
Assignee: cosmin.malutan → andreea.matei
(Assignee)

Comment 20

4 years ago
Created attachment 780399 [details] [diff] [review]
0001-Bug-871441-Changed-select-method-to-handle-xul-menul.patch

Updated the patch, tested with an updated testcase, also checked it against the patch from bug 865640 (which is currently blocked by this).

testChromeSelect doesn't satisfy the needs here due to the xul namespace for in content chrome pages.
I'll file a bug as follow up to update the mutt test later and the page needed.
Attachment #771570 - Attachment is obsolete: true
Attachment #780399 - Flags: review?(hskupin)
Attachment #780399 - Flags: review?(dave.hunt)
Comment on attachment 780399 [details] [diff] [review]
0001-Bug-871441-Changed-select-method-to-handle-xul-menul.patch

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

Some minor things to fix. Otherwise it looks good.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +738,5 @@
>          // Unwrap the XUL element's XPCNativeWrapper
>          this.element = utils.unwrapNode(this.element);
>          // Get the list of menuitems
> +        menuitems = this.element.getElementsByTagNameNS(NAMESPACE_XUL, "menupopup")[0].
> +                    getElementsByTagNameNS(NAMESPACE_XUL, "menuitem");

Please split this up to multiple lines. As of now this line is way too long.

::: mutt/mutt/tests/js/elementslib/menulist.js
@@ +57,5 @@
> +  controller.waitForPageLoad();
> +
> +  // Open Plugins section
> +
> +  // Add event listener to wait until the view has been changed

nit: Can you make those 3 lines a single comment?

@@ +66,5 @@
> +
> +  var plugin = new elementslib.ID(controller.window.document, "category-plugin");
> +  controller.click(plugin);
> +
> +  mozmill.utils.waitFor(function () {

This is now assert.waitFor()
Attachment #780399 - Flags: review?(hskupin)
Attachment #780399 - Flags: review?(dave.hunt)
Attachment #780399 - Flags: review-
(Assignee)

Comment 22

4 years ago
Created attachment 780925 [details] [diff] [review]
0001-Bug-871441-Changed-select-method-to-handle-xul-menul.patch

Updated as requested.
Attachment #780399 - Attachment is obsolete: true
Attachment #780925 - Flags: review?(hskupin)
Attachment #780925 - Flags: review?(dave.hunt)
Attachment #780925 - Flags: review?(hskupin)
Attachment #780925 - Flags: review?(dave.hunt)
Attachment #780925 - Flags: review+
Landed on master:
https://github.com/mozilla/mozmill/commit/095c73ae109892a3ed2b115fe03dd5e7c7366fc2

I don't think we should spend our time on getting this out for Mozmill 1.5. Lets wait with our tests for 2.0 to get re-enabled.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-2.0?][mozmill-1.5?][sprint2013-32] → [mozmill-2.0][ateamtrack: p=mozmill q=2013q3 m=1]
I did a test with csb and have seen that the test added here is failing:

    "message": "menulist.select is not a function", 
    "fileName": "resource://mozmill/modules/frame.js -> file:///mozilla/code/mozmill/mutt/mutt/tests/js/elementslib/menulist.js", 
    "name": "TypeError", 
    "lineNumber": 78

Andreea, can you please check that?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

4 years ago
It is not failing for me, I checked the ID's and anonid's, are the same no matter the locale. The only thing that may differ is the label name, for some locales it's still in English, but for others is translated. So those might fail there, but since we have to select through the label, I'm not sure if we can make this mutt test work no matter the locale.
This test is failing for me on Linux64 with the following build:
 https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/07/2013-07-25-00-40-04-mozilla-aurora-l10n/firefox-24.0a2.csb.linux-x86_64.tar.bz2
In rebasing for a large patch for landing in unrelated bug 767286, I noticed that this commit 095c73a appears to have introduced a regression, at least on my Windows XP32 client.

js\elementslib\mozelement.js | test

[1;31mERROR[0m | Test Failure | {
  "exception": {
    "stack": [
      "BaseError@resource://mozmill/modules/errors.js:27", 
      "AssertionError@resource://mozmill/modules/errors.js:76", 
      "Assert_waitFor@resource://mozmill/modules/assertions.js:607", 
      "browserAdditions/controller.waitForPageLoad@resource://mozmill/driver/controller.js:991", 
      "test@resource://mozmill/modules/frame.js -> file:///c:/mozmill/master-bug-767286-rev1/mutt/mutt/tests/js/elementslib/mozelement.js:14", 
      "Runner.prototype.execFunction@resource://mozmill/modules/frame.js:756", 
      "Runner.prototype.runTestModule@resource://mozmill/modules/frame.js:710", 
      "Runner.prototype.runTestFile@resource://mozmill/modules/frame.js:694", 
      "runTestFile@resource://mozmill/modules/frame.js:779", 
      "Bridge.prototype._execFunction@resource://jsbridge/modules/Bridge.jsm:140", 
      "Bridge.prototype.execFunction@resource://jsbridge/modules/Bridge.jsm:147", 
      "@resource://jsbridge/modules/Server.jsm:32", 
      ""
    ], 
    "message": "controller.waitForPageLoad(): Timeout waiting for page loaded.", 
    "fileName": "resource://mozmill/modules/errors.js", 
    "name": "AssertionError", 
    "lineNumber": 27
  }
}

If I roll back to one commit prior to 09573a, (50a7159) the test passes.

Tested in:
Windows XP32 SP3
mozmill-env w/mozmill 2.0rc4 latest
latest Nightly 25.0a1 20130724222711 139885
Thanks Jonathan! Can you please file a new bug for it? Also set [mozmill-2.0?] for it.
Or wait, I backed out the patch given that we have still tests not passing.

https://github.com/mozilla/mozmill/commit/48782575c0fd105fd4227fa8667179023ef910dc
Status: REOPENED → ASSIGNED
(Assignee)

Comment 30

4 years ago
Created attachment 788037 [details] [diff] [review]
0001-Bug-871441-Changed-select-method-to-handle-xul-menul.patch

Updated the test to wait for the Discovery pane to load when opening addons manager, as we do in our addons shared module. Tested intensively on windows as well with Nightly and Aurora, no more failures. 
What Jonathan caught was failing intermittently, that's why I missed it. Now I ran all mutt tests for hundreds of times.

As mentioned before, if we're using a locale that doesn't have the label buttons in English, the test will fail at changing the menulist, since we use plain text as dtds are not available at the moment.
Attachment #780925 - Attachment is obsolete: true
Attachment #788037 - Flags: review?(hskupin)
Attachment #788037 - Flags: review?(dave.hunt)
With the new patch provided the failing test in comment 27 now passes and I don't observe any other discrepancies between the patch vs. current master. I did about 25 `mutt testall` patch runs.

Total pass: 86
Total fail: 1 (known unrelated bug 898378)
Total skip: 2

Tested in:
Windows XP32 SP3
mozmill-env w/mozmill 2.0rc4 latest
latest Nightly 26.0a1 20130809030203 141852
(Assignee)

Comment 32

4 years ago
Thanks Jonathan, great to hear! I've also tested this version a lot on all platforms to be sure it's not failing intermittently.
(In reply to Andreea Matei [:AndreeaMatei] from comment #30)
> As mentioned before, if we're using a locale that doesn't have the label
> buttons in English, the test will fail at changing the menulist, since we
> use plain text as dtds are not available at the moment.

Have we ever filed a bug against the add-on manager to get this fixed? It's kinda bad that a localized string has to be used here.
Comment on attachment 788037 [details] [diff] [review]
0001-Bug-871441-Changed-select-method-to-handle-xul-menul.patch

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

Looks good now. Thanks for the update.

Landed on master:
https://github.com/mozilla/mozmill/commit/367474ec3dd549ef1f5d86eed0df4ebd277c1a8a

::: mutt/mutt/tests/js/testElementsLib/testMenuList.js
@@ +85,5 @@
> +             getAnonymousElementByAttribute(parent, "anonid", "state-menulist");
> +  var menulist =  new elementslib.Elem(node);
> +
> +  menulist.select(null, "Never Activate");
> +  expect.equal(menulist.getNode().label, "Never Activate",

As mentioned before please check that we can get this fixed in the add-on manager by adding an anonymous id, class, or whatever necessary.
Attachment #788037 - Flags: review?(hskupin)
Attachment #788037 - Flags: review?(dave.hunt)
Attachment #788037 - Flags: review+
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-2.0][ateamtrack: p=mozmill q=2013q3 m=1] → [mozmill-2.0][ateamtrack: p=mozmill q=2013q3 m=2]
Andreea, what about Mozmill 1.5? Would a backport help us here too?
Flags: needinfo?(andreea.matei)
(Assignee)

Comment 36

4 years ago
Created attachment 792806 [details] [diff] [review]
0001-Bug-871441-Followup-to-use-dtds-for-selecting-with-l.patch

Added a follow up to use dtds. Tested with several locales, it works fine.

Regarding the 1.5, I'll have to check more if we're affected there as well when we select with the label, create a short testcase. Will come back with more info asap.
Attachment #792806 - Flags: review?(hskupin)
Attachment #792806 - Flags: review?(dave.hunt)
Flags: needinfo?(andreea.matei)
Comment on attachment 792806 [details] [diff] [review]
0001-Bug-871441-Followup-to-use-dtds-for-selecting-with-l.patch

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

r=me with the tiny nit fixed.

::: mutt/mutt/tests/js/testElementsLib/testMenuList.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/. */
> +const { getEntity } = require("l10n");

nit: missing blank line above.
Attachment #792806 - Flags: review?(hskupin)
Attachment #792806 - Flags: review?(dave.hunt)
Attachment #792806 - Flags: review+
Restoring needinfo flag for Andreea, which seems to got lost.
Flags: needinfo?(andreea.matei)
(Assignee)

Comment 39

4 years ago
Created attachment 793902 [details] [diff] [review]
0001-Bug-871441-Followup-to-use-dtds-for-selecting-with-l.patch

Updated follow up patch. Next I'll check the select for Mozmill 1.5.
Attachment #792806 - Attachment is obsolete: true
Attachment #793902 - Flags: review?(hskupin)
(Assignee)

Updated

4 years ago
Blocks: 907983
(Assignee)

Comment 40

4 years ago
We fail as well in 1.5 to select the element in a xul menulist through label option, the fix landed in 2.0 works for 1.5 too. Uploading the patch asap after running reports.
Status: RESOLVED → REOPENED
Flags: needinfo?(andreea.matei)
Resolution: FIXED → ---
Whiteboard: [mozmill-2.0][ateamtrack: p=mozmill q=2013q3 m=2] → [mozmill-2.0][mozmill-1.5.22+][ateamtrack: p=mozmill q=2013q3 m=2]
Whiteboard: [mozmill-2.0][mozmill-1.5.22+][ateamtrack: p=mozmill q=2013q3 m=2] → [mozmill-2.0?][mozmill-1.5.22+][ateamtrack: p=mozmill q=2013q3 m=2]
Comment on attachment 793902 [details] [diff] [review]
0001-Bug-871441-Followup-to-use-dtds-for-selecting-with-l.patch

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

Landed on master as:
https://github.com/mozilla/mozmill/commit/4f3bfb647538360775f28346c17321e7f1a91efb
Attachment #793902 - Flags: review?(hskupin) → review+
Leaving open until we got the Mozmill 1.5 fix landed. Andreea, whenever you have tested that all is fine please upload the patch. Please check your fix with bug 865640.
Whiteboard: [mozmill-2.0?][mozmill-1.5.22+][ateamtrack: p=mozmill q=2013q3 m=2] → [mozmill-2.0][mozmill-1.5.22+][ateamtrack: p=mozmill q=2013q3 m=2]
(Assignee)

Comment 43

4 years ago
Created attachment 794560 [details] [diff] [review]
[1.5] 0001-Bug-871441-Focus-inside-the-select-method-is-not-set.patch

It is working with the patch fix from bug 865640, reports have included tests:
- testAddons/testPluginDisableAffectsAboutPlugins.js
- restartTests/testAddons_pluginDisabledAfterRestart

It also has some whitespaces removed across the file and "indx" renamed to "index".
Reports:
OS X: http://mozmill-crowd.blargon7.com/#/functional/report/6d53b7d749a60e65551b4b1d1908f2e2
http://mozmill-crowd.blargon7.com/#/functional/report/6d53b7d749a60e65551b4b1d1908e0b4

Linux: http://mozmill-crowd.blargon7.com/#/functional/report/6d53b7d749a60e65551b4b1d19090064
http://mozmill-crowd.blargon7.com/#/functional/report/6d53b7d749a60e65551b4b1d190922d9

Windows: http://mozmill-crowd.blargon7.com/#/functional/report/6d53b7d749a60e65551b4b1d1908f887
http://mozmill-crowd.blargon7.com/#/functional/report/6d53b7d749a60e65551b4b1d19090844
Attachment #794560 - Flags: review?(hskupin)
Attachment #794560 - Flags: review?(dave.hunt)
Comment on attachment 794560 [details] [diff] [review]
[1.5] 0001-Bug-871441-Focus-inside-the-select-method-is-not-set.patch

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

Looks good. Landed on hotfix-1.5:
https://github.com/mozilla/mozmill/commit/8ccd420fea3b65734c566c9145431430aeae0f48
Attachment #794560 - Flags: review?(hskupin)
Attachment #794560 - Flags: review?(dave.hunt)
Attachment #794560 - Flags: review+
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Depends on: 911101

Updated

4 years ago
Depends on: 912941

Updated

4 years ago
Depends on: 914528
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.