changing the HTML body doesn't pick up ARIA role

RESOLVED FIXED in mozilla20

Status

()

Core
Disability Access APIs
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla20
access
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 688654 [details] [diff] [review]
part1: don't mix up with types of ARIA role
Attachment #688654 - Flags: review?(trev.saunders)
(Assignee)

Comment 1

4 years ago
Created attachment 688660 [details] [diff] [review]
part2: update role map
Attachment #688660 - Flags: review?(trev.saunders)
Comment on attachment 688654 [details] [diff] [review]
part1: don't mix up with types of ARIA role

>+Accessible::HasGenericType(AccGenericType aType) const
> {
>-  mRoleMapEntry = aRoleMapEntry;
>-  if (mRoleMapEntry)
>-    mGenericTypes |= mRoleMapEntry->accTypes;
>+  return (mGenericTypes & aType) ||
>+    (mRoleMapEntry && mRoleMapEntry->IsOfType(aType));

you need to to this because changing the body can change the role map which can mean you need to change the generic type bits, and you don't know which ofthe existing ones came from aria and which not?

So this brings up an interesting case, it would seem you can change the role of a document mid life, and it won't be recreated the way a non doc accessible would be.

>+  bool IsDoc() const { return HasGenericType(eDocuments); }

This concerns me a bit since if we add eDocument to the generic type in a rolemap entry even accessibles that aren't HyperTextAccessibles will be downcastable to that class, or whatever type is involved.
(Assignee)

Comment 3

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #2)

> you need to to this because changing the body can change the role map which
> can mean you need to change the generic type bits, and you don't know which
> ofthe existing ones came from aria and which not?

yes

> So this brings up an interesting case, it would seem you can change the role
> of a document mid life, and it won't be recreated the way a non doc
> accessible would be.

details? we track @role changes on body element and update the aria map.

> >+  bool IsDoc() const { return HasGenericType(eDocuments); }
> 
> This concerns me a bit since if we add eDocument to the generic type in a
> rolemap entry even accessibles that aren't HyperTextAccessibles will be
> downcastable to that class, or whatever type is involved.

It seems you did that suggestion in bug 810572. I think I can address it there.
(Assignee)

Comment 4

4 years ago
Created attachment 693206 [details] [diff] [review]
part1 (v2): don't mix up with types of ARIA role

updated to trunk
Attachment #688654 - Attachment is obsolete: true
Attachment #688654 - Flags: review?(trev.saunders)
Attachment #693206 - Flags: review?(trev.saunders)
(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > you need to to this because changing the body can change the role map which
> > can mean you need to change the generic type bits, and you don't know which
> > ofthe existing ones came from aria and which not?
> 
> yes
> 
> > So this brings up an interesting case, it would seem you can change the role
> > of a document mid life, and it won't be recreated the way a non doc
> > accessible would be.
> 
> details? we track @role changes on body element and update the aria map.

sure, butt shouldn't we fire events when documents role changes which changing the role map can do?

Also if we're going to allow documents to change there role mid life without being recreated do we want to consider exending that to other accessibles?
(Assignee)

Comment 6

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> sure, butt shouldn't we fire events when documents role changes which
> changing the role map can do?
> 
> Also if we're going to allow documents to change there role mid life without
> being recreated do we want to consider exending that to other accessibles?

as you know there's no events when role is changed, we recreate an accessible. I wouldn't recreate the whole document when its role is changed at least for now and I wouldn't do opposite things for non document accessibles. In other words I wouldn't change behavior for now. We don't have any single use case that needs that.

You might be curious why I was looking at this bug at all. Bug 660083 makes us to create an accessible a little bit early so that intermittently we create a document accessible having no body. That was a cause of this bug.
(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > sure, butt shouldn't we fire events when documents role changes which
> > changing the role map can do?
> > 
> > Also if we're going to allow documents to change there role mid life without
> > being recreated do we want to consider exending that to other accessibles?
> 
> as you know there's no events when role is changed, we recreate an
> accessible. I wouldn't recreate the whole document when its role is changed
> at least for now and I wouldn't do opposite things for non document
> accessibles. In other words I wouldn't change behavior for now. We don't
> have any single use case that needs that.


true, though it might make some things a bit faster.
Comment on attachment 693206 [details] [diff] [review]
part1 (v2): don't mix up with types of ARIA role

>-Accessible::SetRoleMapEntry(nsRoleMapEntry* aRoleMapEntry)
>+inline bool
>+Accessible::HasGenericType(AccGenericType aType) const
> {
>-  mRoleMapEntry = aRoleMapEntry;
>-  if (mRoleMapEntry)
>-    mGenericTypes |= mRoleMapEntry->accTypes;
>+  return (mGenericTypes & aType) ||
>+    (mRoleMapEntry && mRoleMapEntry->IsOfType(aType));

if the only thing that can have its role map change is a document, I think I'd rather add a special case to SetRoleMapEntry() than slow down checking generic types significantly.
(Assignee)

Comment 9

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #7)

> true, though it might make some things a bit faster.

we should back child adoption logic for those

(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> >-  if (mRoleMapEntry)
> >-    mGenericTypes |= mRoleMapEntry->accTypes;
> >+  return (mGenericTypes & aType) ||
> >+    (mRoleMapEntry && mRoleMapEntry->IsOfType(aType));
> 
> if the only thing that can have its role map change is a document, I think
> I'd rather add a special case to SetRoleMapEntry() than slow down checking
> generic types significantly.

It doesn't seem as a real significant slowdown and until it is I would keep the code in unified way. Keeping ARIA role types in different ways depending on accessible type complicates a logic.
(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> 
> > true, though it might make some things a bit faster.
> 
> we should back child adoption logic for those

I'd think changing the role and firing events would be even faster, but if some platforms don't support that then it might be hard.

> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > >-  if (mRoleMapEntry)
> > >-    mGenericTypes |= mRoleMapEntry->accTypes;
> > >+  return (mGenericTypes & aType) ||
> > >+    (mRoleMapEntry && mRoleMapEntry->IsOfType(aType));
> > 
> > if the only thing that can have its role map change is a document, I think
> > I'd rather add a special case to SetRoleMapEntry() than slow down checking
> > generic types significantly.
> 
> It doesn't seem as a real significant slowdown and until it is I would keep
> the code in unified way. Keeping ARIA role types in different ways depending
> on accessible type complicates a logic.

I don't really ant to prove it, but I'd think its at least twice as slow as it was, since you add a couple branches and fetching memory for the role map.

I wasn't suggesting  changing how things are stored, I was suggesting SetRoleMapEntry do this

void
Accessible::SetRoleMapEntry(RoleMapEntry* aEnt)
{
  if (mRoleMapEntry) {
    NS_ASSERTION(IsDoc(), "chainging role map entry of not document!");
    mGenericType &= ~mRoleMapEntry->AccType; // documents have no generic types that can also come from aria
  }

  mRoleMapEntry = aEnt;
  mGenericType |= aEnt->AccType;
}
(Assignee)

Comment 11

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #10)

> I don't really ant to prove it, but I'd think its at least twice as slow as
> it was, since you add a couple branches and fetching memory for the role map.

right, the question is whether 1 tick is fast enough to not feel the slowness of 2 ticks. I think the most heavy job we do is accessible tree creation but even there these methods aren't called for each iterated node.

> I wasn't suggesting  changing how things are stored, I was suggesting
> SetRoleMapEntry do this
> 
> void
> Accessible::SetRoleMapEntry(RoleMapEntry* aEnt)
> {
>   if (mRoleMapEntry) {
>     NS_ASSERTION(IsDoc(), "chainging role map entry of not document!");
>     mGenericType &= ~mRoleMapEntry->AccType; // documents have no generic
> types that can also come from aria
>   }

this doesn't seem working when we share the logic between native and ARIA documents.

> 
>   mRoleMapEntry = aEnt;
>   mGenericType |= aEnt->AccType;
> }

that works until we stop to recreate an accessible/tree when it doesn't make sense (for example, on setting/changing weak roles (granted they are likely won't provide a type) or on platforms that can live without accessible recreation like ATK).

It sounds like we try to optimize stuff without any evidence they need to be optimized and we sacrifice some code flexibility. If you like then we can have a separate bug on it to get back to it one day.
(In reply to alexander :surkov from comment #11)
> (In reply to Trevor Saunders (:tbsaunde) from comment #10)
> 
> > I don't really ant to prove it, but I'd think its at least twice as slow as
> > it was, since you add a couple branches and fetching memory for the role map.
> 
> right, the question is whether 1 tick is fast enough to not feel the
> slowness of 2 ticks. I think the most heavy job we do is accessible tree
> creation but even there these methods aren't called for each iterated node.

I'm not sure of nice way to make GetOrCreateAccessible() faster, so I tend to think adidng any number of things there is not ok.

> > I wasn't suggesting  changing how things are stored, I was suggesting
> > SetRoleMapEntry do this
> > 
> > void
> > Accessible::SetRoleMapEntry(RoleMapEntry* aEnt)
> > {
> >   if (mRoleMapEntry) {
> >     NS_ASSERTION(IsDoc(), "chainging role map entry of not document!");
> >     mGenericType &= ~mRoleMapEntry->AccType; // documents have no generic
> > types that can also come from aria
> >   }
> 
> this doesn't seem working when we share the logic between native and ARIA
> documents.

and neither does storing if an accessible is a doc accessible then, so as I see it we need to rework this then anyway.

> > 
> >   mRoleMapEntry = aEnt;
> >   mGenericType |= aEnt->AccType;
> > }
> 
> that works until we stop to recreate an accessible/tree when it doesn't make
> sense (for example, on setting/changing weak roles (granted they are likely
> won't provide a type) or on platforms that can live without accessible
> recreation like ATK).

wouldn't eak roles still change the platform role? if not what is there purpose?

I'd really rather not have tree update work differently on different platforms, so unless we think we can avoid recreation everywhere some day then I'm terribly interested in this.

> It sounds like we try to optimize stuff without any evidence they need to be
> optimized and we sacrifice some code flexibility. If you like then we can

we know tree creation /update needs to be much faster, and this is some tiny piece of that.  It also doesn't seem like the flexibility is that bad either.
(Assignee)

Comment 13

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #12)

> wouldn't eak roles still change the platform role? if not what is there
> purpose?

they are used to add some semantics but do not affect on role, for example, landmarks roles.

> I'd really rather not have tree update work differently on different
> platforms, so unless we think we can avoid recreation everywhere some day
> then I'm terribly interested in this.

alternatively we could not recreate children when QI isn't changed since iirc that was a point of accessible recreation but that's likely out of scope of this bug.

> > It sounds like we try to optimize stuff without any evidence they need to be
> > optimized and we sacrifice some code flexibility. If you like then we can
> 
> we know tree creation /update needs to be much faster, and this is some tiny
> piece of that.  It also doesn't seem like the flexibility is that bad either.

Honestly I don't believe that tiny thing will change anything. On the another hand I have concerns about flexibility of this approach. Note I'm not saying we shouldn't take this approach ever but I would keep it separately from this bug and work on it as a part of tree creation performance work. Would you be ok with that?
> > > It sounds like we try to optimize stuff without any evidence they need to be
> > > optimized and we sacrifice some code flexibility. If you like then we can
> > 
> > we know tree creation /update needs to be much faster, and this is some tiny
> > piece of that.  It also doesn't seem like the flexibility is that bad either.
> 
> Honestly I don't believe that tiny thing will change anything. On the
> another hand I have concerns about flexibility of this approach. Note I'm
> not saying we shouldn't take this approach ever but I would keep it
> separately from this bug and work on it as a part of tree creation
> performance work. Would you be ok with that?

I dont like to make things worse and say we may fix them in the future, but maybe.
(Assignee)

Comment 15

4 years ago
So am I. If I was sure that's a nice approach then I would follow it. Otherwise I consider it as possible performance improvement. And here again I'm not sure it's worth perf improvement so I suggested to file a bug to keep the issue tracked. It should be fair.
(In reply to alexander :surkov from comment #15)
> So am I. If I was sure that's a nice approach then I would follow it.

I'm not sure of anything but it doesn't seem like it would be that hard to make generic if some day we actually need it.

> Otherwise I consider it as possible performance improvement. And here again

I'd tend to think its just fixing something in the way that slows things down the least.

> I'm not sure it's worth perf improvement so I suggested to file a bug to

I don't believe it does any harm, it would take all of what 10 minutes to change back to what you did and check it works if some daay we decide we need it, but its probably not worth fighting about.

> keep the issue tracked. It should be fair.

I'm not sure what you mean by far.
Attachment #693206 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 17

4 years ago
part1: http://hg.mozilla.org/integration/mozilla-inbound/rev/5d59a9ec28f4
Whiteboard: [leave open]
Linking libxul fails on my system:

../../accessible/src/atk/nsMaiInterfaceText.o: In function `mozilla::a11y::Accessible::IsHyperText() const':
/home/mh/mozilla-central/accessible/src/atk/../generic/Accessible.h:482: undefined reference to `mozilla::a11y::Accessible::HasGenericType(mozilla::a11y::AccGenericType) const'
/home/mh/mozilla-central/accessible/src/atk/../generic/Accessible.h:482: undefined reference to `mozilla::a11y::Accessible::HasGenericType(mozilla::a11y::AccGenericType) const'
/home/mh/mozilla-central/accessible/src/atk/../generic/Accessible.h:482: undefined reference to `mozilla::a11y::Accessible::HasGenericType(mozilla::a11y::AccGenericType) const'
/home/mh/mozilla-central/accessible/src/atk/../generic/Accessible.h:482: undefined reference to `mozilla::a11y::Accessible::HasGenericType(mozilla::a11y::AccGenericType) const'
/home/mh/mozilla-central/accessible/src/atk/../generic/Accessible.h:482: undefined reference to `mozilla::a11y::Accessible::HasGenericType(mozilla::a11y::AccGenericType) const'
../../accessible/src/atk/nsMaiInterfaceText.o:/home/mh/mozilla-central/accessible/src/atk/../generic/Accessible.h:482: more undefined references to `mozilla::a11y::Accessible::HasGenericType(mozilla::a11y::AccGenericType) const' follow
Created attachment 694256 [details] [diff] [review]
Fixup for build failure with some toolchains
Attachment #694256 - Flags: review?(trev.saunders)
Assignee: surkov.alexander → mh+mozilla
Assignee: mh+mozilla → surkov.alexander
Created attachment 694258 [details] [diff] [review]
Fixup for build failure with some toolchains
Attachment #694258 - Flags: review?(trev.saunders)
Attachment #694256 - Attachment is obsolete: true
Attachment #694256 - Flags: review?(trev.saunders)
Comment on attachment 694258 [details] [diff] [review]
Fixup for build failure with some toolchains

This further breaks the build :(

In file included from /home/mh/mozilla-central/layout/base/nsPresShell.cpp:147:
In file included from ../../dist/include/mozilla/a11y/DocAccessible.h:14:
In file included from ../../dist/include/mozilla/a11y/HyperTextAccessibleWrap.h:10:
In file included from ../../dist/include/mozilla/a11y/HyperTextAccessible.h:13:
../../dist/include/mozilla/a11y/AccessibleWrap.h:11:10: fatal error: 'Accessible-inl.h' file not found
#include "Accessible-inl.h"
         ^
Attachment #694258 - Flags: review?(trev.saunders)
Created attachment 694266 [details] [diff] [review]
Fixup for build failure with some toolchains

This is getting messy.
Attachment #694266 - Flags: review?(trev.saunders)
Attachment #694258 - Attachment is obsolete: true
(In reply to alexander :surkov from comment #17)
> part1: http://hg.mozilla.org/integration/mozilla-inbound/rev/5d59a9ec28f4

Backed out for the compile failures
https://hg.mozilla.org/integration/mozilla-inbound/rev/40adf72fd612

https://tbpl.mozilla.org/php/getParsedLog.php?id=18122238&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18122142&tree=Mozilla-Inbound
(Assignee)

Comment 24

4 years ago
relanding: http://hg.mozilla.org/integration/mozilla-inbound/rev/d08057e095a2
adding #include Accessible-inl.h in cpp where needed
Comment on attachment 694266 [details] [diff] [review]
Fixup for build failure with some toolchains

Mike this is obsolete and not required with the relanded patch right?
Comment on attachment 694266 [details] [diff] [review]
Fixup for build failure with some toolchains

Indeed
Attachment #694266 - Flags: review?(trev.saunders)
Attachment #694266 - Attachment is obsolete: true
Still busted:
https://tbpl.mozilla.org/php/getParsedLog.php?id=18129165&tree=Mozilla-Inbound

Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f042470c7ae8
Comment on attachment 688660 [details] [diff] [review]
part2: update role map

sorry I forgot about this after we decided on on part 1.

If we're going to support this shouldn't we suport document.body.setAttribute("role", "button"); too?

btw add bug link?
(Assignee)

Comment 29

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #28)
> Comment on attachment 688660 [details] [diff] [review]
> part2: update role map
> 
> sorry I forgot about this after we decided on on part 1.
> 
> If we're going to support this shouldn't we suport
> document.body.setAttribute("role", "button"); too?

we support it, see http://mxr.mozilla.org/mozilla-central/source/accessible/src/generic/DocAccessible.cpp#1671

> btw add bug link?

where?
(Assignee)

Comment 30

4 years ago
(In reply to Ed Morley (Away 20th Dec-2nd Jan) [UTC+0; email:edmorley@moco] from comment #27)
> Still busted:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=18129165&tree=Mozilla-
> Inbound
> 
> Backed out:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f042470c7ae8

unhappy landing. I hope this time it's ok http://hg.mozilla.org/integration/mozilla-inbound/rev/1412ac59be4f
(Assignee)

Comment 31

4 years ago
bug 823812 filed on Accessible::SetRoleMapEntry issue
https://hg.mozilla.org/mozilla-central/rev/1412ac59be4f
Attachment #688660 - Flags: review?(trev.saunders) → review+
> where?

treeupdate/test_doc.html the test you changed, maybe you fixed locally?
(Assignee)

Comment 34

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #33)
> > where?
> 
> treeupdate/test_doc.html the test you changed, maybe you fixed locally?

got it, I've got confused in my hg queue, I was looking at different patch :)
(Assignee)

Comment 35

4 years ago
> Comment on attachment 688660 [details] [diff] [review]
> part2: update role map

http://hg.mozilla.org/integration/mozilla-inbound/rev/7192d0dae605
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/7192d0dae605
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.