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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

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.

Comment 1

[alexander :surkov (:asurkov)]

let's get blocking, we need to fix a mess we have.

Comment 2

[alexander :surkov (:asurkov)]

(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.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: part1: no mContent assumptions

Comment 4

[alexander :surkov (:asurkov)]

Attachment: part2: no content documents

Comment 5

[Trevor Saunders (:tbsaunde)]

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.

Comment 6

[Trevor Saunders (:tbsaunde)]

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() ?

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch

combined version, Trev's findings are addressed

Comment 8

[Trevor Saunders (:tbsaunde)]

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?

Comment 9

[alexander :surkov (:asurkov)]

(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.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch2

fix AT crashes

Comment 11

[Trevor Saunders (:tbsaunde)]

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()

Comment 12

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e0271552b1b0

Comment 13

[Ed Morley [:emorley]]

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