Closed Bug 1422583 Opened 7 years ago Closed 6 years ago

Double-click tracker is not reset in "Release Actions"

Categories

(Remote Protocol :: Marionette, defect, P2)

Product:
Remote Protocol
Component:
Marionette
Version:
59 Branch
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox-esr60 fixed, firefox58 unaffected, firefox59 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- fixed
firefox58 --- unaffected
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: barancev, Assigned: whimboo)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(5 files)

trace log
27.68 KB, text/plain
Details
wdspec trace log
29.90 KB, text/plain
Details
trace.log
27.15 KB, text/x-log
Details
[marionette] Reset double click tracker when releasing actions
866 bytes, patch
ato
: review+
Details | Diff | Splinter Review
[wdspec] Add action tests for double click state when releasing action chain
4.56 KB, patch
ato
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

Using Java Selenium 3.8.1 and Firefox Nightly (Built from https://hg.mozilla.org/mozilla-central/rev/ea747bb2ffb77d1fd62b5fa6217cbee15b73d31f) run this code:

driver.get("http://www.seleniumhq.org/");
WebElement input = driver.findElement(By.name("q"));
new Actions(driver).sendKeys(input, "testme").perform();
new Actions(driver).click(input)
        .keyDown(Keys.SHIFT)
        .sendKeys(Keys.LEFT)
        .sendKeys(Keys.LEFT)
        .keyUp(Keys.SHIFT)
        .sendKeys(Keys.DELETE)
        .perform();
assertEquals("test", input.getAttribute("value"));


Actual results:

org.junit.ComparisonFailure: 
Expected :test
Actual   :me


Expected results:

It is a regression issue, it is known to work in Nightly at 2017-11-29T08:33:33Z, but failed at 2017-11-30T04:14:49Z.
Maybe a regression from bug 1385476?
Depends on: 1385476
Flags: needinfo?(mjzffr)
More information: if I replace
new Actions(driver).sendKeys(input, "testme").perform();
with
input.sendKeys("testme");
in the provided scenario -- it works as expected.

So it's "actions interferention".
For future reference, it would be immensely useful if we had these
test cases that you run upstream in the WPT repository [1], if it
is the case that they test spec-conforming behaviour.

  [1] https://github.com/w3c/web-platform-tests
This scenario is a part of Selenium Java regression test suite. It's worth to rewrite it all to WPT, I agree ;)
Here the pushlog between the two Nightly builds:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3f6b9aaed8cd57954e0c960cde06d25228196456&tochange=84d925e10c0d939865fd9e92219bddfc9d4cd5c2

I run mozregression against this scenario and it is indeed a regression from bug 1385476.
Blocks: 1385476
status-firefox58: --- → unaffected
status-firefox59: --- → affected
No longer depends on: 1385476
Keywords: regression
Status: UNCONFIRMED → NEW
Ever confirmed: true
Here the Python test I used (and converted from Java)

    def test_sendkeys(self):
        self.driver.get("http://www.seleniumhq.org/")
        input = self.driver.find_element_by_id("q")
        ActionChains(self.driver).click(input).send_keys("testme").perform()
        (ActionChains(self.driver).click(input)
            .key_down(Keys.SHIFT)
            .send_keys(Keys.LEFT)
            .send_keys(Keys.LEFT)
            .key_up(Keys.SHIFT)
            .send_keys(Keys.DELETE)
            .perform())
        self.assertEquals("test", input.get_attribute("value"))
I will look at this tomorrow.
I wrote a wdspec test to match:

def test_shift_delete_erases_keys(session, key_reporter, key_chain):
    key_chain \
        .send_keys("efcd") \
        .key_down(Keys.SHIFT) \
        .send_keys([Keys.LEFT, Keys.LEFT]) \
        .key_up(Keys.SHIFT) \
        .send_keys([Keys.DELETE]) \
        .perform()
    assert get_keys(key_reporter) == "ef"


This test passes.

Alexei, could you attach a trace-level geckodriver log so I can see what the Marionette commands are doing differently from what I see locally?
Flags: needinfo?(mjzffr) → needinfo?(barancev)
I'll be able to attach trace log bit later. Now I just want to note that your scenario is not equivalent to the original one. You have to use two key chains, one to set initial input value, another one to modify it. With a single key chain everything works well, yes.
Flags: needinfo?(barancev)
Alexei, I don't think you have to test again. I was also able to reproduce, and as you said two ActionChains have to be used.
The W3C WebDriver actions model operates with multiple, distinct
virtual keyboard devices.  Every action chain needs to define the
ID of the device it wants to control, so that it is in theory
possible to combine parallel interaction sequences from an arbitrary
set of input sources.  This was meant to be for combining multiple
pointer inputs to simulate complex touch gestures, but could also
be what is happening with keyboards here.

The Selenium actions API smooths over this by not letting you define
the device ID, so I wonder if the two action chains defined using
the Selenium API maps down to two unique virtual keyboards.  If the
intention is for the API to be backwards compatible the action chain
shim needs to map them down to the same device ID.

I guess we can’t take any further action on this bug until we get
the trace-log.
Attached file trace log — 

    
    

    
  
Another test case with multiple chains, also passes. 

Note that the webdriver client used for wdspec tests forces use to construct actions chains at a lower level, which is why I build up separate mouse and key chains, then combine them. 

The test below constructs the chain as it should be (that is, the click happens before we enter any keys) and it passes:

def test_shift_delete_erases_keys(session, url):
    session.url = url("/webdriver/tests/actions/support/test_actions_wdspec.html")
    input_el = session.find.css("#keys", all=False)
    session.execute_script("resetEvents();")
    mouse_chain1 = session.actions.sequence("pointer", "pointer_id",
        {"pointerType": "mouse"})
    mouse_chain2 = session.actions.sequence("pointer", "pointer_id",
        {"pointerType": "mouse"})
    key_chain1 = session.actions.sequence("key", "keyboard_id")
    key_chain2 = session.actions.sequence("key", "keyboard_id")
    mouse_chain1 \
        .click(element=input_el)
    key_chain1 \
        .pause(0) \
        .pause(0) \
        .pause(0) \
        .send_keys("testme")
    session.actions.perform([mouse_chain1.dict, key_chain1.dict])
    mouse_chain2 \
        .click(element=input_el)
    key_chain2 \
        .pause(0) \
        .pause(0) \
        .pause(0) \
        .key_down(Keys.SHIFT) \
        .send_keys([Keys.LEFT, Keys.LEFT]) \
        .key_up(Keys.SHIFT) \
        .send_keys([Keys.DELETE])
    session.actions.perform([mouse_chain2.dict, key_chain2.dict])
    assert get_keys(input_el) == "test"


I suspect that the Selenium client, in allowing API calls that combine mouse and key actions in one sequence, does something silly when constructing the payload for the actions endpoint. I'll read through the trace log now to check whether this is true.

    
    

    
  
Hmm, Henrik's trace log looks like it should work fine. The payload looks like it's constructed correctly, so my guess that Selenium is doing something silly with the payload was wrong.  

However, the payload produced by my test is the same (as far as I can tell) and it passes. Maybe it's a platform-specific issue. (I'm running on mac). I will push to try and see what happens on linux. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=264ef5e2af1930e2a7e2e85e83a9b3df69416f3a

    
    

    
  
Oh, it was not my trace log but one from Alexei. I only moved it into an attachment.

Otherwise I'm also on MacOS and the Selenium test in comment 6 produces the failure for me starting with your change.

    
    

    
  

      
      
      
      

        Attached file
          
        wdspec trace log
        — 
      

    


  
Aha! The failure reproduces on try on linux. Log attached.

    
    

    
  
[Mass Change 2018-01-15] Moving bugs to backlog
Priority: -- → P3

    
    

    
  
Maja, how important should we treat this regression? Just want to remind that this will hit the beta population soon with the first Firefox 59.0 dev edition coming out.
Flags: needinfo?(mjzffr)

    
    

    
  
I was stumped when I first looked at it last month and I don't have much time to look at it in the next while. I'll try to squeeze it in, but as we discussed f2f, I don't think it's a high priority right now.
Flags: needinfo?(mjzffr)
Priority: P3 → P2

    
    

    
  
Seems likely this affects 60 and 61 as well. Too late to fix in 59 though.

          status-firefox59:
          affected → wontfix

          status-firefox60:
          --- → ?

          status-firefox61:
          --- → ?

    
    

    
  
I can still reproduce this failure with my Selenium Python test as referenced in comment 6. Interesting is that when I add a sleep between the first and second action sequence it works, once the delay is longer than 400ms.

The reason why it is not reproducible with the wdspec testcase is that Selenium also sends mouse actions:

First action:
> {"actions": [{"parameters": {"pointerType": "mouse"}, "type": "pointer", "id": "mouse", "actions": [{"duration": 250, "x": 0, "type": "pointerMove", "y": 0, "origin": {"element-6066-11e4-a52e-4f735466cecf": "db1f4361-a406-2849-bb35-8e679e812ee4"}}, {"duration": 0, "button": 0, "type": "pointerDown"}, {"duration": 0, "button": 0, "type": "pointerUp"}]}, {"type": "key", "id": "key", "actions": [{"duration": 0, "type": "pause"}, {"duration": 0, "type": "pause"}, {"type": "keyDown", "value": "t"}, {"type": "keyUp", "value": "t"}, {"type": "keyDown", "value": "e"}, {"type": "keyUp", "value": "e"}, {"type": "keyDown", "value": "s"}, {"type": "keyUp", "value": "s"}, {"type": "keyDown", "value": "t"}, {"type": "keyUp", "value": "t"}, {"type": "keyDown", "value": "m"}, {"type": "keyUp", "value": "m"}, {"type": "keyDown", "value": "e"}, {"type": "keyUp", "value": "e"}]}]}
 
Second action: 
> {"actions": [{"parameters": {"pointerType": "mouse"}, "type": "pointer", "id": "mouse", "actions": [{"duration": 250, "x": 0, "type": "pointerMove", "y": 0, "origin": {"element-6066-11e4-a52e-4f735466cecf": "e9d6c289-d3ab-6a46-aaa5-96be1083031c"}}, {"duration": 0, "button": 0, "type": "pointerDown"}, {"duration": 0, "button": 0, "type": "pointerUp"}, {"duration": 0, "type": "pause"}, {"duration": 0, "type": "pause"}]}, {"type": "key", "id": "key", "actions": [{"duration": 0, "type": "pause"}, {"duration": 0, "type": "pause"}, {"type": "keyDown", "value": "\ue008"}, {"type": "keyDown", "value": "\ue012"}, {"type": "keyUp", "value": "\ue012"}, {"type": "keyDown", "value": "\ue012"}, {"type": "keyUp", "value": "\ue012"}, {"type": "keyUp", "value": "\ue008"}, {"type": "keyDown", "value": "\ue017"}, {"type": "keyUp", "value": "\ue017"}]}]}

With the extra delays of 250ms for the pointer move we get 650ms, which is what the following commit added as double click interval: https://hg.mozilla.org/mozilla-central/rev/4391f6b7a338#l2.15

Which means that the `click()` action as performed in the second action sequence causes a double click, as long as there is no delay >640ms in-between.

Given that there is no need to click the same element again after sending the keys to it, the extra `click(input)` can be removed.

This makes my testcase invalid, but checking the original testcase from Aleksei I can see that there is no extra `click` action for the first sequence, but simply a `send_keys_to_element()` call equivalent to Python.

Aleksei, I would be really interested in a trace log for this particular failure. Can you please attach one to this bug? Thanks.
Flags: needinfo?(barancev)

    
    

    
  

      
      
      
      

        Attached file
          
        trace.log
        — 
      

    


  
Trace log for the original test case
Flags: needinfo?(barancev)

    
    

    
  
Thanks Alexei. So indeed, both of your action sequences are using the same input source ("default mouse"), and are sending a `pointerDown` and `pointerUp` action. The `pointerMove` action has even a duration of 100ms only. That means it clearly falls into the double click duration check.

Andreas, I had a look at the Webdriver spec but actually I cannot find anything related to double clicks and how those should be handled. Is that a topic which hasn't been discussed yet?

Or should the double click state actually have been reset by using another action sequence?
Flags: needinfo?(ato)

    
    

    
  
So I really think that in your case you want to call `Release Actions` in between your two action sequences. I don't see that this is happening.

Further our implementation resets all states but misses to also reset the DoubleClickTracker:

https://dxr.mozilla.org/mozilla-central/rev/c2e3be6a1dd352b969a45f0b85e87674e24ad284/testing/marionette/listener.js#786-790

Adding the following line would fix it if `release` or `reset_actions` (like for Python bindings) is called.

> event.DoubleClickTracker.resetClick();

    
    

    
  


  
Given that all internal states for the virtual devices are getting
cleared when releasing actions, also the timer for double click
event detection has to be canceled.

        Attachment #9005144 -
        Flags: review?(ato)
Assignee: nobody → hskupin
Status: NEW → ASSIGNED

    
    

    
  


  
The click status of a mouse action has to be carried forward
to the next "Perform Actions" call if the action sequence
hasn't been reset before.

        Attachment #9005145 -
        Flags: review?(ato)

    
    

    
  
Let me know if that is the correct approach to fix this problem. If yes, I will start a try build. Thanks.
Flags: needinfo?(ato)
Summary: Regression in Actions that use Keys.SHIFT → Double-click tracker is not reset in "Release Actions"

    
    

    
  
Comment on attachment 9005145 [details] [diff] [review]
[wdspec] Add action tests for double click state when releasing action chain

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

Yes, this looks correct.

        Attachment #9005145 -
        Flags: review?(ato) → review+

        Attachment #9005144 -
        Flags: review?(ato) → review+

    
    

    
  
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b306b75c312a
[marionette] Reset double click tracker when releasing actions. r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ec600e74178
[wdspec] Add action tests for double click state when releasing action chain. r=ato

    
    

    
  
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12772 for changes under testing/web-platform/tests

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/b306b75c312a
https://hg.mozilla.org/mozilla-central/rev/3ec600e74178
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox63:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

    
    

    
  
I think this (well at least
https://hg.mozilla.org/mozilla-central/rev/b306b75c312a) is a good
candidate for an uplift to beta.  What do you think?
Flags: needinfo?(hskupin)

    
    

    
  
It's too late for Firefox 62. The only possible option would be for ESR60, but given that I haven't heard problems with that yet, I wonder if that is necessary.

          status-firefox60:
          ? → wontfix

          status-firefox61:
          ? → wontfix

          status-firefox62:
          --- → wontfix

          status-firefox-esr60:
          --- → ?
Flags: needinfo?(hskupin)

    
    

    
  
Any thoughts on whether you want this on ESR60 or not?
Flags: needinfo?(hskupin)

    
    

    
  
Lets try to land the test-only patch for esr60 if it applies cleanly.
Flags: needinfo?(hskupin)
Whiteboard: [checkin-needed-beta]

    
  
Product: Testing → Remote Protocol

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: