Support for right and double click #462

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@mikz
Contributor

mikz commented Feb 9, 2013

Hi!

My app heavily relies on right & double clicks so I added support for them to this awesome project.

Hope the tests are enough.

@mhoran

This comment has been minimized.

Show comment Hide comment
@mhoran

mhoran Feb 9, 2013

Collaborator

The click code is going through a significant refactor at the moment as required for Qt 5 support. I'd prefer to have you rework this after that code is merged.

Collaborator

mhoran commented Feb 9, 2013

The click code is going through a significant refactor at the moment as required for Qt 5 support. I'd prefer to have you rework this after that code is merged.

@mikz

This comment has been minimized.

Show comment Hide comment
@mikz

mikz Feb 9, 2013

Contributor

Sure. Could you ping to this thread after it gets merged?

Contributor

mikz commented Feb 9, 2013

Sure. Could you ping to this thread after it gets merged?

@mikz

This comment has been minimized.

Show comment Hide comment
@mikz

mikz Feb 9, 2013

Contributor

And I see travis failing even it is green locally. So no rush.

Contributor

mikz commented Feb 9, 2013

And I see travis failing even it is green locally. So no rush.

@mhoran

This comment has been minimized.

Show comment Hide comment
@mhoran

mhoran Feb 12, 2013

Collaborator

@mikz, the new click handling code has been merged.

Collaborator

mhoran commented Feb 12, 2013

@mikz, the new click handling code has been merged.

@mikz

This comment has been minimized.

Show comment Hide comment
@mikz

mikz Feb 12, 2013

Contributor

@mhoran It is done, can you check it?

Contributor

mikz commented Feb 12, 2013

@mhoran It is done, can you check it?

@mhoran

View changes

src/JavascriptInvocation.cpp
+
+ QPoint mousePos(x, y);
+
+ QMouseEvent event(QEvent::MouseMove, mousePos, Qt::NoButton, Qt::NoButton, Qt::NoModifier);

This comment has been minimized.

Show comment Hide comment
@mhoran

mhoran Feb 12, 2013

Collaborator

Is mouseMove required if we've already clicked?

@mhoran

mhoran Feb 12, 2013

Collaborator

Is mouseMove required if we've already clicked?

@mhoran

View changes

src/JavascriptInvocation.cpp
+}
+
+void JavascriptInvocation::doubleClick(int x, int y) {
+ JavascriptInvocation::click(x, y);

This comment has been minimized.

Show comment Hide comment
@mhoran

mhoran Feb 12, 2013

Collaborator

Let's move this out to doubleClick in capybara.js.

@mhoran

mhoran Feb 12, 2013

Collaborator

Let's move this out to doubleClick in capybara.js.

@mikz

This comment has been minimized.

Show comment Hide comment
@mikz

mikz Feb 12, 2013

Contributor

@mhoran
The mouseMove is not needed - removed it.
And moved the click in doubleClick to capybara.js.

Contributor

mikz commented Feb 12, 2013

@mhoran
The mouseMove is not needed - removed it.
And moved the click in doubleClick to capybara.js.

src/capybara.js
+
+ doubleClick: function(index) {
+ this.clickNodeAt(index, function(x, y) {
+ CapybaraInvocation.click(x, y);

This comment has been minimized.

Show comment Hide comment
@mhoran

mhoran Feb 19, 2013

Collaborator

Should we instead call clickNodeAt twice, once for each type of click? That way if the first click causes the element to become unclickable, a ClickFailed error will be raised.

@mhoran

mhoran Feb 19, 2013

Collaborator

Should we instead call clickNodeAt twice, once for each type of click? That way if the first click causes the element to become unclickable, a ClickFailed error will be raised.

src/JavascriptInvocation.cpp
@@ -47,6 +47,26 @@ void JavascriptInvocation::click(int x, int y) {
QApplication::sendEvent(m_page, &event);
}
+void JavascriptInvocation::rightClick(int x, int y) {

This comment has been minimized.

Show comment Hide comment
@mhoran

mhoran Feb 19, 2013

Collaborator

There's a lot of duplication here, with the exception of mouseMove. Should we instead abstract out a click method which takes a type -- left, right, double -- and call through to that from JavaScript? The only complication is mouseMove for the initial click -- but maybe we just ignore that if the type is doubleClick.

@mhoran

mhoran Feb 19, 2013

Collaborator

There's a lot of duplication here, with the exception of mouseMove. Should we instead abstract out a click method which takes a type -- left, right, double -- and call through to that from JavaScript? The only complication is mouseMove for the initial click -- but maybe we just ignore that if the type is doubleClick.

This comment has been minimized.

Show comment Hide comment
@mikz

mikz Feb 20, 2013

Contributor

I'm not sure because:

  • left click
    • position
    • move
    • press
    • release
  • right click
    • position
    • move
    • press
  • double click
    • position
    • dbl click
    • release

Only shared thing is position & move & press in left click and right click. Abstracting that does not make much sense for me.
Maybe combining creating event & sending it to one method call would be ok?

@mikz

mikz Feb 20, 2013

Contributor

I'm not sure because:

  • left click
    • position
    • move
    • press
    • release
  • right click
    • position
    • move
    • press
  • double click
    • position
    • dbl click
    • release

Only shared thing is position & move & press in left click and right click. Abstracting that does not make much sense for me.
Maybe combining creating event & sending it to one method call would be ok?

spec/driver_spec.rb
- element.innerHTML = event.type;
+ // https://developer.mozilla.org/en-US/docs/DOM/MouseEvent
+ // button == 2 means right mouse button
+ var type = event.button == 2 ? 'contextmenu' : event.type;

This comment has been minimized.

Show comment Hide comment
@jferris

jferris Feb 20, 2013

Member

Can we extract the 2 to a variable or something to give it meaning instead of adding a comment?

@jferris

jferris Feb 20, 2013

Member

Can we extract the 2 to a variable or something to give it meaning instead of adding a comment?

@mikz

This comment has been minimized.

Show comment Hide comment
@mikz

mikz Feb 20, 2013

Contributor

@jferris done, should I squash the commits?

Contributor

mikz commented Feb 20, 2013

@jferris done, should I squash the commits?

@jferris

This comment has been minimized.

Show comment Hide comment
@jferris

jferris Feb 20, 2013

Member

That would be great; thanks.

Member

jferris commented Feb 20, 2013

That would be great; thanks.

@mikz

This comment has been minimized.

Show comment Hide comment
@mikz

mikz Feb 20, 2013

Contributor

@jferris done

something else has to be done?

Contributor

mikz commented Feb 20, 2013

@jferris done

something else has to be done?

@jferris

This comment has been minimized.

Show comment Hide comment
@jferris

jferris Feb 20, 2013

Member

@mhoran any further ideas on the duplication there? I don't see an obvious way to consolidate it.

Member

jferris commented Feb 20, 2013

@mhoran any further ideas on the duplication there? I don't see an obvious way to consolidate it.

@mhoran

This comment has been minimized.

Show comment Hide comment
@mhoran

mhoran Feb 20, 2013

Collaborator

Looks good. I left a few final comments. I can't see an easy way to remove the duplication while preserving the click test for double click events, though the mouseEvent abstraction looks better.

Collaborator

mhoran commented Feb 20, 2013

Looks good. I left a few final comments. I can't see an easy way to remove the duplication while preserving the click test for double click events, though the mouseEvent abstraction looks better.

@mhoran

This comment has been minimized.

Show comment Hide comment
@mhoran

mhoran Feb 20, 2013

Collaborator

Also, could you fix the date on your commit? It's registering as having been authored last year.

Collaborator

mhoran commented Feb 20, 2013

Also, could you fix the date on your commit? It's registering as having been authored last year.

@mikz

This comment has been minimized.

Show comment Hide comment
@mikz

mikz Feb 25, 2013

Contributor

@mhoran I fixed the date, moved mouseEvents to private, but I did not came up with better name for clickNodeAt.
Do you have any suggestions? Only thing I could think of is performActionAt(index, ...) which is quite bad.

Contributor

mikz commented Feb 25, 2013

@mhoran I fixed the date, moved mouseEvents to private, but I did not came up with better name for clickNodeAt.
Do you have any suggestions? Only thing I could think of is performActionAt(index, ...) which is quite bad.

@mhoran

This comment has been minimized.

Show comment Hide comment
@mhoran

mhoran Feb 26, 2013

Collaborator

@mikz, let's rename clickNodeAt to click, click to leftClick and make the corresponding change to JavascriptInvocation::click. This will leave us with leftClick, rightClick, and doubleClick, with click encapsulating the implementation.

Also, I just merged some changes to master which conflict with this PR. We've also got #479 in the pipeline, which may get merged before you have the chance to take care of this.

Collaborator

mhoran commented Feb 26, 2013

@mikz, let's rename clickNodeAt to click, click to leftClick and make the corresponding change to JavascriptInvocation::click. This will leave us with leftClick, rightClick, and doubleClick, with click encapsulating the implementation.

Also, I just merged some changes to master which conflict with this PR. We've also got #479 in the pipeline, which may get merged before you have the chance to take care of this.

@mikz

This comment has been minimized.

Show comment Hide comment
@mikz

mikz Feb 27, 2013

Contributor

@mhoran done & rebased

Contributor

mikz commented Feb 27, 2013

@mhoran done & rebased

@mhoran

This comment has been minimized.

Show comment Hide comment
@mhoran

mhoran Feb 27, 2013

Collaborator

Merged as 89ad8b6 with a few changes.

Thanks!

Collaborator

mhoran commented Feb 27, 2013

Merged as 89ad8b6 with a few changes.

Thanks!

@mhoran mhoran closed this Feb 27, 2013

@mhoran

This comment has been minimized.

Show comment Hide comment
@mhoran

mhoran Feb 27, 2013

Collaborator

@mikz, I just noticed that you don't trigger mouse button release on right click. Is that intentional? I would think that we need to trigger that or else we'll wind up in some interesting state.

Collaborator

mhoran commented Feb 27, 2013

@mikz, I just noticed that you don't trigger mouse button release on right click. Is that intentional? I would think that we need to trigger that or else we'll wind up in some interesting state.

@mikz

This comment has been minimized.

Show comment Hide comment
@mikz

mikz Feb 27, 2013

Contributor

Yes it is. Triggering the release triggers another mouse event.

Contributor

mikz commented Feb 27, 2013

Yes it is. Triggering the release triggers another mouse event.

@mhoran

This comment has been minimized.

Show comment Hide comment
@mhoran

mhoran Feb 27, 2013

Collaborator

When I run the fixture you provided in Chrome, I do get two mouse events for right click. Isn't that the proper behavior?

Collaborator

mhoran commented Feb 27, 2013

When I run the fixture you provided in Chrome, I do get two mouse events for right click. Isn't that the proper behavior?

@mikz

This comment has been minimized.

Show comment Hide comment
@mikz

mikz Feb 27, 2013

Contributor

What is really important is the 'contextmenu' event. I double checked how chrome behaves and I found that actually the test I wrote for right click is wrong.
The event type should be 'contextmenu' no matter what. So it looks like the event is not fired at all.
I fixed the spec in b99f6ec but unfortunately it fails with my code :(.
I'm struggling to propagate contextmenu event to the page. That there is no window does not make it easier. Guess there is no way how to see what is happening, right?

edit: after long fight with qt I managed to fire contextmenu event - e63af09. I was trying to create event and fire it like others, but that did not work. Reading through webkit source code I found swallowContextMenuEvent which tries to push contextmenu events to page.

What to do now? Should I squash the commits?

Contributor

mikz commented Feb 27, 2013

What is really important is the 'contextmenu' event. I double checked how chrome behaves and I found that actually the test I wrote for right click is wrong.
The event type should be 'contextmenu' no matter what. So it looks like the event is not fired at all.
I fixed the spec in b99f6ec but unfortunately it fails with my code :(.
I'm struggling to propagate contextmenu event to the page. That there is no window does not make it easier. Guess there is no way how to see what is happening, right?

edit: after long fight with qt I managed to fire contextmenu event - e63af09. I was trying to create event and fire it like others, but that did not work. Reading through webkit source code I found swallowContextMenuEvent which tries to push contextmenu events to page.

What to do now? Should I squash the commits?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment