Closed Bug 818407 Opened 11 years ago Closed 11 years ago

changing the HTML body doesn't pick up ARIA role

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(2 files, 4 obsolete files)

      No description provided.
Attachment #688654 - Flags: review?(trev.saunders)
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.
(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.
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?
(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.
(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;
}
(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.
(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.
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+
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
Attachment #694256 - Flags: review?(trev.saunders)
Assignee: surkov.alexander → mh+mozilla
Assignee: mh+mozilla → surkov.alexander
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)
This is getting messy.
Attachment #694266 - Flags: review?(trev.saunders)
Attachment #694258 - Attachment is obsolete: true
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
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?
(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?
(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
bug 823812 filed on Accessible::SetRoleMapEntry issue
Attachment #688660 - Flags: review?(trev.saunders) → review+
> where?

treeupdate/test_doc.html the test you changed, maybe you fixed locally?
(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 :)
https://hg.mozilla.org/mozilla-central/rev/7192d0dae605
Status: ASSIGNED → RESOLVED
Closed: 11 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.