1336937 - Add aria-pressed to the filter buttons

Status: VERIFIED FIXED

(DevTools :: Console, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

It's a regression due to react refactoring. This issue raised on both webconsole and netmonitor (bug 1335322). 

We should ensure to set aria-pressed="true" if filter button is toggled and set aria-pressed="false" vise versa. Plus, it would be great if we can add a test in case of regression.

Nicolas, would you like to take a look at this? Thanks:)

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Sure, thanks for reporting

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1336937 - Add aria-pressed to the filter buttons in new console.

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

Comment 3

[Julian Descottes [:jdescottes]]

Comment on attachment 8833955 [details]
Bug 1336937 - Add aria-pressed to the filter buttons in new console.

https://reviewboard.mozilla.org/r/110064/#review111076

Looks good thanks! 
(my comment below is more a question than a request for change!)

::: devtools/client/webconsole/new-console-output/components/filter-button.js:40
(Diff revision 1)
>      if (active) {
>        classList.push("checked");
>      }
>  
>      return dom.button({
> +      "aria-pressed": (active === true).toString(),

I don't think toString() is mandatory here?

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

> I don't think toString() is mandatory here?

I thought it was, when I didn't put it, the attribute wasn't appended in the node.
But doing a quick test https://jsfiddle.net/n4zthvxw/1/ seems to indicate that it should work 

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

My comment was cut-off (thanks bugzilla emoji's support :) ).

> I thought it was, when I didn't put it, the attribute wasn't appended in the node.
> But doing a quick test https://jsfiddle.net/n4zthvxw/1/ seems to indicate that it should work 

I'll double-check later to see if I did something wrong the first time.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8833955 [details]
Bug 1336937 - Add aria-pressed to the filter buttons in new console.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56b0d9ecb97b
Add aria-pressed to the filter buttons in new console. r=jdescottes

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56b0d9ecb97b

Comment 9

[Timea Zsoldos [:tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 53.0a2 (ID=20170206004003) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 54.0a1 on Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.11.