Closed Bug 612830 Opened 10 years ago Closed 8 years ago

make HTML document accessible work even when there's no body

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access, Whiteboard: [bk1])

Attachments

(1 file, 3 obsolete files)

1) We don't update tree when there's no body.
2) We set stale state when there's no body and continue working with old body.
3) We don't create document accessible without body (perhaps we should, though I'm not sure if this case is possible).

Perhaps the body shouldn't play the role it plays now and we should use document element instead (well, take into account role attribute on body and nothing else). If we follow this way then we can save all rules we have for body and we should be in good shape.
let's get blocking, we need to fix a mess we have.
blocking2.0: --- → ?
(In reply to comment #0)

> Perhaps the body shouldn't play the role it plays now and we should use
> document element instead (well, take into account role attribute on body and
> nothing else).

Hm, ARIA stuffs are applicable to body only per ARIA impl guide. The suggestion looks wrong.

> If we follow this way then we can save all rules we have for
> body and we should be in good shape.

So we can't replace mContent on document element because we have lot of mContent usage in nsAccessible class related with ARIA stuffs. We might want to keep it nsnull but we need to add lot of null checks in base classes.

But anyway I don't like to keep document accessible stale when body was just removed. It doesn't sound like good idea.
blocking2.0: ? → ---
Summary: make HTML document accessible work under assumption there's no body → make HTML document accessible work even when there's no body
Whiteboard: [bk1]
Attached patch part1: no mContent assumptions (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #673175 - Flags: review?(trev.saunders)
Attached patch part2: no content documents (obsolete) — Splinter Review
Attachment #673177 - Flags: review?(trev.saunders)
Depends on: 741408
Blocks: 799133
Comment on attachment 673175 [details] [diff] [review]
part1: no mContent assumptions

> Accessible::ApplyARIAState(uint64_t* aState) const
> {
>-  if (!mContent->IsElement())
>+  if (!HasOwnContent() || !mContent->IsElement())

so, DocAccessible already overrides this, why not handle it there?  I guess you'd need to over ride it in DummyAccessible to be safe in all cases, but DocAccessible should be enough for this bug I think.
Attachment #673175 - Flags: review?(trev.saunders) → review+
Comment on attachment 673177 [details] [diff] [review]
part2: no content documents

> DocAccessible::NativeState()
> {

it looks like the call to GetEditor() below  will always crash because it assumes there is content.

>-  if (contentElm && mContent != contentElm)
>+  if (mContent != contentElm)

shouldn't you do the same thing in ProcessContentInserted() ?
Attachment #673177 - Flags: review?(trev.saunders)
Attached patch patch (obsolete) — Splinter Review
combined version, Trev's findings are addressed
Attachment #673175 - Attachment is obsolete: true
Attachment #673177 - Attachment is obsolete: true
Attachment #674101 - Flags: review?(trev.saunders)
Comment on attachment 674101 [details] [diff] [review]
patch

>       this.finalCheck = function rootContentRemoved_finalCheck()
>       {
>         var tree = {
>           role: ROLE_DOCUMENT,
>-          states: {
>-            // Out of date root content involves stale state presence.
>-            states: 0,
>-            extraStates: EXT_STATE_STALE
>-          },
>           children: [ ]
>         };
>         testAccessibleTree(getDocNode(aID), tree);

might it be better to check stale is absent?
Attachment #674101 - Flags: review?(trev.saunders) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #8)

> might it be better to check stale is absent?

I wouldn't really want to test absent things that AT don't depend on.
Attached patch patch2Splinter Review
fix AT crashes
Attachment #674101 - Attachment is obsolete: true
Attachment #675947 - Flags: review?(trev.saunders)
Comment on attachment 675947 [details] [diff] [review]
patch2

> DocAccessible::NativeState()
> {
>-  // The root content of the document might be removed so that mContent is
>-  // out of date.
>-  uint64_t state = (mContent->GetCurrentDoc() == mDocument) ?
>-    0 : states::STALE;
>-
>   // Document is always focusable.
>-  state |= states::FOCUSABLE; // keep in sync with NativeIteractiveState() impl
>+  uint64_t state = states::FOCUSABLE; // keep in sync with NativeIteractiveState() impl

spelling nit, NativeInteractiveState()
Attachment #675947 - Flags: review?(trev.saunders) → review+
https://hg.mozilla.org/mozilla-central/rev/e0271552b1b0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 807596
You need to log in before you can comment on or make changes to this bug.