Closed
Bug 1001634
Opened 11 years ago
Closed 10 years ago
Add MathMLAccessible class
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: jwei, Assigned: surkov)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 6 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
MarcoZ
:
review+
|
Details | Diff | Splinter Review |
Add a class representing presentation MathML elements. This will replace the current usage of a generic HyperTextAccessible.
Reporter | ||
Comment 1•11 years ago
|
||
Added MathMLAccessible. Currently has a base class with static methods providing an implementation to be shared by MathMLAccessible as well as the MathMLTableAccessible added in bug 1001637 which will be inheriting from HTMLTableAccessible, which isn't ideal. With regards to bug 916419 comment 26, all roles are currently still included. In addition, the mfenced operators are still unexposed in the accessible tree, and further investigation needs to be done to determine exactly how to do this.
Reporter | ||
Comment 2•11 years ago
|
||
Comment on attachment 8412901 [details] [diff] [review] MathMLAccessible WIP I couldn't find a better solution than having a common base class with static methods that MathMLAccessible and the table-related MathML accessibles inherit from, and was wondering if you could think of a better solution for this.
Attachment #8412901 -
Flags: feedback?(surkov.alexander)
Comment 3•11 years ago
|
||
> I couldn't find a better solution than having a common base class with
> static methods that MathMLAccessible and the table-related MathML
> accessibles inherit from, and was wondering if you could think of a better
> solution for this.
I forget if I asked before, but can we just have MathMLUtils? its certainly not OOish, but imho that's because OO is incapable of handling this. either that or the "base class" seem kind of fine on further reflection or atleast good enough I wouldn't worry too much about.
Assignee | ||
Comment 4•11 years ago
|
||
if we don't have nicer than static methods then I agree that utils class might be nicer than inheritance. Could you file a patch that makes the need of the hierarchy evident (mathml table patch iirc)?
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8412901 [details] [diff] [review] MathMLAccessible WIP cancelling feedback until comment is addressed
Attachment #8412901 -
Flags: feedback?(surkov.alexander)
Reporter | ||
Comment 6•10 years ago
|
||
Switched to utils class for common MathML implementations.
Attachment #8412901 -
Attachment is obsolete: true
Attachment #8434640 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8434640 [details] [diff] [review] MathMLAccessible Implementation Review of attachment 8434640 [details] [diff] [review]: ----------------------------------------------------------------- related bugs for complete overview are bug 1001635 and bug 1001637 Neil, probably you have some ideas how to implement that nicer
Attachment #8434640 -
Flags: superreview?(neil)
Comment 8•10 years ago
|
||
Comment on attachment 8434640 [details] [diff] [review] MathMLAccessible Implementation The only way I could think of improving this would be use tables of atoms. The only built-in way I can think of to achieve this is using a static atom table, but maybe you are already using lists of atoms for some other reason and can copy that code. >+void >+MathMLUtils::GetMathMLNativeAttributes(Accessible* aAccessible, >+ nsIPersistentProperties* aAttributes) >+{ >+ nsAutoString value; >+ nsIContent* content = aAccessible->GetContent(); >+ role accRole = GetMathMLRole(aAccessible); >+ >+ // accent >+ if (content->GetAttr(kNameSpaceID_None, nsGkAtoms::accent_, value) && >+ (accRole == roles::MATHML_STYLE || >+ accRole == roles::MATHML_OPERATOR || >+ accRole == roles::MATHML_OVER || >+ accRole == roles::MATHML_UNDER_OVER)) >+ nsAccUtils::SetAccAttr(aAttributes, nsGkAtoms::accent_, value); At the very least you should test the role(s) before the attribute. A possible idea I have for simplifying this is to have a table of bitmasks i.e. #define ROLEBIT(role) (1ULL << (role & 63)) for (size_t index = 0; index <= ArrayLength(sMaskList); index++) if ((sMaskList[index] & ROLEBIT(accRole)) && content->GetAttr(kNameSpaceID_None, sAttributeList[index], value)) nsAccUtils::SetAccAttr(aAttributes, sAttributeList[index], value); >+void >+MathMLUtils::GetMathMLRelationByType(Accessible* aAccessible, >+ Relation* aRelation, >+ RelationType aType) >+{ >+} [Not sure what this is achieving.] >+bool >+MathMLUtils::IsMathMLPresentationTag(const nsIAtom* aTag) >+{ ... >+} >+ >+role >+MathMLUtils::GetMathMLRole(Accessible* aAccessible) >+{ ... >+} A table lookup might help here too. If you have a 1-1 mapping of presentation tags into roles then a simple table of static atoms in role order starting with <math:math> and ending with <math:msline> would work. Otherwise you would need a list of MathML presentation tags with the appropriate role, or NOTHING if there is a presentation tag with no specific role.
Attachment #8434640 -
Flags: superreview?(neil)
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8434640 [details] [diff] [review] MathMLAccessible Implementation Review of attachment 8434640 [details] [diff] [review]: ----------------------------------------------------------------- Jonathan, do you have any time to finish it? ::: accessible/src/base/RoleMap.h @@ +1060,5 @@ > + "math", > + ATK_ROLE_UNKNOWN, > + NSAccessibilityUnknownRole, > + 0, > + IA2_ROLE_UNKNOWN, that should be equation role @@ +1068,5 @@ > + "mathml identifier", > + ATK_ROLE_UNKNOWN, > + NSAccessibilityUnknownRole, > + 0, > + IA2_ROLE_UNKNOWN, until we have a good mapping it makes sense to map into generic hypertext role, i.e. do what we currently do (we create HyperTextAccessible for them) ::: accessible/src/base/nsAccessibilityService.cpp @@ +1035,5 @@ > newAcc = new EnumRoleAccessible(content, document, roles::DIAGRAM); > } > } else if (content->IsMathML()){ > + if (MathMLUtils::IsMathMLPresentationTag(content->Tag())) > + newAcc = new MathMLAccessible(content, document); we could use EnumRole here as well if we are ok to spend extra byte for it, that will save us from long if/else to calculate accessible role ::: accessible/src/generic/Accessible.cpp @@ +2485,5 @@ > // Accessible protected > ENameValueFlag > Accessible::NativeName(nsString& aName) > { > + if (mContent->IsHTML() || mContent->IsMathML()) { MathML content can be labelled by HTML? ::: accessible/src/generic/MathMLAccessible.cpp @@ +19,5 @@ > + > +MathMLAccessible::MathMLAccessible(nsIContent* aNode, DocAccessible* aDoc) : > + HyperTextAccessibleWrap(aNode, aDoc) > +{ > + mType = eMathMLType; this one sounds like a quite generic type @@ +28,5 @@ > +{ > + nsCOMPtr<nsIPersistentProperties> attributes = > + HyperTextAccessibleWrap::NativeAttributes(); > + > + MathMLUtils::GetMathMLNativeAttributes(this, attributes); rather SetMathMLAttrs() to stay closer to nsAccUtils pattern @@ +36,5 @@ > + > +role > +MathMLAccessible::NativeRole() > +{ > + return MathMLUtils::GetMathMLRole(this); maybe MathMLRoleFor(nsIContent* aElm)? @@ +43,5 @@ > +Relation > +MathMLAccessible::RelationByType(RelationType aType) > +{ > + Relation rel = HyperTextAccessibleWrap::RelationByType(aType); > + MathMLUtils::GetMathMLRelationByType(this, &rel, aType); maybe SetMathMLRelations() ::: accessible/src/generic/MathMLUtils.h @@ +11,5 @@ > + > +/** > + * Utility class for an accessible representing a MathML node. > + */ > +class MathMLUtils it doesn't seem to be a big class so it makes sense to merge it with nsAccUtils ::: accessible/tests/mochitest/name/test_mathml.html @@ +16,5 @@ > + > + <script type="application/javascript"> > + function doTest() > + { > + testTokenValue("mi", "y"); i'm not sure they have to have subtree, I think AT is supposed to use text interface, like paragraphs, after all they are closer to paragraphs rather than buttons
Attachment #8434640 -
Flags: review?(surkov.alexander) → review+
Reporter | ||
Comment 11•10 years ago
|
||
(In reply to alexander :surkov from comment #10) > Jonathan, do you have any time to finish it? I'll revisit this again tomorrow and apply suggestions from comments. There's a bunch of refactoring to do as code has moved around since I wrote this and the other patches - which is good since I need to re-familiarise myself with the code base.
Assignee | ||
Comment 12•10 years ago
|
||
Thanks for doing this! Sorry it took so long. Mostly stuff can be easily fixed by removing "src/" from your patches. Also I guess it'd be nice to keep MathMLAccessible classes inside html folder since mathml is part of html5. And pls address Neil's comments.
Reporter | ||
Comment 14•10 years ago
|
||
(In reply to Frédéric Wang (:fredw) from comment #13) > Just unbitrotting the patch... Thanks for the refresh on this and the other patches. (In reply to alexander :surkov from comment #10) > we could use EnumRole here as well if we are ok to spend extra byte for it, > that will save us from long if/else to calculate accessible role I'll probably set up a map from the atom to the role enum value. It'll simplify this and MathMLRoleFor. For SetMathMLAttrs, I'll put together a bitmask table for roles to attributes as suggested by Neil. > MathML content can be labelled by HTML? Sorry, what do you mean? > rather SetMathMLAttrs() to stay closer to nsAccUtils pattern > maybe MathMLRoleFor(nsIContent* aElm)? > maybe SetMathMLRelations() Renamed. > it doesn't seem to be a big class so it makes sense to merge it with > nsAccUtils True. Moved to nsAccUtils. > i'm not sure they have to have subtree, I think AT is supposed to use text > interface, like paragraphs, after all they are closer to paragraphs rather > than buttons Okay. So would getText() be a better way to test these?
Comment 15•10 years ago
|
||
@Jonathan: what is the rationale for exposing the following elements to the accessibility tree? <mspace> <mphantom> <mpadded> <maligngroup> <malignmark> After discussion with Joanie, these will probably not be needed for ATK as they are only involved in visual stuff. I see that you are converting them to NSAccessibilityGroupRole role in attachment 8532959 [details] [diff] [review]. Is it something required by the current WebKit Mac implementation? At least <mphantom> is visibility=hidden in WebKit and I've opened bug 1108378 to do the same in Gecko.
Flags: needinfo?(jwei)
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Frédéric Wang (:fredw) from comment #15) > @Jonathan: what is the rationale for exposing the following elements to the > accessibility tree? let's move discussion to bug 1108378
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Jonathan Wei [:jwei] from comment #14) > > MathML content can be labelled by HTML? > > Sorry, what do you mean? If I read you right the path you touched now allows to use HTML label for MathML elements, something like <mo id="m"></mo><label for="m">label</label> > > i'm not sure they have to have subtree, I think AT is supposed to use text > > interface, like paragraphs, after all they are closer to paragraphs rather > > than buttons > > Okay. So would getText() be a better way to test these? text interface is must have I think, I'm not sure about name, maybe Jamie has answer.
Reporter | ||
Comment 18•10 years ago
|
||
(In reply to Frédéric Wang (:fredw) from comment #15) > @Jonathan: what is the rationale for exposing the following elements to the > accessibility tree? I had included them in case ATs would like to infer author intent from the visual elements, but I guess that's difficult to do generically, and the author shouldn't be using visual layout to change semantic meaning anyway. I'm totally fine with having them removed from the list of MathML presentation elements unless there was a particular reason to keep them. > I see that you are converting them > to NSAccessibilityGroupRole role in attachment 8532959 [details] [diff] [review] > Is it something required by the current WebKit Mac implementation? That was done for all the MathML accessibles since that's how WebKit exposes them on OS X.
Flags: needinfo?(jwei)
Reporter | ||
Comment 19•10 years ago
|
||
(In reply to alexander :surkov from comment #17) > If I read you right the path you touched now allows to use HTML label for > MathML elements, something like > <mo id="m"></mo><label for="m">label</label> Hmm. Yeah, that'd be a bit weird. Maybe only call GetNameFromSubtree? If it makes sense to calculate names for the container types based on their children.
Comment 20•10 years ago
|
||
(In reply to Jonathan Wei [:jwei] from comment #18) > I had included them in case ATs would like to infer author intent from the > visual elements, but I guess that's difficult to do generically, and the > author shouldn't be using visual layout to change semantic meaning anyway. > I'm totally fine with having them removed from the list of MathML > presentation elements unless there was a particular reason to keep them. OK, would it be possible to modify the accessibility code to not expose these elements at all? Adding visibility=hidden as in bug 1108378 will not work for mspace/mpadded because many reftests rely on the fact that they can have a background color applied.
Flags: needinfo?(jwei)
Reporter | ||
Comment 21•10 years ago
|
||
(In reply to Frédéric Wang (:fredw) from comment #20) > OK, would it be possible to modify the accessibility code to not expose > these elements at all? Adding visibility=hidden as in bug 1108378 will not > work for mspace/mpadded because many reftests rely on the fact that they can > have a background color applied. Yes. They can just not have accessibles created for them at the same point we decide how to construct the accessible.
Flags: needinfo?(jwei)
Comment 22•10 years ago
|
||
(In reply to Jonathan Wei [:jwei] from comment #21) > Yes. They can just not have accessibles created for them at the same point > we decide how to construct the accessible. Thank you. I actually did some testing and figured that out yesterday. I think the only special case is for <mphantom> where we also want the descendants not to have any accessible exposed. The patch from bug 1108378 will work for that.
Comment 23•10 years ago
|
||
Here is the idea, but that needs to be properly merged into the original patch.
Attachment #8434640 -
Attachment is obsolete: true
Reporter | ||
Comment 25•10 years ago
|
||
Addressed most of the comments. Added static arrays for mapping content tag to role, and bit-mask table for mapping roles to each attribute. Don't create accessibles for mspace, mphantom, mpadded, malignmentgroup, and malignmark. TODO: fix mochitests.
Attachment #8532910 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8534888 -
Attachment is obsolete: true
Updated•10 years ago
|
Comment 27•10 years ago
|
||
Hi Jonathan. I refreshed your patches again (I have not tested yet, so I hope nothing is broken). I saw that you wanted to fix the Mochitest here. Can you indicate the status on your work and when you plan to update the patch? Thanks.
Flags: needinfo?(jwei)
Assignee | ||
Comment 28•10 years ago
|
||
I'll take it if John doesn't mind
Assignee: jwei → surkov.alexander
Flags: needinfo?(jwei)
Assignee | ||
Comment 29•10 years ago
|
||
basically this is Jonathan patch, just updated to current approaches. Many mappings can be questionable but that should be ok as first step on the way. I think I'll file follow ups to sort things out.
Attachment #8574240 -
Flags: review?(mzehe)
Reporter | ||
Comment 30•10 years ago
|
||
(In reply to Frédéric Wang (:fredw) from comment #27) > Hi Jonathan. I refreshed your patches again (I have not tested yet, so I > hope nothing is broken). I saw that you wanted to fix the Mochitest here. > Can you indicate the status on your work and when you plan to update the > patch? Thanks. (In reply to alexander :surkov from comment #28) > I'll take it if John doesn't mind Not at all. I had hoped to get this and the other changes finished much earlier, but unfortunately didn't have the chance and won't be able to do so until around April. Apologies again for all the delays.
Assignee | ||
Comment 31•10 years ago
|
||
(In reply to Jonathan Wei [:jwei] from comment #30) > Not at all. I had hoped to get this and the other changes finished much > earlier, but unfortunately didn't have the chance and won't be able to do so > until around April. Apologies again for all the delays. no worries, I just felt like we need finish the important work you did. You're always welcome back.
Comment 32•10 years ago
|
||
Comment on attachment 8574240 [details] [diff] [review] patch OK, get this baby off the ground, then!
Attachment #8574240 -
Flags: review?(mzehe) → review+
Assignee | ||
Comment 33•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e5a79d25ab
Updated•10 years ago
|
Attachment #8557465 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8557465 -
Attachment is obsolete: false
https://hg.mozilla.org/mozilla-central/rev/b6e5a79d25ab
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Updated•10 years ago
|
Attachment #8557465 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•