Closed
Bug 1424156
Opened 7 years ago
Closed 7 years ago
[a11y] Console input from the user should not be treated as "live" content
Categories
(DevTools :: Console, defect)
DevTools
Console
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);
Updated•7 years ago
|
Blocks: devtoolsa11y
Comment 1•7 years ago
|
||
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,
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jteh
Comment 4•7 years ago
|
||
mozreview-review |
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 5•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
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 8•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
mozreview-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 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+
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
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?
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
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.
Comment 15•7 years ago
|
||
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
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9484bc066246
https://hg.mozilla.org/mozilla-central/rev/a13e4aa50688
https://hg.mozilla.org/mozilla-central/rev/0ba823f7ada8
https://hg.mozilla.org/mozilla-central/rev/2a26976b3f16
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•