Closed Bug 1354211 Opened 7 years ago Closed 6 years ago

Make Element Clear WebDriver conforming

Categories

(Remote Protocol :: Marionette, enhancement, P2)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 3 open bugs)

Details

Attachments

(10 files, 3 obsolete files)

Bug 1354211 - Consider <optgroup> when finding container element.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1354211 - Add shorthand for emulating DOM blur event.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1354211 - Add element.findClosest for finding ancestor by CSS selector.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1354211 - Use element.findClosest in element.getContainer.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1354211 - Add element.isReadOnly.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1354211 - Add element.isDisabled.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1354211 - Add element.isEditingHost and element.isEditable.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1354211 - Make WebDriver:ElementClear conforming to standard.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1354211 - Remove clearElement atom.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1354211 - Remove Mn test for WebDriver:ElementClear.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details 
The Element Clear command is implemented using Selenium JS fragment atoms in Marionette.

We should implement the command according to the WebDriver standard.  The steps are described in https://w3c.github.io/webdriver/webdriver-spec.html#element-clear.
Assignee: nobody → ato
Blocks: selenium-atoms, webdriver
Status: NEW → ASSIGNED
Priority: -- → P1
Writing tests for this, I discovered that the specification must also be wrong.  It checks hasAttribute("checked"), although it’s not clear to be why it’s checking the attribute state (as opposed to the property) and why it is possible to clear the value property of elements such as <input type=image> and <input type=hidden>.

I filed https://github.com/w3c/webdriver/issues/884 about this, but I suspect substantive changes are required to the specification before we can claim that Marionette’s implementation of Element Clear is spec conforming.
Priority: P1 → P3
Priority: P3 → P2
Andreas, are you still working on this bug?
Flags: needinfo?(ato)
Attached patch 0003-tmp.patch (obsolete) — — Splinter Review 

    
    

    
  
(In reply to Henrik Skupin (:whimboo) from comment #3)

> Andreas, are you still working on this bug?

I did some work on this back in April and I’ve attached the little
work that I did.  I started implementing keyboard interactability
but found it very hard to understand what the specification is
saying.  You can judge by the WIP patches yourself.

In any case, I’m happy to relinquish this bug to someone else.  If
you want to have a go at implementing this, feel free to do so.  I
will be busy with the window tracking work in the coming weeks.

(P.S. Sorry for the low quality of the code in the patches and
remember that they might not apply to m-c HEAD now.)
Assignee: ato → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(ato)

    
    

    
  
Since my last comment, whimboo implemented the keyboard interactability
concept in https://bugzil.la/1414322, which was the major blocker
for this.  I also noticed when looking at https://bugzil.la/1417224
that it would probably be less work to reimplement Element Clear
here because Marionette currently relies on a Selenium atom.
Assignee: nobody → ato
Depends on: 1414322

    
  

        Attachment #8928607 -
        Attachment is obsolete: true

    
  

        Attachment #8928608 -
        Attachment is obsolete: true

    
  

        Attachment #8928609 -
        Attachment is obsolete: true

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  
Status: NEW → ASSIGNED

    
    

    
  
Comment on attachment 8939249 [details]
Bug 1354211 - Make WebDriver:ElementClear conforming to standard.

https://reviewboard.mozilla.org/r/209674/#review215356

::: testing/marionette/interaction.js:309
(Diff revision 3)
> +  if (!element.isInView(el)) {
> +    element.scrollIntoView(el);
> +  }
> +  if (!element.isInView(el)) {
> +    throw new ElementNotInteractableError(
> +        pprint`Element ${el} could not be scrolled into view`);

Please note that editable elements can be interacted with even those are outside of the viewport. Given that clearing the data is closer to the send_keys method than click, we should consider not failing here. Maybe it might require some discussion for the spec, if that is not clear.

::: testing/marionette/interaction.js:314
(Diff revision 3)
> +        pprint`Element ${el} could not be scrolled into view`);
> +  }
> +
> +  let attr;
> +  if (element.isEditingHost(el)) {
> +    attr = "innerHTML";

Why do we need `innerHTML` here? Shouldn't we instead rely on https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent?

    
    

    
  
Comment on attachment 8939249 [details]
Bug 1354211 - Make WebDriver:ElementClear conforming to standard.

https://reviewboard.mozilla.org/r/209674/#review215356

> Please note that editable elements can be interacted with even those are outside of the viewport. Given that clearing the data is closer to the send_keys method than click, we should consider not failing here. Maybe it might require some discussion for the spec, if that is not clear.

The standard says we should check for both keyboard- and pointer
interactability (“element is not interactable”, which implies both).

I do tend to agree with you that following the same approach as
Element Send Keys might be better though.  Maybe you want to file
a bug, and possibly check what other implementations are doing?

> Why do we need `innerHTML` here? Shouldn't we instead rely on https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent?

WebDriver says we should clear the element’s innerHTML which also
clears the element’s subtree.  Not sure if textContent does that?

    
    

    
  
Comment on attachment 8939251 [details]
Bug 1354211 - Remove Mn test for WebDriver:ElementClear.

https://reviewboard.mozilla.org/r/209678/#review216584

        Attachment #8939251 -
        Flags: review?(dburns) → review+

    
    

    
  
Comment on attachment 8939242 [details]
Bug 1354211 - Consider <optgroup> when finding container element.

https://reviewboard.mozilla.org/r/209660/#review216586

        Attachment #8939242 -
        Flags: review?(dburns) → review+

    
    

    
  
Comment on attachment 8939249 [details]
Bug 1354211 - Make WebDriver:ElementClear conforming to standard.

https://reviewboard.mozilla.org/r/209674/#review216592

::: testing/web-platform/tests/webdriver/tests/element_clear.py:1
(Diff revision 3)
> +import pytest

these tests should be in /interaction as its in that part of the spec

        Attachment #8939249 -
        Flags: review?(dburns) → review-

    
    

    
  
Comment on attachment 8939243 [details]
Bug 1354211 - Add shorthand for emulating DOM blur event.

https://reviewboard.mozilla.org/r/209662/#review216588

        Attachment #8939243 -
        Flags: review?(dburns) → review+

    
    

    
  
Comment on attachment 8939246 [details]
Bug 1354211 - Add element.isReadOnly.

https://reviewboard.mozilla.org/r/209668/#review216596

        Attachment #8939246 -
        Flags: review?(dburns) → review+

    
    

    
  
Comment on attachment 8939247 [details]
Bug 1354211 - Add element.isDisabled.

https://reviewboard.mozilla.org/r/209670/#review216598

        Attachment #8939247 -
        Flags: review?(dburns) → review-

    
    

    
  
Comment on attachment 8939247 [details]
Bug 1354211 - Add element.isDisabled.

https://reviewboard.mozilla.org/r/209670/#review216600

        Attachment #8939247 -
        Flags: review- → review+

    
    

    
  
Comment on attachment 8939244 [details]
Bug 1354211 - Add element.findClosest for finding ancestor by CSS selector.

https://reviewboard.mozilla.org/r/209664/#review216602

        Attachment #8939244 -
        Flags: review?(dburns) → review+

    
    

    
  
Comment on attachment 8939245 [details]
Bug 1354211 - Use element.findClosest in element.getContainer.

https://reviewboard.mozilla.org/r/209666/#review216604

        Attachment #8939245 -
        Flags: review?(dburns) → review+

    
    

    
  
Comment on attachment 8939248 [details]
Bug 1354211 - Add element.isEditingHost and element.isEditable.

https://reviewboard.mozilla.org/r/209672/#review216606

        Attachment #8939248 -
        Flags: review?(dburns) → review+

    
    

    
  
Comment on attachment 8939250 [details]
Bug 1354211 - Remove clearElement atom.

https://reviewboard.mozilla.org/r/209676/#review216608

        Attachment #8939250 -
        Flags: review?(dburns) → review+

    
    

    
  
Comment on attachment 8939249 [details]
Bug 1354211 - Make WebDriver:ElementClear conforming to standard.

https://reviewboard.mozilla.org/r/209674/#review215356

> The standard says we should check for both keyboard- and pointer
> interactability (“element is not interactable”, which implies both).
> 
> I do tend to agree with you that following the same approach as
> Element Send Keys might be better though.  Maybe you want to file
> a bug, and possibly check what other implementations are doing?

With the standard you mean the WebDriver spec? I feel it's more an issue for that, which needs to be discussed. Not sure if checking other implementations make that much sense at this point.

> WebDriver says we should clear the element’s innerHTML which also
> clears the element’s subtree.  Not sure if textContent does that?

Please see my link above. It explains everything. Especially to highlight are the differences to innerHTML:

> Element.innerHTML returns the HTML as its name indicates. Quite often, in order to retrieve or write text within an element, people use innerHTML. However, textContent often has better performance because the text is not parsed as HTML. Moreover, using textContent can prevent XSS attacks.

    
    

    
  
Comment on attachment 8939249 [details]
Bug 1354211 - Make WebDriver:ElementClear conforming to standard.

https://reviewboard.mozilla.org/r/209674/#review215356

> With the standard you mean the WebDriver spec? I feel it's more an issue for that, which needs to be discussed. Not sure if checking other implementations make that much sense at this point.

Yes, WebDriver is the standard we are implementing here.

The standard should reflect what existing implementations do.

> Please see my link above. It explains everything. Especially to highlight are the differences to innerHTML:
> 
> > Element.innerHTML returns the HTML as its name indicates. Quite often, in order to retrieve or write text within an element, people use innerHTML. However, textContent often has better performance because the text is not parsed as HTML. Moreover, using textContent can prevent XSS attacks.

I’m not sure if there’s any particular advantage in using
textContent.  I checked and it does remove subtrees under the
contenteditable element, but functionally, they appear equivalent.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8939249 [details]
Bug 1354211 - Make WebDriver:ElementClear conforming to standard.

https://reviewboard.mozilla.org/r/209674/#review217134

::: testing/web-platform/tests/webdriver/tests/interaction/element_clear.py:339
(Diff revision 4)
> +    assert session.execute_script("return window.scrollY") == 0
> +
> +    response = element_clear(session, element)
> +    assert_success(response)
> -        assert element.property("value") == ""
> +    assert element.property("value") == ""
> +    assert session.execute_script("return window.scrollY") > 0

This should use `getBoundingClientRect` and check the values are positive numbers as well. Might be worth adding a test for an element that is definitely out of the viewport and can be scrolled to

        Attachment #8939249 -
        Flags: review?(dburns) → review-

    
    

    
  
Comment on attachment 8939249 [details]
Bug 1354211 - Make WebDriver:ElementClear conforming to standard.

https://reviewboard.mozilla.org/r/209674/#review217134

> This should use `getBoundingClientRect` and check the values are positive numbers as well. Might be worth adding a test for an element that is definitely out of the viewport and can be scrolled to

I’m not sure I understand what you want to test with
getBoundingClientRect?

Also this <input> element is definitely outside the viewport
(margin-top: 200vh).

    
  

        Attachment #8939249 -
        Flags: review?(dburns)

    
  
Blocks: 1429403

    
    

    
  
Comment on attachment 8939249 [details]
Bug 1354211 - Make WebDriver:ElementClear conforming to standard.

https://reviewboard.mozilla.org/r/209674/#review217134

> I’m not sure I understand what you want to test with
> getBoundingClientRect?
> 
> Also this <input> element is definitely outside the viewport
> (margin-top: 200vh).

just checking that the `scrollY` is not sufficient to gaurantee it has moved into the viewport. if its outside of the viewport then `top`, `left` will be negative. If the element has been scrolled to then it will have positive number

    
    

    
  
Comment on attachment 8939249 [details]
Bug 1354211 - Make WebDriver:ElementClear conforming to standard.

https://reviewboard.mozilla.org/r/209674/#review217134

    
    

    
  
Comment on attachment 8939249 [details]
Bug 1354211 - Make WebDriver:ElementClear conforming to standard.

https://reviewboard.mozilla.org/r/209674/#review217134

> just checking that the `scrollY` is not sufficient to gaurantee it has moved into the viewport. if its outside of the viewport then `top`, `left` will be negative. If the element has been scrolled to then it will have positive number

Oh I see now, thanks for explaining.  I should be able to reuse
the old scrolling into view test for this.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8939249 [details]
Bug 1354211 - Make WebDriver:ElementClear conforming to standard.

https://reviewboard.mozilla.org/r/209674/#review218104

::: testing/web-platform/tests/webdriver/tests/interaction/element_clear.py:355
(Diff revision 5)
> +    # check if element cleared is scrolled into view
> +    rect = session.execute_script("""
> +        let [input] = arguments;
> +        return input.getBoundingClientRect();
> +        """, args=(element,))
> +    pageDict = {"innerHeight": session.execute_script("return window.innerHeight"),

Do you think this would be better with one `execute_script` instead of the multiple calls.

        Attachment #8939249 -
        Flags: review?(dburns) → review+

    
    

    
  
Comment on attachment 8939249 [details]
Bug 1354211 - Make WebDriver:ElementClear conforming to standard.

https://reviewboard.mozilla.org/r/209674/#review218104

> Do you think this would be better with one `execute_script` instead of the multiple calls.

Sure, that will be clearer.  I just copied this from the old test.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4bf05eb6e9b6
Consider <optgroup> when finding container element. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/4cdb2c2eb4c4
Add shorthand for emulating DOM blur event. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/9350c2e8e01f
Add element.findClosest for finding ancestor by CSS selector. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/1c2ba38f6989
Use element.findClosest in element.getContainer. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/65547df52684
Add element.isReadOnly. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/81c470ac944a
Add element.isDisabled. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/66f58d8f3355
Add element.isEditingHost and element.isEditable. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/f9f835bce31b
Make WebDriver:ElementClear conforming to standard. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/9ab0d72e5e0b
Remove clearElement atom. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/15b42d200275
Remove Mn test for WebDriver:ElementClear. r=automatedtester

    
  
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: