900097 - statusbarpanel shouldn't be a button accessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

statusbarpanel is exposed as a button accessible (see bug 419867), now it doesn't seem anything in common with buttons (neither visually neither by usage (for example error console uses it as a container http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/content/console.xul#95).

Besides confusing accessibility (like button containing a textfield) it's caused bug 888981.

MSD claims that status bar was removed (https://developer.mozilla.org/en-US/docs/XUL/statusbarpanel) so perhaps all status bar related stuff are just a rudiment.

Comment 1

[Alex Keybl [:akeybl]]

surkov, is this a blocker for resolution of bug 888981? If so, please set the tracking-firefox24 flag to ?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Alex Keybl [:akeybl] from comment #1)
> surkov, is this a blocker for resolution of bug 888981? If so, please set
> the tracking-firefox24 flag to ?

yes, it should be an easy (and less risky) fix for that bug.

Comment 3

[alexander :surkov (:asurkov)]

Enn, can you advise us how to proceed here?

Comment 4

[Neil Deakin]

statusbarpanel was intended to be a type of button, but an xbl issue (bug 119389) means that it only becomes a button when either class="statusbarpanel-iconic" or class="statusbarpanel-iconic-text" is used.

For compatibility I'd suggest making it only use a button accessible in those cases.

Comment 5

[bhavana bajaj [:bajaj]]

(In reply to alexander :surkov from comment #3)
> Enn, can you advise us how to proceed here?

Tracking this back, as I think this was not intentional.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to bhavana bajaj [:bajaj] from comment #5)
> (In reply to alexander :surkov from comment #3)
> > Enn, can you advise us how to proceed here?
> 
> Tracking this back, as I think this was not intentional.

right, thank you, presumably didn't hit f5 before sending the comment, due to some reason bugzilla didn't alert about collision

Comment 7

[Max Li [:maxli]]

Attachment: Patch

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 787412 [details] [diff] [review]
Patch

>+            let classAttr = this.getAttribute('class');
>+            return classAttr === 'statusbarpanel-iconic' ||
>+                   classAttr === 'statusbarpanel-iconic-text' ?

Use this.classList.contains. (Also, use double quotes.)

Comment 9

[alexander :surkov (:asurkov)]

Wouldn't it be nicer to back out bug 419867 instead? Should statusbarpanel binding have display="xul:button" attribute?

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #9)
> Wouldn't it be nicer to back out bug 419867 instead? Should statusbarpanel
> binding have display="xul:button" attribute?

you'd need to add a nsIAccessibleProvider to the statusbarpanel-iconic-text binding as well, but please do this it'll make bug 846185 easier.

Comment 11

[Trevor Saunders (:tbsaunde)]

Comment on attachment 787412 [details] [diff] [review]
Patch

r- for last comment please have bindings always return the same value.

Comment 12

[Neil Deakin]

(In reply to alexander :surkov from comment #9)
> Wouldn't it be nicer to back out bug 419867 instead? Should statusbarpanel
> binding have display="xul:button" attribute?

Yes, you should do this instead. The two bindings that are button-type should use button accessibleTypes.

Comment 13

[Neil Deakin]

Comment on attachment 787412 [details] [diff] [review]
Patch

The comment suggests you wanted to mark this -.

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to Neil Deakin from comment #13)
> Comment on attachment 787412 [details] [diff] [review]
> Patch
> 
> The comment suggests you wanted to mark this -.

hah, yes :/

Comment 15

[Max Li [:maxli]]

Attachment: Patch v2

Comment 16

[alexander :surkov (:asurkov)]

Also, it'd be nice to have some a11y tests (role/test_general.xul looks like a good place for this)

Comment 17

[alexander :surkov (:asurkov)]

Neil, what about display="xul:button"?

Comment 18

[Neil Deakin]

It's ignore on the base statusbarpanel so you could remove it if you wanted, but it is used on the others.

Comment 19

[Max Li [:maxli]]

Attachment: tests patch

Comment 20

[Max Li [:maxli]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce71b5adcb79

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Max Li [:maxli]]

Comment on attachment 787806 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 722265
User impact if declined: Accessibility users will experience browser hangs when using certain addons like DownThemAll or by opening the error console.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): None known.
String or IDL/UUID changes made by this patch: None

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7c668796e2d2
https://hg.mozilla.org/releases/mozilla-beta/rev/3b873fda98fc

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 787862 [details] [diff] [review]
tests patch

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

r=me, thank you

Comment 25

[Phil Ringnalda (:philor)]

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