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

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks 1 bug, {access})

unspecified
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [bk1])

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

9 years ago
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.
Assignee

Comment 1

9 years ago
let's get blocking, we need to fix a mess we have.
blocking2.0: --- → ?
Assignee

Comment 2

9 years ago
(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]
Assignee

Comment 3

7 years ago
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #673175 - Flags: review?(trev.saunders)
Assignee

Comment 4

7 years ago
Posted patch part2: no content documents (obsolete) — Splinter Review
Attachment #673177 - Flags: review?(trev.saunders)
Assignee

Updated

7 years ago
Depends on: 741408
Assignee

Updated

7 years ago
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)
Assignee

Comment 7

7 years ago
Posted 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+
Assignee

Comment 9

7 years ago
(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.
Assignee

Comment 10

7 years ago
Posted 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
Last Resolved: 7 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.