Last Comment Bug 818407 - changing the HTML body doesn't pick up ARIA role
: changing the HTML body doesn't pick up ARIA role
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla20
Assigned To: alexander :surkov
:
: alexander :surkov
Mentors:
Depends on:
Blocks: aria
  Show dependency treegraph
 
Reported: 2012-12-05 00:51 PST by alexander :surkov
Modified: 2012-12-26 05:21 PST (History)
2 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part1: don't mix up with types of ARIA role (8.61 KB, patch)
2012-12-05 00:51 PST, alexander :surkov
no flags Details | Diff | Splinter Review
part2: update role map (6.28 KB, patch)
2012-12-05 01:17 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
part1 (v2): don't mix up with types of ARIA role (9.60 KB, patch)
2012-12-17 18:20 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
Fixup for build failure with some toolchains (4.62 KB, patch)
2012-12-20 00:19 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Fixup for build failure with some toolchains (957 bytes, patch)
2012-12-20 00:31 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Fixup for build failure with some toolchains (1.96 KB, patch)
2012-12-20 00:55 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review

Description alexander :surkov 2012-12-05 00:51:14 PST
Created attachment 688654 [details] [diff] [review]
part1: don't mix up with types of ARIA role
Comment 1 alexander :surkov 2012-12-05 01:17:38 PST
Created attachment 688660 [details] [diff] [review]
part2: update role map
Comment 2 Trevor Saunders (:tbsaunde) 2012-12-10 03:57:07 PST
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.
Comment 3 alexander :surkov 2012-12-10 21:24:38 PST
(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.
Comment 4 alexander :surkov 2012-12-17 18:20:46 PST
Created attachment 693206 [details] [diff] [review]
part1 (v2): don't mix up with types of ARIA role

updated to trunk
Comment 5 Trevor Saunders (:tbsaunde) 2012-12-18 01:57:38 PST
(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?
Comment 6 alexander :surkov 2012-12-18 02:09:07 PST
(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.
Comment 7 Trevor Saunders (:tbsaunde) 2012-12-18 02:19:47 PST
(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 8 Trevor Saunders (:tbsaunde) 2012-12-18 02:22:15 PST
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.
Comment 9 alexander :surkov 2012-12-18 02:55:09 PST
(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.
Comment 10 Trevor Saunders (:tbsaunde) 2012-12-18 04:59:15 PST
(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;
}
Comment 11 alexander :surkov 2012-12-18 07:28:33 PST
(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.
Comment 12 Trevor Saunders (:tbsaunde) 2012-12-18 17:57:48 PST
(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.
Comment 13 alexander :surkov 2012-12-18 20:22:03 PST
(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?
Comment 14 Trevor Saunders (:tbsaunde) 2012-12-19 03:06:26 PST
> > > 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.
Comment 15 alexander :surkov 2012-12-19 03:27:17 PST
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.
Comment 16 Trevor Saunders (:tbsaunde) 2012-12-19 11:30:52 PST
(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.
Comment 18 Mike Hommey [:glandium] 2012-12-20 00:13:31 PST
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
Comment 19 Mike Hommey [:glandium] 2012-12-20 00:19:49 PST
Created attachment 694256 [details] [diff] [review]
Fixup for build failure with some toolchains
Comment 20 Mike Hommey [:glandium] 2012-12-20 00:31:12 PST
Created attachment 694258 [details] [diff] [review]
Fixup for build failure with some toolchains
Comment 21 Mike Hommey [:glandium] 2012-12-20 00:47:37 PST
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"
         ^
Comment 22 Mike Hommey [:glandium] 2012-12-20 00:55:30 PST
Created attachment 694266 [details] [diff] [review]
Fixup for build failure with some toolchains

This is getting messy.
Comment 24 alexander :surkov 2012-12-20 03:50:01 PST
relanding: http://hg.mozilla.org/integration/mozilla-inbound/rev/d08057e095a2
adding #include Accessible-inl.h in cpp where needed
Comment 25 Trevor Saunders (:tbsaunde) 2012-12-20 05:38:52 PST
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 26 Mike Hommey [:glandium] 2012-12-20 06:11:08 PST
Comment on attachment 694266 [details] [diff] [review]
Fixup for build failure with some toolchains

Indeed
Comment 28 Trevor Saunders (:tbsaunde) 2012-12-20 09:06:23 PST
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?
Comment 29 alexander :surkov 2012-12-20 19:11:30 PST
(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?
Comment 30 alexander :surkov 2012-12-20 19:17:50 PST
(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
Comment 31 alexander :surkov 2012-12-20 19:39:04 PST
bug 823812 filed on Accessible::SetRoleMapEntry issue
Comment 32 :Ms2ger (⌚ UTC+1/+2) 2012-12-22 06:53:29 PST
https://hg.mozilla.org/mozilla-central/rev/1412ac59be4f
Comment 33 Trevor Saunders (:tbsaunde) 2012-12-24 02:01:06 PST
> where?

treeupdate/test_doc.html the test you changed, maybe you fixed locally?
Comment 34 alexander :surkov 2012-12-24 02:04:13 PST
(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 :)
Comment 35 alexander :surkov 2012-12-24 04:41:42 PST
> Comment on attachment 688660 [details] [diff] [review]
> part2: update role map

http://hg.mozilla.org/integration/mozilla-inbound/rev/7192d0dae605
Comment 36 Ryan VanderMeulen [:RyanVM] 2012-12-26 05:21:27 PST
https://hg.mozilla.org/mozilla-central/rev/7192d0dae605

Note You need to log in before you can comment on or make changes to this bug.