1490840 - HyperTextAccessible::ReplaceText causes two distinct DOM 'input' events

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Eitan Isaacson [:eeejay]]

When calling ReplaceText() with a string on an input element, we get two DOM events. The first is a delete action. The second is an insert action. It really should only have a single event with the new value.

I tried solving this with TextEditor::ReplaceTextAsAction, but it caused some regressions with editable divs. Something to do with selection ranges.

So I went with a less disruptive patch that deletes the text if the replacement is an empty string. Otherwise, it selects the whole range and does an insert which will overwrite the range.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Only delete text in ReplaceTextAsAction if the string is empty. r?Jamie

Comment 2

[James Teh [:Jamie]]

Comment on attachment 9008561 [details] [diff] [review]
Only delete text in ReplaceTextAsAction if the string is empty. r?Jamie

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

nit: In the commit message, you wrote ReplaceTextAsAction. I'm guessing this should be HyperTextAccessible::ReplaceText.

::: accessible/tests/browser/text/browser_test_settextcontents.js
@@ +1,1 @@
> +function waitForDOMInputEvent() {

nit: Missing license header.

@@ +3,5 @@
> +    content.document.getElementById("input").addEventListener("input", evt => {
> +      resolve(evt.target.value);
> +    }, { once: true });
> +  });
> +}

Any reason not to use BrowserTestUtils.waitForEvent here?

@@ +10,5 @@
> +  let input = findAccessibleChildByID(accDoc, "input", [nsIAccessibleEditableText]);
> +  let contentListener = ContentTask.spawn(browser, {}, waitForDOMInputEvent);
> +  input.setTextContents("goodbye");
> +  let newValue = await contentListener;
> +  is(newValue, "goodbye", "input set to new value in first event.");

I feel I'm missing some context here. How does the DOM input event affect a11y? I understand it fires two events, but why does that matter to us? And if it does affect a11y events in a way that matters to you, should we not be testing for a11y events rather than DOM events?

@@ +14,5 @@
> +  is(newValue, "goodbye", "input set to new value in first event.");
> +}
> +
> +/**
> + * Test caching of accessible object states

I'm guessing this comment needs to be updated?

Comment 3

[Eitan Isaacson [:eeejay]]

(In reply to James Teh [:Jamie] from comment #2)
> Comment on attachment 9008561 [details] [diff] [review]
> Only delete text in ReplaceTextAsAction if the string is empty. r?Jamie
> 
> Review of attachment 9008561 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> nit: In the commit message, you wrote ReplaceTextAsAction. I'm guessing this
> should be HyperTextAccessible::ReplaceText.
> 
> ::: accessible/tests/browser/text/browser_test_settextcontents.js
> @@ +1,1 @@
> > +function waitForDOMInputEvent() {
> 
> nit: Missing license header.
> 
> @@ +3,5 @@
> > +    content.document.getElementById("input").addEventListener("input", evt => {
> > +      resolve(evt.target.value);
> > +    }, { once: true });
> > +  });
> > +}
> 
> Any reason not to use BrowserTestUtils.waitForEvent here?

ContentTaskUtils, good idea.

> 
> @@ +10,5 @@
> > +  let input = findAccessibleChildByID(accDoc, "input", [nsIAccessibleEditableText]);
> > +  let contentListener = ContentTask.spawn(browser, {}, waitForDOMInputEvent);
> > +  input.setTextContents("goodbye");
> > +  let newValue = await contentListener;
> > +  is(newValue, "goodbye", "input set to new value in first event.");
> 
> I feel I'm missing some context here. How does the DOM input event affect
> a11y? I understand it fires two events, but why does that matter to us? And
> if it does affect a11y events in a way that matters to you, should we not be
> testing for a11y events rather than DOM events?

It doesn't affect a11y events in a bad way. At least not that I know.

A web app would expect a single input event after the user pastes a value into a field, not a deletion and then insertion. So this patch aligns our API to work more similarly to how a non-AT user would interact with a page in that case.

I stumbled on this when trying to migrate our Android auto-fill JS code to the a11y API. The test expects one "input" event after the input's DOM node gets assigned a new value. If it is a pair of events (delete-insert) there is no certain way of knowing that an expected delete happened (you would need to wait for an arbitrary period to know no followup inserts came in).

So yeah, I admit this patch is more for reliable tests and less for a user case.

> 
> @@ +14,5 @@
> > +  is(newValue, "goodbye", "input set to new value in first event.");
> > +}
> > +
> > +/**
> > + * Test caching of accessible object states
> 
> I'm guessing this comment needs to be updated?

Oops.

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: Only delete text in ReplaceText if the string is empty. r?Jamie

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Only delete text in ReplaceText if the string is empty.

Made it a mochitest.

Comment 6

[Eitan Isaacson [:eeejay]]

Attachment: Only delete text in ReplaceText if the string is empty. r?Jamie

Oops. ignore that last one. Forgot to stage files.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4923d4f4dc0
Only delete text in ReplaceText if the string is empty. r=Jamie

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c4923d4f4dc0