Closed Bug 1097705 Opened 10 years ago Closed 9 years ago

Add ability to right or middle click an element

Categories

(Testing :: Marionette Client and Harness, defect, P1)

defect

Tracking

(firefox38 fixed, firefox39 fixed)

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: ahal, Assigned: chmanchester)

References

(Blocks 1 open bug)

Details

(Keywords: pi-marionette-client, pi-marionette-server, pi-marionette-spec, Whiteboard: [marionette=1.0])

Attachments

(1 file)

At the moment it's only possible to left click an element. Luckily the underlying library marionette uses (EventUtils) supports this already, so this bug should just be a matter of propagating the proper parameters.
Hi,

I'm not an expert in marionette so I'm not sure the patch is complete (mainly I do not know what to implement in testing/marionette/marionette-server.js when the context is "chrome") but it works well locally and I think it could be a good start. If you have time and can provide me some information about possible missing/wrong things I will be happy to finish this patch.
Attachment #8524759 - Flags: feedback?(ahalberstadt)
In order to not make the Marionette client a special snowflake in the world, modelling this after the common WebDriver client APIs is an option.  They typically have Action APIs that allow you to compose different actions into a chain using the builder pattern:

    Actions(driver).send_keys("foo").click(el).perform()

More to the point, they don't expose all of the interfaces of Keyboard and Mouse (concepts which marionette-client doesn't have AFAICT) on the HTMLElement directly.

The HTMLElement.click method also behaves differently than a context or middle click in that it adds some precondition checks, so conflating the two behaviours of “do what I want” where the element is checked for visibility, enabledness, &c. with the “do what I mean” approach of calling Mouse.click(HTMLElement) is also confusing.
Comment on attachment 8524759 [details] [diff] [review]
Add ability to right or middle click an element

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

This looks great thanks! My one suggestion would be to add a mapping to the click call so consumers could use "left", "middle", "right" as values for the button parameter as well. I think doing this is fine, but you should get review from either :ato or :AutomatedTester, seems like there might be some webdriver compatibility concerns with this.
Attachment #8524759 - Flags: feedback?(ahalberstadt) → feedback+
(In reply to Andreas Tolfsen (:ato) from comment #2)
> In order to not make the Marionette client a special snowflake in the world,
> modelling this after the common WebDriver client APIs is an option.  They
> typically have Action APIs that allow you to compose different actions into
> a chain using the builder pattern:
> 
>     Actions(driver).send_keys("foo").click(el).perform()
> 
> More to the point, they don't expose all of the interfaces of Keyboard and
> Mouse (concepts which marionette-client doesn't have AFAICT) on the
> HTMLElement directly.
> 
> The HTMLElement.click method also behaves differently than a context or
> middle click in that it adds some precondition checks, so conflating the two
> behaviours of “do what I want” where the element is checked for visibility,
> enabledness, &c. with the “do what I mean” approach of calling
> Mouse.click(HTMLElement) is also confusing.

I don't really care too much which way we do this, but it sounds like this is a bigger issue than just this bug. Right now marionette's implementation of Actions doesn't even support click (much less right or middle click). Julien's patch is just adding an optional argument to an existing API so seems like there wouldn't be much downside in taking it for the time being.

Aside: personally creating an Actions object simply to click an element seems like overkill to me, but I'm sure webdriver had good reasons for doing it that way.
Andreas, if you agree with Andrew in comment 4, could you have a look at attachment 1 [details] [diff] [review] and tell me if there is something to add or change (other than the button parameter passed as a string as mentionned in comment 3) ?

Once done, I'll rework on this patch and ask you to review it if that's OK.
Flags: needinfo?(ato)
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
(In reply to Julien Pagès from comment #5)
> Andreas, if you agree with Andrew in comment 4, could you have a look at
> attachment 1 [details] [diff] [review] and tell me if there is something to
> add or change (other than the button parameter passed as a string as
> mentionned in comment 3) ?

I still think it's a bad idea to conflate click to mean “any type of click event” because there are different connotations tied to the shorthand click on a WebElement vs. an advanced user interaction click, which the right- and middle clicks would be part of.

But I'm not the reviewer.
Flags: needinfo?(ato)
Andreas, are you saying you'd prefer elem.right_click() to elem.click('right')? Or are you saying it shouldn't even be possible to do a right click without an Action chain?
(In reply to Andrew Halberstadt [:ahal] from comment #7)
> Andreas, are you saying you'd prefer elem.right_click() to
> elem.click('right')? Or are you saying it shouldn't even be possible to do a
> right click without an Action chain?

Let me try to clarify what I mean.  WebElement.click usually has special semantics because it's a “do what I mean” type of command that checks for staleness and visibility before clicking the element.  It's a convenience function because it's also meant to work on devices with no mouse pointer devices.

Because clicking a different button than the primary is more uncommon and lacks these precondition checks, they're usually exposed through the advanced user interaction interface, Actions.  In marionette-client you'll find things like tap, double_tap, press, flick, &c. on that: http://marionette-client.readthedocs.org/en/latest/#action-objects

But I note that HTMLElement also has tap exposed with different arguments ((x,y) instead of (el,x,y)).

Whether we want HTMLElement.click to have these special semantics for alternate clicks is the question.  If we do this patch looks fine, but it will differ from the behaviour of WebDriver.

Another point I'd like to mention is that the WebDriver specification is changing towards using numbers for pointing out which buttons to press on the wire protocol level (not yet landed in the spec) because mouse input devices can have more than three buttons and most window managers number them starting from 0.
This bug is a blocker for us to get some of the tabbed browsing Mozmill tests converted to Marionette. Julien, do you have an update for us? Or won't you have time to continue on that one?
Flags: needinfo?(j.parkouss)
This is not a problem of time for me, but I did not know where to go from comment 8.

Particularly:

(Andreas Tolfsen (:ato) from comment #8)

> Whether we want HTMLElement.click to have these special semantics for
> alternate clicks is the question.  If we do this patch looks fine, but it
> will differ from the behaviour of WebDriver.

So maybe we want to implement this only in a the advanced Actions object (that's where tap, press, flick, etc methods are living for now) or maybe we want that only directly in Marionette (that's what I did) or maybe both. Or maybe I misunderstood!

I don't know much about webDriver spec. At first, I would have implemented this both in Marionette and in advanced actions, as it is only a matter of adding an argument. But as ato said, clicks that are not normal (ie right, middle, or anything else) lacks some preconditions so it may be unsafe to put this in a simple and general "marionette.click" - see again comment 8.

Well, if there is more info about what we want, I'm still interested to work on this. :)
Flags: needinfo?(j.parkouss)
David and Andreas, we need further input from you both to be able to continue on this blocker. Can you please check and tell us your thoughts? Thanks.
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
This should only be on the _actions object_ and not on the marionette object. If we leave it on HTMLElement we are potentially leaving that Class open for becoming a God object[1], and we don't really want that.

Since right click and middle click rarely need to be able to check the state of the element implicitly before interacting with it. If we need to check it's state then we can use other method calls.


[1] http://en.wikipedia.org/wiki/God_object
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
Julien, is comment 12 enough to go on? If you're going to pick this up soon please let me know, otherwise I'll take a stab at it this week. Much appreciated!
Flags: needinfo?(j.parkouss)
Hey Chris,

Sure, go ahead. :)

I had some some questions I wanted to ask to David, but I did not find him on irc yesterday. That may take time and the bug is blocking, so, take it. :)
Assignee: j.parkouss → cmanchester
Flags: needinfo?(j.parkouss)
Whiteboard: [marionette=1.0]
Priority: -- → P1
(In reply to Chris Manchester [:chmanchester] from comment #15)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4315056573ed

Whoops, need to send "ESCAPE" in chrome to close the context menu when e10s is enabled. I'll run the fix through try when it opens.
(In reply to Chris Manchester [:chmanchester] from comment #16)
> Whoops, need to send "ESCAPE" in chrome to close the context menu when e10s
> is enabled. I'll run the fix through try when it opens.

Is that only necessary with Marionette when e10s is enabled? Our experience with Mozmill has been shown that this is necessary all the time.
(In reply to Henrik Skupin (:whimboo) from comment #17)
> (In reply to Chris Manchester [:chmanchester] from comment #16)
> > Whoops, need to send "ESCAPE" in chrome to close the context menu when e10s
> > is enabled. I'll run the fix through try when it opens.
> 
> Is that only necessary with Marionette when e10s is enabled? Our experience
> with Mozmill has been shown that this is necessary all the time.

Sending the esc key to a content element seems to work ok with e10s off.
Ok, so it's not only necessary when e10s is enabled as you stated in comment 16.
(In reply to Henrik Skupin (:whimboo) from comment #19)
> Ok, so it's not only necessary when e10s is enabled as you stated in comment
> 16.

Comment 16 is consistent with this, the relevant distinction is chrome/content.

New try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6285243be4b
https://hg.mozilla.org/mozilla-central/rev/f37231bfe5f6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
We kinda would like to have this support for Firefox 38 to be able to run our Firefox UI tests. Would it be possible to backport this patch?
Flags: needinfo?(dburns)
Flags: needinfo?(cmanchester)
Flags: needinfo?(cmanchester)
Product: Testing → Remote Protocol

Moving bugs for Marionette client due to component changes.

Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: