Closed
Bug 509696
Opened 15 years ago
Closed 15 years ago
Empty role on body removes read-only state; breaks virtual buffers
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
People
(Reporter: MarcoZ, Assigned: davidb)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(1 file, 5 obsolete files)
5.89 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
The result is that for NVDA at least, no virtual buffer is created. The construct is <body role="">...</body> This causes a DocAccessible to be created which has the nsIAccessibleStates::STATE_READONLY not set, causing NVDA's virtual buffer logic not to create one. This is obviously error in markup, but we should handle it gracefully nonetheless. Jamie found this in the Ext3JS recently released.
Assignee | ||
Comment 2•15 years ago
|
||
This might be the way to go. We could alternatively check HasDefinedARIAToken but that seems like overkill. The less error checking we do, the easier it will be for other browsers to match our behavior. The author is really being naughty here. No test yet.
Assignee | ||
Comment 3•15 years ago
|
||
Probably a cleaner approach (less spaghetti). If you guys agree we can push this and I'll make sure the aria impl guide gets updated.
Attachment #393759 -
Attachment is obsolete: true
Attachment #394342 -
Flags: review?(surkov.alexander)
Attachment #394342 -
Flags: review?(marco.zehe)
Reporter | ||
Comment 4•15 years ago
|
||
Comment on attachment 394342 [details] [diff] [review] patch1 Yes, this is a much better approach. :) Also like the test part. Thanks!
Attachment #394342 -
Flags: review?(marco.zehe) → review+
Comment 5•15 years ago
|
||
I'm not sure every accessible with not recognized role should have readonly state because readonly state is property of control elements and document. Probably more spagetti approach is more right way :)
Assignee | ||
Comment 6•15 years ago
|
||
(In reply to comment #4) Thanks Marco. (In reply to comment #5) I can go either way on this one. It *might* actually be harder to discover bugs/issues with the 'better' approach, so I might agree with you. Note that our test suite passes with both approaches.
Assignee | ||
Updated•15 years ago
|
Attachment #394342 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 7•15 years ago
|
||
I reworked the wip and added the tests.
Attachment #394342 -
Attachment is obsolete: true
Attachment #395091 -
Flags: review?(surkov.alexander)
Comment 8•15 years ago
|
||
does the spec treat empty role attribute and missed role attribute differently?
Assignee | ||
Comment 9•15 years ago
|
||
Comment on attachment 395091 [details] [diff] [review] patch2 Ignore patch2 it has cruft. Regarding role absence and role="", I think we can lead the way here. My current thinking is that we can treat both cases the same.
Attachment #395091 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 10•15 years ago
|
||
I propose we go ahead and fix; with follow up to the user agent group.
Attachment #395091 -
Attachment is obsolete: true
Attachment #395583 -
Flags: review?(surkov.alexander)
Comment 11•15 years ago
|
||
(In reply to comment #9) > Regarding role absence and role="", I think we can lead the way here. My > current thinking is that we can treat both cases the same. This was my original thinking also.
Assignee | ||
Comment 12•15 years ago
|
||
So far consensus looks probable: http://lists.w3.org/Archives/Public/wai-xtech/2009Aug/0190.html
Assignee | ||
Comment 13•15 years ago
|
||
From the thread above, it sounds like our rule might be: If the role attribute is present, but we have no mapping, then don't remove the readonly state bit. I think I'll try this approach.
Assignee | ||
Comment 14•15 years ago
|
||
Attachment #395583 -
Attachment is obsolete: true
Attachment #396526 -
Flags: review?(surkov.alexander)
Attachment #395583 -
Flags: review?(surkov.alexander)
Assignee | ||
Updated•15 years ago
|
Attachment #396526 -
Flags: superreview?(marco.zehe)
Assignee | ||
Updated•15 years ago
|
Attachment #396526 -
Flags: superreview?(marco.zehe) → review?(marco.zehe)
Reporter | ||
Updated•15 years ago
|
Attachment #396526 -
Flags: review?(marco.zehe) → review+
Comment 15•15 years ago
|
||
If we agree empty role should be treated as if it would absent then we shouldn't initialize mRoleMapEntry it would be nice to extend testcases, we should have test for ARIA role not mapped to AT role.
Assignee | ||
Comment 16•15 years ago
|
||
This feels like it is getting messy (too many special cases). Thoughts? I tested manually the cases that are currently commented out. Do we want to file a bug on making our document accessible refresh based on dynamic changes to the body role attribute? I'm leaning towards patch 1 as my favorite; but if we feel this is most robust I'm fine with pushing it.
Attachment #396526 -
Attachment is obsolete: true
Attachment #396854 -
Flags: review?(surkov.alexander)
Attachment #396526 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 17•15 years ago
|
||
(In reply to comment #16) > I'm leaning towards patch 1 as my favorite I mean't patch 2.
Comment 18•15 years ago
|
||
(In reply to comment #16) > Created an attachment (id=396854) [details] > WIP > > This feels like it is getting messy (too many special cases). Thoughts? I think I like this. > I > tested manually the cases that are currently commented out. Do we want to file > a bug on making our document accessible refresh based on dynamic changes to the > body role attribute? Technically we should invalidate the accessible if role attr is changed. Does it not work?
Comment 19•15 years ago
|
||
Comment on attachment 396854 [details] [diff] [review] WIP >- if (!content || !content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::role, roleString)) { >+ if (!content || >+ !content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::role, roleString) || >+ roleString.IsEmpty()) { >+ // We treat role="" as if the role attribute is absent nit: would be nice to refer to the spec. >+ // We only force the readonly bit off if we have a real mapping for the aria >+ // role. This preserves the ability for screen readers to use readonly >+ // (primarily on the document) as the hint for creating a virtual buffer. >+ if (mRoleMapEntry->role != nsIAccessibleRole::ROLE_NOTHING) { >+ *aState &= ~nsIAccessibleStates::STATE_READONLY; >+ } nit: no braces needed for a single if statement.
Assignee | ||
Comment 20•15 years ago
|
||
(In reply to comment #18) > (In reply to comment #16) > > Created an attachment (id=396854) [details] [details] > > WIP > > > > This feels like it is getting messy (too many special cases). Thoughts? > > I think I like this. > OK. > > I > > tested manually the cases that are currently commented out. Do we want to file > > a bug on making our document accessible refresh based on dynamic changes to the > > body role attribute? > > Technically we should invalidate the accessible if role attr is changed. Does > it not work? Doesn't seem to.
Assignee | ||
Comment 21•15 years ago
|
||
I can fix the nits locally, or would you prefer a new patch?
Comment 22•15 years ago
|
||
Comment on attachment 396854 [details] [diff] [review] WIP if you like to deal with invalidation problem in another bug then r=me
Attachment #396854 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Updated•15 years ago
|
Summary: Role attribute on body with empty string causes DocAccessible not to have read-only state. → Empty role on body removes read-only state; breaks virtual buffers
Assignee | ||
Comment 23•15 years ago
|
||
Pushed as changeset: http://hg.mozilla.org/mozilla-central/rev/6aab4d7bb538 Spun off bug 513679 for doc updating. Thanks guys.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•