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)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: MarcoZ, Assigned: davidb)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 5 obsolete files)

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.
I think I see the problem, taking.
Assignee: nobody → bolterbugz
Attached patch wip (obsolete) — Splinter Review
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.
Attached patch patch1 (obsolete) — Splinter Review
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)
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+
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 :)
(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.
Attachment #394342 - Flags: review?(surkov.alexander)
Attached patch patch2 (obsolete) — Splinter Review
I reworked the wip and added the tests.
Attachment #394342 - Attachment is obsolete: true
Attachment #395091 - Flags: review?(surkov.alexander)
does the spec treat empty role attribute and missed role attribute differently?
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)
Attached patch the real patch2 (obsolete) — Splinter Review
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)
(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.
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.
Attached patch patch 3 (more robust approach) (obsolete) — Splinter Review
Attachment #395583 - Attachment is obsolete: true
Attachment #396526 - Flags: review?(surkov.alexander)
Attachment #395583 - Flags: review?(surkov.alexander)
Attachment #396526 - Flags: superreview?(marco.zehe)
Attachment #396526 - Flags: superreview?(marco.zehe) → review?(marco.zehe)
Attachment #396526 - Flags: review?(marco.zehe) → review+
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.
Attached patch WIPSplinter Review
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)
(In reply to comment #16)
> I'm leaning towards patch 1 as my favorite

I mean't patch 2.
(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 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.
(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.
I can fix the nits locally, or would you prefer a new patch?
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+
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
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.

Attachment

General

Created:
Updated:
Size: