Closed Bug 1424156 Opened 2 years ago Closed 2 years ago

[a11y] Console input from the user should not be treated as "live" content

Categories

(DevTools :: Console, defect)

defect
Not set

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: Jamie, Assigned: Jamie)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(4 files)

STR (with the NVDA screen reader):
1. Start NVDA and Firefox.
2. Open the Web Console with control+shift+k.
3. Type "foobar" and press enter.
Expected: After pressing enter, NVDA should just report the ReferenceError.
Actual: After pressing enter, NVDA reports "log  foobar  error  ReferenceError..."

Any console messages which are input from the user should get the attribute aria-live="off". I did this as part of bug 1057127, but it seems that code is now compat code and doesn't apply. See around line 507 of devtools/client/webconsole/console-output.js:

    if (this.category == "input") {
      // Assistive technology tools shouldn't echo input to the user,
      // as the user knows what they've just typed.
      container.setAttribute("aria-live", "off");
    }

I'd be happy to take a stab at this, but I'm a little confused by the compat code at this stage; I'm not quite sure what code actually gets used in practice. I'm also not sure where the categories are defined now; there's this CATEGORY_INPUT constant in webconsole.js, but that doesn't seem to get used by console-output.js.

In addition, the spans with class "icon" should also get aria-live="off". I assume that's easy enough to do around line 867:

    let icon = this.document.createElementNS(XHTML_NS, "span");
    icon.className = "icon";
    icon.title = l10n.getStr("severity." + this._severityNameCompat);
Keywords: access
Hey James,

The code you are pointed to is not the one used in the console now. We refactored it to a React component and what's currently used is the code from devtools/client/webconsole/new-console-output . From there, I guess we should modifiy the Message component, either by checking the `type` props passed in https://searchfox.org/mozilla-central/rev/3ec05888ca32b2d8a14d700474efb0c63411fca2/devtools/client/webconsole/new-console-output/components/Message.js#120 , and could be done by adding :

```
"aria-live": type === MESSAGE_TYPE.COMMAND ? "off" : "on"
```

to https://searchfox.org/mozilla-central/rev/3ec05888ca32b2d8a14d700474efb0c63411fca2/devtools/client/webconsole/new-console-output/components/Message.js#244-248

MESSAGE_TYPE comes from the devtools/client/webconsole/new-console-output/constants.js


For the icon, it is handled by the MessageIcon component in devtools/client/webconsole/new-console-output/components/MessageIcon.js , and the `aria-live: "off"` part could be added in https://searchfox.org/mozilla-central/rev/3ec05888ca32b2d8a14d700474efb0c63411fca2/devtools/client/webconsole/new-console-output/components/MessageIcon.js#24-27,
Assignee: nobody → jteh
Comment on attachment 8939437 [details]
Bug 1424156 part 1: Don't treat Web Console input from the user as live content for accessibility.

https://reviewboard.mozilla.org/r/209756/#review215338

Looks good to me, thanks !
Attachment #8939437 - Flags: review?(nchevobbe) → review+
Comment on attachment 8939438 [details]
Bug 1424156 part 2: Don't treat message icons in the Web Console as live content for accessibility.

https://reviewboard.mozilla.org/r/209758/#review215340

Thanks James, this is good to land.
We should also add some tests on those components to make sure they work as intended and we don't regress them.
We can have either a mochitest, or a mocha test.

Mocha tests are faster and easier to right, with the drawback of not being run on TRY (we - the team - do it manually almost every day, so we know when something is broken). A mocha test for the icon could be added after https://searchfox.org/mozilla-central/rev/51cd1093c5c2113e7b041e8cc5c9bf2186abda13/devtools/client/webconsole/new-console-output/test/components/message-icon.test.js#17-22

You can run the mocha tests with  `cd devtools/client/webconsole/ && yarn && yarn test`
You might need to do `cd devtools/client/netmonitor/ && yarn` if the previous command gave you an error though.
Attachment #8939438 - Flags: review?(nchevobbe) → review+
I added a Mocha test for the message icon as suggested. I spent some time trying to figure out how to add tests for the message to check whether aria-live is set correctly based on the type. However, this is a bit tricky because constructing a Message directly requires passing a lot of parameters. I get the impression I'm supposed to do this with stubs... but there is no stub generator for ConsoleCommand. What I want to do is check that an error message gets aria-live off but a console command gets aria-live on.
Comment on attachment 8939733 [details]
Bug 1424156 part 3: Add test for aria-live in Web Console message icon.

https://reviewboard.mozilla.org/r/210046/#review215846

This looks good to me, thanks !

I think we could test the aria-live attribute on the different message type components by adding a new test file like : 

```diff
diff --git a/devtools/client/webconsole/new-console-output/test/components/message-types-aria.test.js b/devtools/client/webconsole/new-console-output/test/components/message-types-aria.test.js
new file mode 100644
--- /dev/null
+++ b/devtools/client/webconsole/new-console-output/test/components/message-types-aria.test.js
@@ -0,0 +1,39 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+"use strict";
+
+// Test utils.
+const expect = require("expect");
+
+const {
+  renderComponent
+} = require("devtools/client/webconsole/new-console-output/test/helpers");
+
+// Components under test.
+const ConsoleApiCall = require("devtools/client/webconsole/new-console-output/components/message-types/ConsoleApiCall");
+const ConsoleCmd = require("devtools/client/webconsole/new-console-output/components/message-types/ConsoleCommand");
+const { ConsoleCommand } = require("devtools/client/webconsole/new-console-output/types");
+
+// Test fakes.
+const { stubPreparedMessages } = require("devtools/client/webconsole/new-console-output/test/fixtures/stubs/index");
+const serviceContainer = require("devtools/client/webconsole/new-console-output/test/fixtures/serviceContainer");
+
+describe.only("message types component aria", () => {
+  describe("ConsoleAPICall", () => {
+    it("sets aria-live to polite", () => {
+      const message = stubPreparedMessages.get("console.log('foobar', 'test')");
+      const wrapper = renderComponent(ConsoleApiCall, { message, serviceContainer });
+      expect(wrapper.getAttribute("aria-live")).toBe("polite");
+    });
+  });
+
+  describe("ConsoleCommand", () => {
+    it("sets aria-live to off", () => {
+      let message = new ConsoleCommand({
+        messageText: `"simple"`,
+      });
+      const wrapper = renderComponent(ConsoleCmd, { message, serviceContainer });
+      expect(wrapper.getAttribute("aria-live")).toBe("off");
+    });
+  });
+});

```

Here it only tests ConsoleApiCall and ConsoleCommand, but the other types can be tested as well (and we do have stubs for them).

I agree the ConsoleCommand is a bit tricky. We can't really copy what we do for the other types since the server isn't involved in this case. 
We directly build a ConsoleCommand message type (different than the ConsoleCommand component), and dispatch it : https://searchfox.org/mozilla-central/rev/b24e6342d744c5a83fab5c15972e11eeb69d68e6/devtools/client/webconsole/jsterm.js#463-467

So here in my WIP diff, I just copied the same code.

If you find this too brittle, we can add a mochitest and look at the generated dom to check the attribute. I don't think it would be hard to do.
Attachment #8939733 - Flags: review?(nchevobbe) → review+
Thanks. I didn't spot that ConsoleCommand function in types.js. That does indeed make constructing this easier.

I took your code and added a test for one other message type. I didn't see the need to add tests for all message types, but having at least one more helps ensure that aria-live="off" is the exceptional case.

Thanks so much for all your help on this. I feel like you did 90% of the work and I just threw it into patches :)... but I doubt I would have been able to figure this all out myself in a reasonable time.
Comment on attachment 8940066 [details]
Bug 1424156 part 4: Add test for aria-live in Web Console messages.

https://reviewboard.mozilla.org/r/210356/#review216110

I only have a few nits, but this looks good and should be ready to land with a green TRY.
Thanks for filing the issue and working on it :)

::: devtools/client/webconsole/new-console-output/test/components/message-types-aria.test.js:15
(Diff revision 1)
> +const { ConsoleCommand } = require("devtools/client/webconsole/new-console-output/types");
> +const EvaluationResult = require("devtools/client/webconsole/new-console-output/components/message-types/EvaluationResult");

nit: could we switch these 2 lines so we have all the components loaded, then the type ?

::: devtools/client/webconsole/new-console-output/test/components/message-types-aria.test.js:26
(Diff revision 1)
> +      const wrapper = renderComponent(ConsoleApiCall,
> +        { message, serviceContainer });

I think it is okay if we make this a single line

::: devtools/client/webconsole/new-console-output/test/components/message-types-aria.test.js:35
(Diff revision 1)
> +      const wrapper = renderComponent(EvaluationResult,
> +        { message, serviceContainer });

same thing, we can make this a single line
Attachment #8940066 - Flags: review?(nchevobbe) → review+
Comment on attachment 8940066 [details]
Bug 1424156 part 4: Add test for aria-live in Web Console messages.

https://reviewboard.mozilla.org/r/210356/#review216110

> I think it is okay if we make this a single line

I've changed this now, but I originally split it because the Mozilla style guide says 80 chars per line and this is quite a bit longer. Is there a different style guide for devtools?
We override some rules indeed. The max line length is 90 (see https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/devtools/.eslintrc.js#154-158).

Thanks for your patch and insights.
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9484bc066246
part 1: Don't treat Web Console input from the user as live content for accessibility. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/a13e4aa50688
part 2: Don't treat message icons in the Web Console as live content for accessibility. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/0ba823f7ada8
part 3: Add test for aria-live in Web Console message icon. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/2a26976b3f16
part 4: Add test for aria-live in Web Console messages. r=nchevobbe
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.