677364 - Fix controller keyboard methods to allow a null parameter for the target element

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

As in the old API all the keyboard and mouse methods allowed 'null' for the target parameter. In such a case the event simply would have been sent to the window instead of the element.

https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/controller.js#L975

We should re-add this to keep the backward compatibility. Those affected functions could simply check for null and use this.window as target.

Comment 1

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

Clint, this bug has to be fixed before we release beta 2 of Mozmill 2.0.

Comment 2

[Jeff Hammel]

Note: the link in the description is now bitrotted

Comment 3

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #2)
> Note: the link in the description is now bitrotted

see https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L349

Comment 4

[Jeff Hammel]

So I just want to make sure we're all on the same page as to what needs to be done.  Looking through controller.js, it looks like 3 methods need to change to support this:

- keypress:
  https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/exte\
nsion/resource/modules/controller.js#L349

- type:
  https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/exte\
nsion/resource/modules/controller.js#L375

- mouseEvent:
  https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/exte\
nsion/resource/modules/controller.js#L441

Am I missing any?

keypress and mouseEvent are really straight-forward.  type, OTOH, will require a more extensive refactor to get it to work like 1.5.

Comment 5

[Jeff Hammel]

Hmmm, its actually worse than that. While in 1.5, all of the mouse functions call mouseEvent, in 2.0 they call the underlying mozelement API :( Trying to figure out what to do about this...

Comment 6

[Jeff Hammel]

Attachment: WIP [checked-in]

This takes care of the basic cases.  There are a few others to be addressed:

- right now, type just calls keypress for each letter, which is how it was in 1.5, though hitherto in 2. it worked otherwise (and only with a TextBox).  The question is what (and when) to return.  Currently it will keypress everything but return false on any errors

- mouseEvent is fixed to accept a null parameter, but the other mouse methods in controller (click, double-click, etc) do not. In the 1.5 architecture, everything went through controller.mouseEvent so this would have sufficed.  However, in 2. we simply call the mozelement methods.  So the first question is....do we care? (How often do we click just on the window? I'm kinda guessing this is a bad idea anyway) And is it worth supporting this? If we decide it is, a few options:

1. Do it the way 1.5. does it.  This *sounds* good, but is actually fairly weird to do with the current state of code

2. copy+paste something for each method we care about.  I would need advice if this is actually a good idea.  Expedient, but stupid (assuming there aren't any dragons around).

3. use a decorator pattern for all of these functions.  Complicated, but hey, its backwards compatability, its going to be awful, it it would be fairly expedient.

Any opinions?  Do we care about the mouse events at all?

Comment 7

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

I don't think that we will ever have to use a click operation on a window itself. The most important case here are really keyboard shortcuts like Cmd+T to open a new tab. Just m2c.

Comment 8

[Jeff Hammel]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> I don't think that we will ever have to use a click operation on a window
> itself. The most important case here are really keyboard shortcuts like
> Cmd+T to open a new tab. Just m2c.

Cool. The existing patch should address both keypress and type.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 561621 [details] [diff] [review]
WIP [checked-in]

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

Looks good. Conditional r+ if you address the two comments

::: mozmill/mozmill/extension/resource/modules/controller.js
@@ +282,5 @@
>  
> +// constructs a MozMillElement from the controller's window
> +MozMillController.prototype.windowElement = function() {
> +  return new mozelement.MozMillElement(undefined, undefined, {'element': this.window});
> +}

This should probably be a getter (see https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/controller.js#L384 for an example)

@@ +997,4 @@
>  }
>  
>  MozMillController.prototype.mouseEvent = function(aTarget, aOffsetX, aOffsetY, aEvent, aExpectedEvent) {
> +  if (aTarget == null) { aTarget = this.windowElement(); }

Is this necessary?  Looking at 1.5 it looks like you aren't allowed to pass null into mouseEvent (https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L444)

Comment 10

[Jeff Hammel]

(In reply to Andrew Halberstadt [:ahal] from comment #9)
<snip/>
> >  
> >  MozMillController.prototype.mouseEvent = function(aTarget, aOffsetX, aOffsetY, aEvent, aExpectedEvent) {
> > +  if (aTarget == null) { aTarget = this.windowElement(); }
> 
> Is this necessary?  Looking at 1.5 it looks like you aren't allowed to pass
> null into mouseEvent
> (https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/
> extension/resource/modules/controller.js#L444)

Nope, I guess not. Changing bug title as such

Comment 11

[Jeff Hammel]

pushed to master: https://github.com/mozautomation/mozmill/commit/0c8a6d198f9cd0e529009a154d7cb8410b67f71d

Comment 12

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

This is not fixed. My updated test on bug 760418 which tests this for real, will show that this is still broken.

Comment 13

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

Attachment: Patch

Pointer to Github pull-request

Comment 14

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

Comment on attachment 630605 [details]
Patch

Not sure how this ever has worked. With the updated tests it's clear that the root element really has to be the window itself and not the documentElement. A key press against documentElement is lost in the nirvana.

This also adds a chrome test. At some point we probably have to move out the chrome tests to a separate firefox folder.

Comment 15

[Andrew Halberstadt [:ahal]]

(In reply to Henrik Skupin (:whimboo) from comment #14)
> Comment on attachment 630605 [details]
> Patch
> 
> Not sure how this ever has worked. With the updated tests it's clear that
> the root element really has to be the window itself and not the
> documentElement. A key press against documentElement is lost in the nirvana.
> 
> This also adds a chrome test. At some point we probably have to move out the
> chrome tests to a separate firefox folder.

Did you test this patch against the mutt tests? I remember that there was a reason I made the rootElement be the documentElement. It had something to do with the window object not being an actual DOM element which caused errors when wrapping it inside a MozMillElement. I need to do a bit of investigating.

Comment 16

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

All Mutt tests are passing. But that doesn't mean anything because we have way to less tests. The same also applies to Mozmill 1.5.x which was using this method and we never had failures. If you are using the rootElement somewhere else we should keep it as is and don't make use of it in keypress/type.

Comment 17

[Andrew Halberstadt [:ahal]]

I think we should file a follow-on bug for the controller.rootElement problem. We should be able to send click's and keypresses the rootElement. Is there a specific test where these aren't working?

I'm a bit confused though because many of the peptests (e.g https://github.com/mozilla/peptest/blob/master/tests/firefox/test_contextMenu.js) are using this feature and don't seem to have any problems.

Comment 18

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

(In reply to Andrew Halberstadt [:ahal] from comment #17)
> I think we should file a follow-on bug for the controller.rootElement
> problem. We should be able to send click's and keypresses the rootElement.
> Is there a specific test where these aren't working?

See my patch or directly run:
mozmill -b %app% -m mutt/tests/js/elementslib/no_key_event_target_content.js

> I'm a bit confused though because many of the peptests (e.g
> https://github.com/mozilla/peptest/blob/master/tests/firefox/
> test_contextMenu.js) are using this feature and don't seem to have any
> problems.

None of those tests really test if the action was performed. The same was the case for the test in Mozmill. It could be that it worked for contextmenus due to access keys but it clearly fails for textboxes.

Also this one equals to false:
alert(controller.window === controller.window.document.documentElement)

The reason is that documentElement is not the window but the root element as the name states. See:

https://developer.mozilla.org/en/DOM/document.documentElement

Returns the Element that is the root element of the document (for example, the <html> element for HTML documents).

Comment 19

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

So I would propose we keep rootElement as it is but change keypress/type and do:

- aTarget = this.rootElement;
+ aTarget = this.window;

Comment 20

[Andrew Halberstadt [:ahal]]

Comment on attachment 630605 [details]
Patch

Ah, ok looking at https://github.com/whimboo/mozmill/blob/778c2f956c8d6391838e26c785875db56d1c62dc/mutt/mutt/tests/js/elementslib/no_key_event_target_content.js I see why it wouldn't work. The keypress event is getting sent to the root document element and not to the textbox element. Whereas if the keypress was going to the window it would have been forwarded to whichever element has focus.

Having controller.documentRoot is still useful though because it can be used to do things like send right click to open context menus, or sending accelKey + r to refresh the page.

The question now becomes what is the expected behaviour? Is it important to be able to send key events to the window with the expectation that it'll propagate to the element with focus? Or is it acceptable to just send these events directly to the elements themselves?

In terms of this test case we could just send some global firefox hotkeys instead. e.g 

> controller.keypress(null, 't', {'accelKey': true});

should open a new tab.


At any rate this patch will cause things to break so I'm r-ing it. The window object isn't a DOM element and so doesn't have the properties/methods that MozMillElement expects.

Comment 21

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

(In reply to Andrew Halberstadt [:ahal] from comment #20)
> The question now becomes what is the expected behaviour? Is it important to
> be able to send key events to the window with the expectation that it'll
> propagate to the element with focus?

Exactly this is how it should behave. If nothing is specified the event gets send to the window itself, which then will forward the event to the window.document.activeElement. That's how it has to work in Gecko and we have to make use of.

> In terms of this test case we could just send some global firefox hotkeys
> instead. e.g 
> 
> > controller.keypress(null, 't', {'accelKey': true});
> 
> should open a new tab.

No. This will not fix the problem but makes the test invalid.

> At any rate this patch will cause things to break so I'm r-ing it. The
> window object isn't a DOM element and so doesn't have the properties/methods
> that MozMillElement expects.

So what do you propose? Are you ok with my proposal from comment 19?

Comment 22

[Andrew Halberstadt [:ahal]]

(In reply to Henrik Skupin (:whimboo) from comment #21)
> So what do you propose? Are you ok with my proposal from comment 19?

Do you mean this line? https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/controller.js#L1087

I don't think that will work since this.window.keypress isn't an existing method.

I think we either have to:
1) refactor MozMillElement to detect whether it is working with a window or an element
or
2) Make the null even more of a special case and send the click event without going through MozMillElement at all.

Comment 23

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

Andrew, I have updated the pull request. Please re-check. Here some more details:

(In reply to Andrew Halberstadt [:ahal] from comment #22)
> I don't think that will work since this.window.keypress isn't an existing
> method.

.keypress() is a member of MozElement, so you can't do the above. MozElement wraps that nicely by automatically detecting the defaultView.

> I think we either have to:
> 1) refactor MozMillElement to detect whether it is working with a window or
> an element

Well, here is the point. It already works perfectly. The problem why it failed were bugs in the MozElement instantiation itself. Even the current code for document.documenentElement returned an invalid element.

Using the 'Elem' locator and creating a window element from it works and the updated pull implements that for sending keypress() and type() events to the window itself. Therefore I do not use the rootElement as before because that will probably break other tests which rely on it, but handle it separately in both of the above methods. It really doesn't have to be used somewhere else and its unique to those methods.

Comment 24

[Andrew Halberstadt [:ahal]]

https://github.com/mozautomation/mozmill/blob/b4290aa9f174aeb7306f4b9bea8cf7b93dd150bd/mozmill/mozmill/extension/resource/driver/mozelement.js#L226

Since window objects don't have the above method (and a few others here and there) this patch will make it so that mouse events won't work on windows (as well as some of the other functions). Keypresses will work fine.

I think the thing to do here is to add 'if ("getBoundingClientRect" in elem)' statements around the methods that windows don't have and try to make the mouse event/other methods work a different way.

The offending methods/properties are:
- getBoundingClientRect
- scrollIntoView
- namespaceURI
- ownerDocument

Comment 25

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

So the current version of EventUtils we have in our repository doesn't support sending mouse events to a window. That means as long as we do not upgrade EventUtils (if it has such a support now) we can't support those mouse events.

Also I think that if a tests needs to send a key to a window it has to create an element from the window itself. That means we should not allow a null parameter for controller.keypress() and controller.type().

Andrew, the patch has been updated and a new commit is up. Please let me know what you think.

Comment 27

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

With my latest push I have added back the Mozmill 1.5.x compatibility in specifying a null object in controller.keypress() and controller.type(). We should only do that in those two methods and nowhere else. If we don't allow that a lot of our tests will fail because those are firing key events against the window.

Andrew, can you please have a look at the patch? I kinda would like to have it in a final state soon. Thanks!

Comment 28

[Andrew Halberstadt [:ahal]]

I commented on the pull request. One question though, how come we are switching to the github view? It is kind of hard to see what is going on there if there are a bunch of commits on the same pull request as things could be changed and then changed back again.

At least if you could set my review flag back to '?' when you do a new commit and I'll be able to notice it and get around to it quicker. Sorry about the delay.

Comment 29

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

(In reply to Andrew Halberstadt [:ahal] from comment #28)
> I commented on the pull request. One question though, how come we are
> switching to the github view? It is kind of hard to see what is going on
> there if there are a bunch of commits on the same pull request as things
> could be changed and then changed back again.

There are two ways you can check changes on github. One is the per commit changes which corresponds to the kinda broken interdiff implementation in Bugzilla. And the other is the full diff which only shows the diff between the latest revision and the target branch. This is so helpful for us that we wanted to give that a try. 

> At least if you could set my review flag back to '?' when you do a new
> commit and I'll be able to notice it and get around to it quicker. Sorry
> about the delay.

So you don't get emails from github when someone comments on a pull request you have already commented on? You should probably enable that. :)

Comment 30

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

Comment on attachment 630605 [details]
Patch

Andrew, it would be nice if you can have a quick glance over the latest commit.

Comment 31

[Andrew Halberstadt [:ahal]]

Comment on attachment 630605 [details]
Patch

Looks good, r+

Comment 32

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

Landed final patch:
https://github.com/mozautomation/mozmill/commit/10bfe55e88b93c19ff9564261c96c9ba261420df

We should be fine now in that area. Closing bug as fixed.

Comment 33

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

I had to push a follow-up because the latest change which removed the check for scrollIntoView() caused us a regression. My bad that I haven't noticed that.

https://github.com/mozautomation/mozmill/commit/4ebde3d6e04cd14dc24df6028f4049d432efe1c2