1430575 - Unable clear <input type=number> after typing invalid data into it

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Alexei Barantsev]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.108 Safari/537.36

Steps to reproduce:

Detected in Firefox Nightly 59.0a1 (2018-01-14) (64-bit)

Regression issue, may be caused by https://bugzilla.mozilla.org/show_bug.cgi?id=1354211

driver.get("https://output.jsbin.com/serilo");
WebElement numInput = driver.findElement(By.id("num"));
numInput.sendKeys("test");
numInput.clear();
numInput.sendKeys("33");
assertEquals(numInput.getAttribute("value"), "33");


Actual results:

getAttribute returns empty value, but visually the field contains text "test33"


Expected results:

getAttribute should return value "33"

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Andreas, can you please have a look?

Comment 2

[Andreas Tolfsen ❲:ato❳]

This appears to be a bug in the WebDriver standard.  We abort
modifying the element if it is empty, which for an <input> is if its
value property is equal to "" (empty string).

The complication is that whilst it is allowed to enter invalid
characters to an <input type=number> in Gecko, it is not in Chrome.
This means this particular WebDriver bug will only occur in Gecko
because the value property isn’t updated when the input is
invalid.

As far as I know there is no way with current web platform APIs to
access the current (invalid) input of <input type=number>.  But we
can additionaly make a check of its ValidityState [1], since the
value === "" check is not sufficient.

I filed [2] against the WebDriver specification.

Regarding the above test case, you should be getting the input’s
_property_ rather than the value, but I corrected this in my own
test case.

  [1] https://html.spec.whatwg.org/#validitystate

Comment 3

[David Burns :automatedtester]

[Mass update] Setting P1 as currently being worked on.

Comment 4

[Alexei Barantsev]

I want to make a note that it works in the current Release (57) and used to work in 59 before the mentioned change https://bugzilla.mozilla.org/show_bug.cgi?id=1354211. So I suppose it is technically possible to implement "old" behaviour. But, yes, it should be coherrent with the specification.

Comment 5

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1430575 - Adjust editable definition to match WebDriver.

Introduces a new function, isSubmittable, to the element module in
Marionette that tests if an element is a submittable form control.

Review commit: https://reviewboard.mozilla.org/r/212954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212954/

Comment 6

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1430575 - Document interaction.clearElement.

Review commit: https://reviewboard.mozilla.org/r/212956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212956/

Comment 7

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1430575 - Add element.satisfiesContraints.

Adds new shorthand function, satisfiesConstraints, to the Marionette
element module, that tests if the submittable form control satisfies
its constraints, as it is described in the HTML standard.

Review commit: https://reviewboard.mozilla.org/r/212958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212958/

Comment 8

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1430575 - Add validity state check for WebDriver:ElementClear.

This patch checks that the element satisfies its form control
constraints, as well as being empty, before deciding not to clear
the element.  This will make it possible to clear elements that
have invalid input.

The "clear a resettable element" algorithm is missing a check of
the <input> element's ValidityState.  WebDriver:ElementClear has
a subtle bug that only manifests in Gecko because Blink rejects
invalid key input to validation fields such as <input type=number>,
but Gecko does not.

The value property of <input type=number> will not be updated unless
the input is actually valid, which means the first step of the algorithm
will pass irregardless of whether the user has actually modified it.

Review commit: https://reviewboard.mozilla.org/r/212960/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212960/

Comment 9

[Andreas Tolfsen ❲:ato❳]

try because try-from-mozreview is broken: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4e3e746960f0d2a3e65f3f076afaa412b7ad810

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8942693 [details]
Bug 1430575 - Document interaction.clearElement.

Review commit: https://reviewboard.mozilla.org/r/212956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212956/

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8942694 [details]
Bug 1430575 - Add element.satisfiesContraints.

Adds new shorthand function, satisfiesConstraints, to the Marionette
element module, that tests if the submittable form control satisfies
its constraints, as it is described in the HTML standard.

Review commit: https://reviewboard.mozilla.org/r/212958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212958/

Comment 12

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8942695 [details]
Bug 1430575 - Add validity state check for WebDriver:ElementClear.

This patch checks that the element satisfies its form control
constraints, as well as being empty, before deciding not to clear
the element.  This will make it possible to clear elements that
have invalid input.

The "clear a resettable element" algorithm is missing a check of
the <input> element's ValidityState.  WebDriver:ElementClear has
a subtle bug that only manifests in Gecko because Blink rejects
invalid key input to validation fields such as <input type=number>,
but Gecko does not.

The value property of <input type=number> will not be updated unless
the input is actually valid, which means the first step of the algorithm
will pass irregardless of whether the user has actually modified it.

Review commit: https://reviewboard.mozilla.org/r/212960/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212960/

Comment 13

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8942692 [details]
Bug 1430575 - Adjust editable definition to match WebDriver.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212954/diff/1-2/

Comment 14

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8942693 [details]
Bug 1430575 - Document interaction.clearElement.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212956/diff/1-2/

Comment 15

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8942694 [details]
Bug 1430575 - Add element.satisfiesContraints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212958/diff/1-2/

Comment 16

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8942695 [details]
Bug 1430575 - Add validity state check for WebDriver:ElementClear.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212960/diff/1-2/

Comment 17

[David Burns :automatedtester]

Comment on attachment 8942692 [details]
Bug 1430575 - Adjust editable definition to match WebDriver.

https://reviewboard.mozilla.org/r/212954/#review218654

Comment 18

[David Burns :automatedtester]

Comment on attachment 8942693 [details]
Bug 1430575 - Document interaction.clearElement.

https://reviewboard.mozilla.org/r/212956/#review218656

::: testing/marionette/interaction.js:295
(Diff revision 2)
>  };
>  
> +/**
> + * Clears the form control or the editable element, if required.
> + *
> + * Before attempting to clear the element, it will be attempted

s/attempted scrolled into view/attempt to scroll into view

::: testing/marionette/interaction.js:300
(Diff revision 2)
> + * Before attempting to clear the element, it will be attempted
> + * scrolled into view if it is not already in the viewport.
> + *
> + * If the element is a submittable form control and it is empty
> + * (it has no value or it has no files associated with it, in the
> + * case it is a <code>&lt;input type=file>&gt; element) or it is an

closing `</code>` is missing which could explain the `>` next to `type=file`

Comment 19

[David Burns :automatedtester]

Comment on attachment 8942694 [details]
Bug 1430575 - Add element.satisfiesContraints.

https://reviewboard.mozilla.org/r/212958/#review218664

::: testing/marionette/element.js:964
(Diff revision 2)
> + *     True if it satisfies all its constraints.
> + *
> + * @throws {InvalidElementStateError}
> + *     If <var>el</var> is not a mutable form control element.
> + */
> +element.satisfiesConstraints = function(el) {

I don't like this this method name as it looks like it should be used as `element.satisiesConstraints(el, constraint);`

As the method is so small why can't this just be in method where you call `element.satisfiesConstraints(el);` in your other patch

Comment 20

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8942694 [details]
Bug 1430575 - Add element.satisfiesContraints.

https://reviewboard.mozilla.org/r/212958/#review218664

> I don't like this this method name as it looks like it should be used as `element.satisiesConstraints(el, constraint);`
> 
> As the method is so small why can't this just be in method where you call `element.satisfiesConstraints(el);` in your other patch

I agree.  Merging it with clearResettableElement in
testing/marionette/interaction.js makes the code flow much nicer.

Comment 21

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8942692 [details]
Bug 1430575 - Adjust editable definition to match WebDriver.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212954/diff/2-3/

Comment 22

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8942693 [details]
Bug 1430575 - Document interaction.clearElement.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212956/diff/2-3/

Comment 23

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8942695 [details]
Bug 1430575 - Add validity state check for WebDriver:ElementClear.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212960/diff/2-3/

Comment 24

[David Burns :automatedtester]

Comment on attachment 8942695 [details]
Bug 1430575 - Add validity state check for WebDriver:ElementClear.

https://reviewboard.mozilla.org/r/212960/#review219316

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f1762f3233a
Adjust editable definition to match WebDriver. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/7220cf5de9a8
Document interaction.clearElement. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/949cf88764e6
Add validity state check for WebDriver:ElementClear. r=automatedtester

Comment 26

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/2f1762f3233a
https://hg.mozilla.org/mozilla-central/rev/7220cf5de9a8
https://hg.mozilla.org/mozilla-central/rev/949cf88764e6