Closed Bug 900097 Opened 11 years ago Closed 11 years ago

statusbarpanel shouldn't be a button accessible

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox24 + fixed
firefox25 + fixed
firefox26 --- fixed

People

(Reporter: surkov, Assigned: maxli)

References

Details

(Keywords: access)

Attachments

(2 files, 1 obsolete file)

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.
surkov, is this a blocker for resolution of bug 888981? If so, please set the tracking-firefox24 flag to ?
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(surkov.alexander)
(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.
OS: Mac OS X → All
Hardware: x86 → All
Enn, can you advise us how to proceed here?
Flags: needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin)
(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.
(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
Assignee: nobody → maxli
Attached patch Patch (obsolete) — Splinter Review
Attachment #787412 - Flags: review?(enndeakin)
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.)
Wouldn't it be nicer to back out bug 419867 instead? Should statusbarpanel binding have display="xul:button" attribute?
(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 on attachment 787412 [details] [diff] [review]
Patch

r- for last comment please have bindings always return the same value.
Attachment #787412 - Flags: review?(enndeakin) → review+
(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 on attachment 787412 [details] [diff] [review]
Patch

The comment suggests you wanted to mark this -.
Attachment #787412 - Flags: review+ → review-
(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 :/
Attached patch Patch v2Splinter Review
Attachment #787412 - Attachment is obsolete: true
Attachment #787806 - Flags: review?(enndeakin)
Also, it'd be nice to have some a11y tests (role/test_general.xul looks like a good place for this)
Attachment #787806 - Flags: review?(enndeakin) → review+
Neil, what about display="xul:button"?
It's ignore on the base statusbarpanel so you could remove it if you wanted, but it is used on the others.
Attached patch tests patchSplinter Review
Attachment #787862 - Flags: review?(surkov.alexander)
https://hg.mozilla.org/mozilla-central/rev/ce71b5adcb79
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Summary: searchbarpanel shouldn't be a button accessible → statusbarpanel shouldn't be a button accessible
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
Attachment #787806 - Flags: approval-mozilla-beta?
Attachment #787806 - Flags: approval-mozilla-aurora?
Attachment #787806 - Flags: approval-mozilla-beta?
Attachment #787806 - Flags: approval-mozilla-beta+
Attachment #787806 - Flags: approval-mozilla-aurora?
Attachment #787806 - Flags: approval-mozilla-aurora+
Comment on attachment 787862 [details] [diff] [review]
tests patch

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

r=me, thank you
Attachment #787862 - Flags: review?(surkov.alexander) → review+
You need to log in before you can comment on or make changes to this bug.