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)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(2 files, 4 obsolete files)
6.28 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
9.60 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #688654 -
Flags: review?(trev.saunders)
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #688660 -
Flags: review?(trev.saunders)
Comment 2•11 years ago
|
||
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•11 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•11 years ago
|
||
updated to trunk
Attachment #688654 -
Attachment is obsolete: true
Attachment #688654 -
Flags: review?(trev.saunders)
Attachment #693206 -
Flags: review?(trev.saunders)
Comment 5•11 years ago
|
||
(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•11 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.
Comment 7•11 years ago
|
||
(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•11 years ago
|
||
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•11 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.
Comment 10•11 years ago
|
||
(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•11 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.
Comment 12•11 years ago
|
||
(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•11 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?
Comment 14•11 years ago
|
||
> > > 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•11 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.
Comment 16•11 years ago
|
||
(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.
Updated•11 years ago
|
Attachment #693206 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Comment 17•11 years ago
|
||
part1: http://hg.mozilla.org/integration/mozilla-inbound/rev/5d59a9ec28f4
Whiteboard: [leave open]
Comment 18•11 years ago
|
||
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•11 years ago
|
||
Attachment #694256 -
Flags: review?(trev.saunders)
Updated•11 years ago
|
Assignee: surkov.alexander → mh+mozilla
Updated•11 years ago
|
Assignee: mh+mozilla → surkov.alexander
Comment 20•11 years ago
|
||
Attachment #694258 -
Flags: review?(trev.saunders)
Updated•11 years ago
|
Attachment #694256 -
Attachment is obsolete: true
Attachment #694256 -
Flags: review?(trev.saunders)
Comment 21•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #694258 -
Attachment is obsolete: true
Comment 23•11 years ago
|
||
(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•11 years ago
|
||
relanding: http://hg.mozilla.org/integration/mozilla-inbound/rev/d08057e095a2 adding #include Accessible-inl.h in cpp where needed
Comment 25•11 years ago
|
||
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•11 years ago
|
||
Comment on attachment 694266 [details] [diff] [review] Fixup for build failure with some toolchains Indeed
Attachment #694266 -
Flags: review?(trev.saunders)
Updated•11 years ago
|
Attachment #694266 -
Attachment is obsolete: true
Comment 27•11 years ago
|
||
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 28•11 years ago
|
||
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•11 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•11 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•11 years ago
|
||
bug 823812 filed on Accessible::SetRoleMapEntry issue
Updated•11 years ago
|
Attachment #688660 -
Flags: review?(trev.saunders) → review+
Comment 33•11 years ago
|
||
> where?
treeupdate/test_doc.html the test you changed, maybe you fixed locally?
Assignee | ||
Comment 34•11 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•11 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]
Comment 36•11 years ago
|
||
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.
Description
•