Closed Bug 1448792 Opened 6 years ago Closed 6 years ago

Make WebDriver:ElementSendKeys for <input type=file> WebDriver conforming

Categories

(Remote Protocol :: Marionette, enhancement, P1)

Version 3
enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug, )

Details

Attachments

(5 files)

Originally filed as: https://github.com/mozilla/geckodriver/issues/1214

If `sendKeysToElement` is called multiple times for the same element, former already uploaded files are kept for another upload. As such the list of files is accumulating each time you call this method.

The problem is in `uploadFile`:

https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/testing/marionette/interaction.js#487-488

I do not see anything in the WebDriver spec which tells that already set files should be taken into account.

Also I don't see that we actually split the path in case of multiple to be uploaded files. But that might be covered by a different bug.

Andreas, would you mind checking too? I think it is something we should fix soonish. I could work on it.
Flags: needinfo?(ato)
It is intentional that multiple calls to the Element Send Keys
command on an <input type=file multiple> appends.  See substep 10
here:

    https://w3c.github.io/webdriver/webdriver-spec.html#dfn-element-send-keys
Flags: needinfo?(ato)
(In reply to Andreas Tolfsen ‹:ato› from comment #1)
> It is intentional that multiple calls to the Element Send Keys
> command on an <input type=file multiple> appends.  See substep 10
> here:
> 
>     https://w3c.github.io/webdriver/webdriver-spec.html#dfn-element-send-keys

Oh, you refer to substep 10.6? It means that only for `multiple` we take already set files for element into account. 

Given that we do not check for `multiple` we would also append the new file for `single` file uploads, right?
It should not be possible to append more files to <input type=file>.
That should only be possible for <input type=file multiple> which
has a FileList object associated with the .files property.

It does look like Marionette does not honour this and always appends
to the element’s FileList:

    https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/testing/marionette/interaction.js#487-488
Yes, that's what I was saying in comment 0 too. Let me create some additional tests and make the file input WebDriver spec conforming.
Assignee: nobody → hskupin
Blocks: webdriver
Status: NEW → ASSIGNED
Priority: -- → P1
Summary: Multiple calls to WebDriver:sendKeysToElement for the same file element concatenates new files → Make WebDriver:sendKeysToElement for `input type=file` webdriver conforming
I started to work on this bug and and stumbled over a spec definition which I think is wrong. But I would like to get some feedback first before filing a webdriver issue on Github. Specifically this is about the `multiple` attribute. Step 10.6 of the spec says:

> 6. Complete implementation specific steps equivalent to setting the selected files on the input. If multiple is true files are be appended to element’s selected files. 

Given that for real user interactions already selected files are getting replaced when new files have been chosen, we should do the same. When reading the above last sentence my understanding is that we are expected to add all the new files, to the list of existing ones. IMO this violates with the definition in the HTML spec.

Note that for attribute `multiple` we pass-in all the files at once, separated by a new line character.
Flags: needinfo?(ato)
Flags: needinfo?(dburns)
Also the WebDriver spec only refers to the `input` event which should be fired when a file or multiple files have been selected. But additionally also a `change` event has to be fired:

> Queue a task to first update the element's selected files so that it represents the user's selection, then fire an event named input at the input element, with the bubbles attribute initialized to true, and finally fire an event named change at the input element, with the bubbles attribute initialized to true.

Our current implementation also includes the following events before setting the files:

>   event.mouseover(el);
>   event.mousemove(el);
>   event.mousedown(el);
>   el.focus();
>   event.mouseup(el);
>   event.click(el);

None of those events are listed in the  WebDriver spec. So do we need them or not?
(In reply to Henrik Skupin (:whimboo) from comment #5)

> I started to work on this bug and and stumbled over a spec
> definition which I think is wrong. But I would like to get
> some feedback first before filing a webdriver issue on
> Github. Specifically this is about the `multiple` attribute. Step
> 10.6 of the spec says:
> 
> > 6. Complete implementation specific steps equivalent to setting
> > the selected files on the input. If multiple is true files are
> > be appended to element’s selected files.
> 
> Given that for real user interactions already selected files are
> getting replaced when new files have been chosen, we should do
> the same. When reading the above last sentence my understanding
> is that we are expected to add all the new files, to the list of
> existing ones. IMO this violates with the definition in the HTML
> spec.

It’s a deliberate design choice in WebDriver that multiple calls
to Element Send Keys on <input type=file multiple> will append more
files rather than replace them.  This is the only way to upload
multiple files through WebDriver at the moment.

There is more discussion in
https://github.com/w3c/webdriver/issues/1230 regarding providing a
specialised API for upload files instead of overloading the meaning
of Element Send Keys, but there does not appear to be any consensus
there yet.

> Note that for attribute `multiple` we pass-in all the files at
> once, separated by a new line character.

That seems wrong.  The expectation is for one file to be passed at a
time if I understand how other drivers work.

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

> Also the WebDriver spec only refers to the `input` event
> which should be fired when a file or multiple files have been
> selected. But additionally also a `change` event has to be fired:
> 
> > Queue a task to first update the element's selected files so
> > that it represents the user's selection, then fire an event
> > named input at the input element, with the bubbles attribute
> > initialized to true, and finally fire an event named change at
> > the input element, with the bubbles attribute initialized to
> > true.
> 
> Our current implementation also includes the following events
> before setting the files:
> 
> >   event.mouseover(el);
> >   event.mousemove(el);
> >   event.mousedown(el);
> >   el.focus();
> >   event.mouseup(el);
> >   event.click(el);
> 
> None of those events are listed in the WebDriver spec. So do we
> need them or not?

Ideally which events to be fired should be derived from HTML, which
has an appendix to this end. <input type=file> is special because
WebDriver does not allow direct interaction with the element: the
attempt to send keys/click the element is intercepted and special
ad-hoc behaviour is enforced.

For this reason we need to call out specifically which events to
fire in this particular case.  If WebDriver is missing some of the
events, we should complement the list.
Flags: needinfo?(ato)
Summary: Make WebDriver:sendKeysToElement for `input type=file` webdriver conforming → Make WebDriver:ElementSendKeys for <input type=file> WebDriver conforming
looks like :ato answered this
Flags: needinfo?(dburns)
(In reply to Andreas Tolfsen 「:ato」 from comment #7)
> It’s a deliberate design choice in WebDriver that multiple calls
> to Element Send Keys on <input type=file multiple> will append more
> files rather than replace them.  This is the only way to upload
> multiple files through WebDriver at the moment.

As discussed today this is not true and other drivers like the chromedriver already implemented multiple uploads by allowing a list of files passed in as text separated by `\n`. That way the files will be set at once, and no appending has to take place.

There are still a couple of open questions, which need to be solved. I refer to https://github.com/w3c/webdriver/issues/1263 here, and lets not discuss it here. My work will be blocked for now.

> There is more discussion in
> https://github.com/w3c/webdriver/issues/1230 regarding providing a
> specialised API for upload files instead of overloading the meaning
> of Element Send Keys, but there does not appear to be any consensus
> there yet.

That is something different.

> Ideally which events to be fired should be derived from HTML, which
> has an appendix to this end. <input type=file> is special because
> WebDriver does not allow direct interaction with the element: the
> attempt to send keys/click the element is intercepted and special
> ad-hoc behaviour is enforced.
> 
> For this reason we need to call out specifically which events to
> fire in this particular case.  If WebDriver is missing some of the
> events, we should complement the list.

In the case of the missing `change` event a lot of people will wait for I landed a patch via https://github.com/w3c/webdriver/pull/1262, which will still need a test being added here.
(In reply to Henrik Skupin (:whimboo) from comment #9)

> (In reply to Andreas Tolfsen 「:ato」 from comment #7)
> 
> > It’s a deliberate design choice in WebDriver that multiple
> > calls to Element Send Keys on <input type=file multiple> will
> > append more files rather than replace them.  This is the only
> > way to upload multiple files through WebDriver at the moment.
> 
> As discussed today this is not true and other drivers like the
> chromedriver already implemented multiple uploads by allowing a
> list of files passed in as text separated by `\n`. That way the
> files will be set at once, and no appending has to take place.

You omitted that IEDriver and Firefox _does not_ split on new line,
so I wouldn’t say there is concensus amongst implementations
here.  It does not follow that by Chrome implementing this, that the
behaviour described is the correct.

Clarification in the specification I feel is necessary to make
progress on this bug.

Also, I have a follow-up question: Do other drivers implement the
multiple calls append approach?

As I mentioned in
https://github.com/w3c/webdriver/issues/1263#issuecomment-393192639
I have reservations against splitting on \n because of the blatant
correctness issues.

> > There is more discussion in
> > https://github.com/w3c/webdriver/issues/1230 regarding providing
> > a specialised API for upload files instead of overloading the
> > meaning of Element Send Keys, but there does not appear to be
> > any consensus there yet.
> 
> That is something different.

Yes, but it gives background to the discussion which is useful for
deciding whether multiple file uploads with \n should be yanked from
the spec or not.  If there is commitment to solve multiple file
uploads by introducing a new endpoint, that has direct effect upon
what decision is reached with regards to aligning Element Send Keys
with the specification.
(In reply to Andreas Tolfsen 「:ato」 from comment #10)
> You omitted that IEDriver and Firefox _does not_ split on new line,
> so I wouldn’t say there is concensus amongst implementations
> here.  It does not follow that by Chrome implementing this, that the
> behaviour described is the correct.

Well, both IEDriver and Firefox haven't implemented the multiple file upload yet. It was just a workaround you were using.

Anyway, it doesn't make sense to discuss it more on this bug. Lets really get some traction on the w3c issue.

> Also, I have a follow-up question: Do other drivers implement the
> multiple calls append approach?

Chromedriver doesn't append, but only set the file list.


I will upload a patch for now which will bring us to what the spec currently proposes. Once everything has been clarified I will continue on this bug. But for now it it blocked.
Whiteboard: [blocked by spec]
Comment on attachment 8982628 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys wdspec conforming for file uploads.

https://reviewboard.mozilla.org/r/248592/#review254822


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/interaction.js:496
(Diff revision 1)
> -  try {
> +    try {
> -    file = await File.createFromFileName(path);
> +      fileObjects.push(await File.createFromFileName(files[i]));
> -  } catch (e) {
> +    } catch (e) {
> -    throw new InvalidArgumentError("File not found: " + path);
> +      throw new InvalidArgumentError("File not found: " + files[i]);
> -  }
> +    }
> +  };

Error: Unnecessary semicolon. [eslint: no-extra-semi]
Comment on attachment 8982629 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys fire change event for <input type=file>.

https://reviewboard.mozilla.org/r/248594/#review254828


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/interaction.js:496
(Diff revision 1)
>      try {
>        fileObjects.push(await File.createFromFileName(files[i]));
>      } catch (e) {
>        throw new InvalidArgumentError("File not found: " + files[i]);
>      }
>    };

Error: Unnecessary semicolon. [eslint: no-extra-semi]
Comment on attachment 8982629 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys fire change event for <input type=file>.

https://reviewboard.mozilla.org/r/248594/#review254986


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/interaction.js:496
(Diff revision 2)
>      try {
>        fileObjects.push(await File.createFromFileName(files[i]));
>      } catch (e) {
>        throw new InvalidArgumentError("File not found: " + files[i]);
>      }
>    };

Error: Unnecessary semicolon. [eslint: no-extra-semi]
Attachment #8986108 - Flags: review?(ato)
Attachment #8982626 - Flags: review?(ato)
Attachment #8982627 - Flags: review?(ato)
Attachment #8982628 - Flags: review?(ato)
Attachment #8982629 - Flags: review?(ato)
Comment on attachment 8982628 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys wdspec conforming for file uploads.

https://reviewboard.mozilla.org/r/248592/#review257832

::: testing/marionette/interaction.js:476
(Diff revision 2)
>   *
>   * @param {HTMLInputElement} el
>   *     An <tt>&lt;input type=file&gt;</tt> element.
> - * @param {string} path
> - *     Full path to file.
> + * @param {string} text
> + *     Full path to a single file, or if multiple files then paths
> + *     are separated by a new line (\n).

Please note that this still needs clarification. But otherwise this patch should be complete.
(In reply to Henrik Skupin (:whimboo) from comment #25)
> > + * @param {string} text
> > + *     Full path to a single file, or if multiple files then paths
> > + *     are separated by a new line (\n).
> 
> Please note that this still needs clarification. But otherwise this patch
> should be complete.

I discussed this with David on Friday. And he wanted to look for an alternative character. As it looks like no binding is using this separator yet, nor has multiple uploads implemented. So we are fine to make changes.

Adding needinfo for David for the spec updates.
Flags: needinfo?(dburns)
Comment on attachment 8986108 [details]
Bug 1448792 - [wdclient] Fix Element.clear() for unknown "url" property.

https://reviewboard.mozilla.org/r/251564/#review257844
Attachment #8986108 - Flags: review?(ato) → review+
Comment on attachment 8982626 [details]
Bug 1448792 - [wdspec] Add basic tests and user prompt tests for Element Send Keys.

https://reviewboard.mozilla.org/r/248588/#review257846

::: commit-message-db78b:1
(Diff revision 2)
> +Bug 1448792 - [wdspec] Add basic and user prompt tests for element_send_keys.

Something wrong with the grammar here.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/send_keys.py:25
(Diff revision 2)
>      value = assert_success(response)
>      assert value is None
> +
> +
> +@pytest.mark.parametrize("type", [True, None, 1, [], {}])
> +def test_invalid_text_type(session, type):

type is a Python built-in and we shouldn’t override it.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/send_keys.py:25
(Diff revision 2)
>      value = assert_success(response)
>      assert value is None
> +
> +
> +@pytest.mark.parametrize("type", [True, None, 1, [], {}])
> +def test_invalid_text_type(session, type):

type is unused.
Comment on attachment 8982627 [details]
Bug 1448792 - [wdspec] Create global fixture and assert methods for event checks.

https://reviewboard.mozilla.org/r/248590/#review257848

::: testing/web-platform/tests/webdriver/tests/support/asserts.py:148
(Diff revision 2)
> +    actual_events = session.execute_script("return window.events")
> +    for expected_event in expected_events:
> +        assert expected_event in actual_events
> +
> +
> +def assert_same_events(session, expected_events):

This name confuses me.  Repurposing terminology from other test
frameworks might be a good idea, for example assert_events_equal
or something.

::: testing/web-platform/tests/webdriver/tests/support/fixtures.py:116
(Diff revision 2)
> +    """Register listeners for tracked events on element."""
> +    def add_event_listeners(element, tracked_events):
> +        element.session.execute_script("""
> +            var trackedEvents = arguments[1];
> +
> +            window.events = [];

This function effectively resets the events we have seen, which I
wouldn’t expect from the function name.

::: testing/web-platform/tests/webdriver/tests/support/fixtures.py:119
(Diff revision 2)
> +            var trackedEvents = arguments[1];
> +
> +            window.events = [];
> +
> +            for (var i = 0; i < trackedEvents.length; i++) {
> +              arguments[0].addEventListener(trackedEvents[i], function (event) {

I know you didn’t write this, but can we extract arguments[0] at
the top of the function and give it a useful name as well?
Attachment #8982627 - Flags: review?(ato) → review+
Comment on attachment 8982628 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys wdspec conforming for file uploads.

https://reviewboard.mozilla.org/r/248592/#review257850

I can’t help but feel somewhat disgusted by the nastiness of this
particular WebDriver API.  If other implementations do not yet
implement the “split on \n” behaviour I don’t understand why we
have to be the first to commit to it.

::: commit-message-8c21c:4
(Diff revision 2)
> +Bug 1448792 - [marionette][wdspec] Make element_send_keys wdspec conforming for file uploads.
> +
> +This patch fixes remaining bugs in Marionette for uploading
> +files via "element_send_keys", and also adds support for

I find these random strings rather confusing.  Can we please use
the command’s actual name, which is Element Send Keys, or the type
command name in Marionette, which is WebDriver:ElementSendKeys?

::: testing/marionette/interaction.js:468
(Diff revision 2)
>  /**
> - * Appends <var>path</var> to an <tt>&lt;input type=file&gt;</tt>'s
> + * Appends <var>text</var> to an <tt>&lt;input type=file&gt;</tt>'s
>   * file list.
>   *
>   * @param {HTMLInputElement} el
>   *     An <tt>&lt;input type=file&gt;</tt> element.
> - * @param {string} path
> - *     Full path to file.
> + * @param {string} text
> + *     Full path to a single file, or if multiple files then paths
> + *     are separated by a new line (\n).
>   *
>   * @throws {InvalidArgumentError}
> - *     If <var>path</var> can not be found.
> + *     If <var>path</var> cannot be found, or when multiple
> + *     files aren't allowed.
>   */

Update to use RST formatting.

::: testing/marionette/interaction.js:479
(Diff revision 2)
> - * @param {string} path
> - *     Full path to file.
> + * @param {string} text
> + *     Full path to a single file, or if multiple files then paths
> + *     are separated by a new line (\n).
>   *
>   * @throws {InvalidArgumentError}
> - *     If <var>path</var> can not be found.
> + *     If <var>path</var> cannot be found, or when multiple

path doesn’t exist.

::: testing/marionette/interaction.js:482
(Diff revision 2)
> -interaction.uploadFile = async function(el, path) {
> -  let file;
> +interaction.uploadFiles = async function(el, text) {
> +  let fileObjects = [];
> +
> +  let files = text.split("\n");

Splitting should be done externally to this function.  This function
should receive an array of paths to set.

::: testing/marionette/interaction.js:485
(Diff revision 2)
> + *     files aren't allowed.
>   */
> -interaction.uploadFile = async function(el, path) {
> -  let file;
> +interaction.uploadFiles = async function(el, text) {
> +  let fileObjects = [];
> +
> +  let files = text.split("\n");

Also this isn’t an array of files, but of file _paths_, which makes
all the difference.

::: testing/marionette/interaction.js:486
(Diff revision 2)
> +  let isMultiple = el.hasAttribute("multiple");
> +
> +  if (isMultiple) {

isMultiple isn’t used more than once, so you can drop the variable
assignment here.

::: testing/marionette/interaction.js:497
(Diff revision 2)
> +  } else if (files.length > 1) {
> +    throw new InvalidArgumentError(
> +        pprint`Element ${el} doesn't accept multiple files`);
> +  }
> +
> +  for (let i = 0; i < files.length; i++) {

Not a problem of course, but a modern for-of loop would be easier
to read.

::: testing/marionette/interaction.js:500
(Diff revision 2)
> +  }
> +
> +  for (let i = 0; i < files.length; i++) {
> -  try {
> +    try {
> -    file = await File.createFromFileName(path);
> +      let file = await File.createFromFileName(files[i]);
> +      fileObjects.push(file);

If this fails we will throw an InvalidArgumentError, but I suppose
that is unlikely to happen?

::: testing/marionette/interaction.js
(Diff revision 2)
> -  // <input type=file> opens OS widget dialogue
> -  // which means the mousedown/focus/mouseup/click events
> -  // occur before the change event
> -  event.mouseover(el);
> -  event.mousemove(el);
> -  event.mousedown(el);
> -  el.focus();
> -  event.mouseup(el);
> -  event.click(el);

It’s not clear to me why we aren’t firing these events anymore.
That seems like a regression.

::: testing/marionette/interaction.js:593
(Diff revision 2)
> -    await interaction.uploadFile(el, value);
> +    el.focus();
> +    await interaction.uploadFiles(el, value);

Seems problematic to fire one event here, but the rest inside
uploadFiles?

I also think it’s a bad idea to change the legacy send keys behaviour.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/file_upload.py:7
(Diff revision 2)
> +@pytest.fixture
> +def create_file(tmpdir_factory):
> +    def inner(filename):
> +        fh = tmpdir_factory.mktemp("tmp").join(filename)
> +        fh.write(filename)
> +
> +        return fh
> +        return str(fh)
> +
> +    inner.__name__ = "create_file"
> +    return inner

Duplicate?

::: testing/web-platform/tests/webdriver/tests/element_send_keys/file_upload.py:32
(Diff revision 2)
> +def test_empty_text(session):
> +    session.url = inline("<input type=file>")
> +    element = session.find.css("input", all=False)
> +
> +    response = element_send_keys(session, element, "")
> +    assert_error(response, "invalid argument")

Is this behaviour called out in the spec?

::: testing/web-platform/tests/webdriver/tests/element_send_keys/file_upload.py:53
(Diff revision 2)
> +def test_multiple_files_last_path_not_found(session, create_file):
> +    files = [create_file("foo"), create_file("bar"), "foo bar"]
> +
> +    session.url = inline("<input type=file multiple>")
> +    element = session.find.css("input", all=False)
> +
> +    response = element_send_keys(session, element,
> +                                 map_files_to_multiline_text(files))
> +    assert_error(response, "invalid argument")

You should probably check that no fiels have been set here, in case
an implementation does lazy evaluation of the paths.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/file_upload.py:70
(Diff revision 2)
> +    response = element_send_keys(session, element,
> +                                 map_files_to_multiline_text(files))
> +    assert_error(response, "invalid argument")

Check that no files have been set.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/file_upload.py:104
(Diff revision 2)
> +    # Reset already uploaded files
> +    element.clear()

You need to assert the file count before and after clearing.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/file_upload.py:115
(Diff revision 2)
> +
> +    assert_files_uploaded(session, element, second_files)
> +
> +
> +def test_single_file(session, create_file):
> +    file = create_file("foo")

file is a Python built-in we shouldn’t overload.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/file_upload.py:126
(Diff revision 2)
> +    assert_success(response)
> +
> +    assert_files_uploaded(session, element, [file])
> +
> +
> +def test_single_file_send_twice(session, create_file):

I would say “replaces” or something in this test title.  I can’t
deduce the correct behaviour from “twice”.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/file_upload.py:142
(Diff revision 2)
> +    assert_success(response)
> +
> +    assert_files_uploaded(session, element, [second_file])
> +
> +
> +def test_single_file_send_twice_with_multiple_attribute(session, create_file):

Something about second invocation appends?

::: testing/web-platform/tests/webdriver/tests/support/asserts.py:121
(Diff revision 2)
> +            var reader = new FileReader();
> +            reader.onload = function(event) {
> +              resolve(reader.result);
> +            };
> +            reader.readAsText(files[index]);
> +        """, [element, file_index])

Tuple

::: testing/web-platform/tests/webdriver/tests/support/asserts.py:133
(Diff revision 2)
> +            for (var i = 0; i < fileList.length; i++) {
> +              files.push(fileList[i].name);
> +            }
> +
> +            return files;
> +        """, args=[element])

Tuple
Attachment #8982628 - Flags: review?(ato) → review-
Comment on attachment 8982629 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys fire change event for <input type=file>.

https://reviewboard.mozilla.org/r/248594/#review257854

::: commit-message-dec61:1
(Diff revision 3)
> +Bug 1448792 - [marionette][wdspec] "Element Send Keys" for file upload has to fire "change" event.

Use imperative language in the commit message to describe _what_
it being changed rather than a description of the normative behaviour
it ought to have.  For example “Make Element Send Keys fire change
event for <input type=file>” reads better.

::: commit-message-dec61:3
(Diff revision 3)
> +Adds a Wdspec test to proof the latest WebDriver specification
> +changes for https://github.com/w3c/webdriver/pull/1262.

Incorrect use of “proof”.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/events.py:35
(Diff revision 3)
> +        "focus",
> +        "input",
> +        "change",
> +    ]
> +
> +    file = create_file("foo")

file is a Python built-in which we should try not to override.
Attachment #8982629 - Flags: review?(ato) → review+
Comment on attachment 8982629 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys fire change event for <input type=file>.

https://reviewboard.mozilla.org/r/248594/#review257858

::: testing/web-platform/tests/webdriver/tests/element_send_keys/events.py:35
(Diff revision 3)
> +        "focus",
> +        "input",
> +        "change",
> +    ]
> +
> +    file = create_file("foo")

create_file is missing an import.
Comment on attachment 8982626 [details]
Bug 1448792 - [wdspec] Add basic tests and user prompt tests for Element Send Keys.

https://reviewboard.mozilla.org/r/248588/#review257872

::: testing/web-platform/tests/webdriver/tests/element_send_keys/send_keys.py:24
(Diff revision 2)
>      response = element_send_keys(session, element, "foo")
>      value = assert_success(response)
>      assert value is None
> +
> +
> +@pytest.mark.parametrize("type", [True, None, 1, [], {}])

Are we testing the absence of a text field anywhere?
Attachment #8982626 - Flags: review?(ato) → review+
Comment on attachment 8982626 [details]
Bug 1448792 - [wdspec] Add basic tests and user prompt tests for Element Send Keys.

https://reviewboard.mozilla.org/r/248588/#review257872

> Are we testing the absence of a text field anywhere?

You mean sending the command without a body? I can definitly add this, and would file a follow-up for all the other `POST` commands.
Comment on attachment 8982627 [details]
Bug 1448792 - [wdspec] Create global fixture and assert methods for event checks.

https://reviewboard.mozilla.org/r/248590/#review257848

> This name confuses me.  Repurposing terminology from other test
> frameworks might be a good idea, for example assert_events_equal
> or something.

I was re-using the same terminology as we have for elements which is `assert_same_element`. I will change it, and maybe we should change the other asserts too?

> This function effectively resets the events we have seen, which I
> wouldn’t expect from the function name.

Oh, right. We should only create the array if `events` hasn't been defined yet.

> I know you didn’t write this, but can we extract arguments[0] at
> the top of the function and give it a useful name as well?

Sure.
Comment on attachment 8982626 [details]
Bug 1448792 - [wdspec] Add basic tests and user prompt tests for Element Send Keys.

https://reviewboard.mozilla.org/r/248588/#review257872

> You mean sending the command without a body? I can definitly add this, and would file a follow-up for all the other `POST` commands.

Yes.  And that sounds like a good plan.
Comment on attachment 8982627 [details]
Bug 1448792 - [wdspec] Create global fixture and assert methods for event checks.

https://reviewboard.mozilla.org/r/248590/#review257848

> I was re-using the same terminology as we have for elements which is `assert_same_element`. I will change it, and maybe we should change the other asserts too?

Yes, assert_element_equal is probably better, but don’t feel compelled
to change that as part of this changeset.
Comment on attachment 8982628 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys wdspec conforming for file uploads.

https://reviewboard.mozilla.org/r/248592/#review257832

> Please note that this still needs clarification. But otherwise this patch should be complete.

We don't have to track two issues for that. See the one which I already have set for this.
Comment on attachment 8982628 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys wdspec conforming for file uploads.

https://reviewboard.mozilla.org/r/248592/#review257850

I will let David and others decide. Until then this patch should be on-hold after applying your review comments.

> I find these random strings rather confusing.  Can we please use
> the command’s actual name, which is Element Send Keys, or the type
> command name in Marionette, which is WebDriver:ElementSendKeys?

This is only one left-over which slipped through. I fixed that in the 2nd commit of this patch series.

> Update to use RST formatting.

I don't understand. All other code like in driver.js uses `<var>..`, wo why should I change the formatting in my patch now? We should better do a global update to cover that in one step if it is invalid.

> Splitting should be done externally to this function.  This function
> should receive an array of paths to set.

We can move this out. It is indeed even better because that way it doesn't collide with the old legacy implementation, which doesn't support multiple file uploads.

> Also this isn’t an array of files, but of file _paths_, which makes
> all the difference.

The naming was copied from the spec. But if you think it's better to use `paths` I will do it.

> isMultiple isn’t used more than once, so you can drop the variable
> assignment here.

I refactored the code for the last update, and indeed it's used only once now.

> Not a problem of course, but a modern for-of loop would be easier
> to read.

Oh yes. I was too carefully using that stuff lately because of all the work in wdspec tests, where it cannot be used due to not being compatible with IE. I will update that part.

> If this fails we will throw an InvalidArgumentError, but I suppose
> that is unlikely to happen?

Very unlikely but I moved it out of the try/catch block now.

> It’s not clear to me why we aren’t firing these events anymore.
> That seems like a regression.

`el.focus()` is already fired in `webdriverSendKeysToElement()` and `legacySendKeysToElement()` so it doesn't have to be done twice. Also we don't call `blur()` for any other code path in `ElementSendKeys`, so to be literaly conformant we have to remove it here.

Thinking more about this and what you said earlier in that review, `uploadFile` should only handle the file selection for the input. So having the events being fired in both of the outer methods, sounds also more appriate. I will move the `input` and `change` events.

Further nothing in the WebDriver spec actually mentions that further events have to be fired. Also by reading the HTML spec nothing refers to those events. Given that this patch should make us spec conformant I removed them.

Note that I also haven't seen yet a use-case for `click`, or any of those mouse events (which we should even replace if necessary at all with key events - the code is in `Element Send Keys`). Only `input` and `change` is important for users to handle the real file uploads.

> Seems problematic to fire one event here, but the rest inside
> uploadFiles?
> 
> I also think it’s a bad idea to change the legacy send keys behaviour.

See my last comment for most of this comment. Further we could restore all the additional events for the legacy behavior.

> Duplicate?

Yes, a busted rebase caused to remain this broken fixture. But I was sure that I fixed that. It's done now.

> Is this behaviour called out in the spec?

> Let files be the result of splitting text on the newline (\n) character. 
> If files is of 0 length, return an error with error code invalid argument.

For some languages an empty string here means length of 0. See https://github.com/w3c/webdriver/pull/1261#issuecomment-393311826.

> You should probably check that no fiels have been set here, in case
> an implementation does lazy evaluation of the paths.

Good idea! Will add this.

Actually this revealed another bug in the `assert_files_uploaded` method when the expected length of files is 0.

> You need to assert the file count before and after clearing.

I don't see a benefit here given that it should be a test for `Element Clear`.
Comment on attachment 8982629 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys fire change event for <input type=file>.

https://reviewboard.mozilla.org/r/248594/#review257858

> create_file is missing an import.

Not sure what you mean here. `create_file` is a fixture and as such will automatically be imported by pytest.
Comment on attachment 8982629 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys fire change event for <input type=file>.

https://reviewboard.mozilla.org/r/248594/#review257854

> Incorrect use of “proof”.

Maybe `to ensure`? Or what else would you propose?
Comment on attachment 8982628 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys wdspec conforming for file uploads.

https://reviewboard.mozilla.org/r/248592/#review257850

> I don't understand. All other code like in driver.js uses `<var>..`, wo why should I change the formatting in my patch now? We should better do a global update to cover that in one step if it is invalid.

jsautodoc for Sphinx doesn’t support arbitrary HTML like jsdoc did,
so all instances of <var> and friends are stripped from the output
HTML on firefox-source-docs.m.o.

It makes sense to fixup this particular API documentation whilst
you’re making changes to the function.

> We can move this out. It is indeed even better because that way it doesn't collide with the old legacy implementation, which doesn't support multiple file uploads.

Yes, good point.

> The naming was copied from the spec. But if you think it's better to use `paths` I will do it.

Oh right.  I think the spec deserves a language tightening too if
you want an idea for a simple patch…

> `el.focus()` is already fired in `webdriverSendKeysToElement()` and `legacySendKeysToElement()` so it doesn't have to be done twice. Also we don't call `blur()` for any other code path in `ElementSendKeys`, so to be literaly conformant we have to remove it here.
> 
> Thinking more about this and what you said earlier in that review, `uploadFile` should only handle the file selection for the input. So having the events being fired in both of the outer methods, sounds also more appriate. I will move the `input` and `change` events.
> 
> Further nothing in the WebDriver spec actually mentions that further events have to be fired. Also by reading the HTML spec nothing refers to those events. Given that this patch should make us spec conformant I removed them.
> 
> Note that I also haven't seen yet a use-case for `click`, or any of those mouse events (which we should even replace if necessary at all with key events - the code is in `Element Send Keys`). Only `input` and `change` is important for users to handle the real file uploads.

> el.focus() is already fired in webdriverSendKeysToElement()
> and legacySendKeysToElement() so it doesn't have to be done
> twice. Also we don't call blur() for any other code path in
> ElementSendKeys, so to be literaly conformant we have to remove it
> here.
> 
> Thinking more about this and what you said earlier in that review,
> uploadFile should only handle the file selection for the input. So
> having the events being fired in both of the outer methods, sounds
> also more appriate. I will move the input and change events.

Makes sense.

> Further nothing in the WebDriver spec actually mentions that
> further events have to be fired. Also by reading the HTML spec
> nothing refers to those events. Given that this patch should make
> us spec conformant I removed them.

Of course the spec won’t say anything about emulating the mouse when
triggering DOM events for the file control element.

But in retrospect it does not make sense to fire mouse emulation
events for a command that has “send keys” in the name…

> See my last comment for most of this comment. Further we could restore all the additional events for the legacy behavior.

I don’t think anyone relies on those events so it should be fine.

Dropping this issue since I bought your argument for the event
delegation.

> > Let files be the result of splitting text on the newline (\n) character. 
> > If files is of 0 length, return an error with error code invalid argument.
> 
> For some languages an empty string here means length of 0. See https://github.com/w3c/webdriver/pull/1261#issuecomment-393311826.

Right yes, but my point was whether it also applied for non-multiple
<input type=file>.  It looks like we split unconditionally of that,
so this is all fine.

> I don't see a benefit here given that it should be a test for `Element Clear`.

Maybe.

But if the number is wrong, that might give us an unrelated and
unexpected error elsewhere.  Isn’t it better to ensure we have the
right conditions set for the test upfront?
Comment on attachment 8982629 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys fire change event for <input type=file>.

https://reviewboard.mozilla.org/r/248594/#review257858

> Not sure what you mean here. `create_file` is a fixture and as such will automatically be imported by pytest.

I saw tests on try failing because create_file didn’t exist.  Maybe
it was something unrelated to this?
Comment on attachment 8982629 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys fire change event for <input type=file>.

https://reviewboard.mozilla.org/r/248594/#review257854

> Maybe `to ensure`? Or what else would you propose?

That works.  Or “test”.
See Also: → 1469601
Comment on attachment 8982626 [details]
Bug 1448792 - [wdspec] Add basic tests and user prompt tests for Element Send Keys.

https://reviewboard.mozilla.org/r/248588/#review257872

> Yes.  And that sounds like a good plan.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1469601.
Comment on attachment 8982626 [details]
Bug 1448792 - [wdspec] Add basic tests and user prompt tests for Element Send Keys.

https://reviewboard.mozilla.org/r/248588/#review257846

> Something wrong with the grammar here.

I don't see a problem here. What do you think is wrong? Should I write: `Add basic tests and user prompt tests...`?

> type is unused.

Ups. Just a `s/3/value` then.
Comment on attachment 8982627 [details]
Bug 1448792 - [wdspec] Create global fixture and assert methods for event checks.

https://reviewboard.mozilla.org/r/248590/#review257848

> Yes, assert_element_equal is probably better, but don’t feel compelled
> to change that as part of this changeset.

The `assert_element_equal` change has been done. Anything else we can do later.
Comment on attachment 8982626 [details]
Bug 1448792 - [wdspec] Add basic tests and user prompt tests for Element Send Keys.

https://reviewboard.mozilla.org/r/248588/#review257846

> I don't see a problem here. What do you think is wrong? Should I write: `Add basic tests and user prompt tests...`?

I think the “and” is the confusing part here.  To me it reads as if
“basic and user prompt tests” belongs together, whereas I think you
mean “basic tests” and “user prompt tests”.

I would propose either dropping “basic” and mention those in the
long-form commit message, or this to remove the confusion:

> Add user prompt tests and basic tests for Element Send Keys
Comment on attachment 8982628 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys wdspec conforming for file uploads.

https://reviewboard.mozilla.org/r/248592/#review257850

> > el.focus() is already fired in webdriverSendKeysToElement()
> > and legacySendKeysToElement() so it doesn't have to be done
> > twice. Also we don't call blur() for any other code path in
> > ElementSendKeys, so to be literaly conformant we have to remove it
> > here.
> > 
> > Thinking more about this and what you said earlier in that review,
> > uploadFile should only handle the file selection for the input. So
> > having the events being fired in both of the outer methods, sounds
> > also more appriate. I will move the input and change events.
> 
> Makes sense.
> 
> > Further nothing in the WebDriver spec actually mentions that
> > further events have to be fired. Also by reading the HTML spec
> > nothing refers to those events. Given that this patch should make
> > us spec conformant I removed them.
> 
> Of course the spec won’t say anything about emulating the mouse when
> triggering DOM events for the file control element.
> 
> But in retrospect it does not make sense to fire mouse emulation
> events for a command that has “send keys” in the name…

So what does that mean for my patch? Given that you dropped the other issue, we should drop this too?

> Maybe.
> 
> But if the number is wrong, that might give us an unrelated and
> unexpected error elsewhere.  Isn’t it better to ensure we have the
> right conditions set for the test upfront?

So I will add an assert for the file length after `clear` is called. Which is the missing part of this test.
Comment on attachment 8982629 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys fire change event for <input type=file>.

https://reviewboard.mozilla.org/r/248594/#review257858

> I saw tests on try failing because create_file didn’t exist.  Maybe
> it was something unrelated to this?

Yes it failed because `create_file` didn't return the inner function due to the bad rebase.
Comment on attachment 8982626 [details]
Bug 1448792 - [wdspec] Add basic tests and user prompt tests for Element Send Keys.

https://reviewboard.mozilla.org/r/248588/#review257846

> I think the “and” is the confusing part here.  To me it reads as if
> “basic and user prompt tests” belongs together, whereas I think you
> mean “basic tests” and “user prompt tests”.
> 
> I would propose either dropping “basic” and mention those in the
> long-form commit message, or this to remove the confusion:
> 
> > Add user prompt tests and basic tests for Element Send Keys

Ok, I will add the extra `test` here.
Comment on attachment 8982628 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys wdspec conforming for file uploads.

https://reviewboard.mozilla.org/r/248592/#review257850

> So what does that mean for my patch? Given that you dropped the other issue, we should drop this too?

Sorry, I forgot to drop it.

> So I will add an assert for the file length after `clear` is called. Which is the missing part of this test.

That works for me.
Comment on attachment 8982628 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys wdspec conforming for file uploads.

https://reviewboard.mozilla.org/r/248592/#review258020

This looks good to go from a technical perspective.

I wouldn’t object to landing this changeset right now, but I
understand you have concerns with landing it before there is
commitment and agreement between vendors.

I think this change is so esoteric and marginal that I don’t think
it will significantly affect interoperability if it were to land,
however.

::: testing/marionette/interaction.js:469
(Diff revision 3)
> - * Appends <var>path</var> to an <tt>&lt;input type=file&gt;</tt>'s
> + * Appends <var>text</var> to an <tt>&lt;input type=file&gt;</tt>'s
>   * file list.
>   *
>   * @param {HTMLInputElement} el
>   *     An <tt>&lt;input type=file&gt;</tt> element.

Do you mind RST‘ifying the rest of this as well?
Attachment #8982628 - Flags: review?(ato) → review+
Comment on attachment 8982628 [details]
Bug 1448792 - [marionette][wdspec] Make Element Send Keys wdspec conforming for file uploads.

https://reviewboard.mozilla.org/r/248592/#review258020

> Do you mind RST‘ifying the rest of this as well?

Will be done in the next commit.
Latest try build shows a test failure in element_clear/clear.py:

> test_resettable_element_focus_when_empty - assert None == []

This is happening because we moved the initialization of `window.events` into the event tracker callback. Instead it's better to move it before registering all the events, so it always returns a list. I will update the affected commit, and push another try for Linux only.
speaking to :whimboo about landing this(a conversation with ato) and doing a follow up bug about changes potentially needed to the spec. THis would mean †he followup bug would be more focused and we can land this bug straight away.

I think this is a sensible plan and we should do it.
Flags: needinfo?(dburns)
Great. Going to land it now.
Whiteboard: [blocked by spec]
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/083b06a7b31e
[wdclient] Fix Element.clear() for unknown "url" property. r=ato
https://hg.mozilla.org/integration/autoland/rev/604eed55c12a
[wdspec] Add basic tests and user prompt tests for Element Send Keys. r=ato
https://hg.mozilla.org/integration/autoland/rev/793b358c6ae0
[wdspec] Create global fixture and assert methods for event checks. r=ato
https://hg.mozilla.org/integration/autoland/rev/bd820cb9ca2d
[marionette][wdspec] Make Element Send Keys wdspec conforming for file uploads. r=ato
https://hg.mozilla.org/integration/autoland/rev/c26eb7edb08f
[marionette][wdspec] Make Element Send Keys fire change event for <input type=file>. r=ato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11968 for changes under testing/web-platform/tests
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: