Closed
Bug 1177765
Opened 9 years ago
Closed 9 years ago
Add xmlroles for MathML
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: fredw, Assigned: fredw)
References
Details
Attachments
(4 files, 9 obsolete files)
1.58 KB,
text/html
|
Details | |
6.93 KB,
patch
|
Details | Diff | Splinter Review | |
3.37 KB,
patch
|
Details | Diff | Splinter Review | |
9.61 KB,
patch
|
Details | Diff | Splinter Review |
In order to help AccessFu to read MathML formulas, we would need some xmlroles for MathML elements based on its position inside its parent container. For example, in <math> <msup> <mi>x</mi> <mfrac><mi>a</mi><mi>b</mi></mfrac> </msup> </mfrac> The <mfrac> has role "fraction" but because it is the second child of the <msup> it would also have xmlrole "mathsuperscript".
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8626702 -
Flags: review?(karlt)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8626703 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
The attached patches provide new xmlroles for MathML elements. See the attached testcase for details. Yura: can you give them a try with AccessFu?
Flags: needinfo?(yzenevich)
Assignee | ||
Comment 5•9 years ago
|
||
I'll now convert the testcase into mochitest...
Comment 6•9 years ago
|
||
Comment on attachment 8626703 [details] [diff] [review] Part 2 - Add xmlroles for MathML Review of attachment 8626703 [details] [diff] [review]: ----------------------------------------------------------------- and pls add some tests ::: accessible/generic/HyperTextAccessible.cpp @@ +979,5 @@ > > if (HasOwnContent()) > GetAccService()->MarkupAttributes(mContent, attributes); > > + // Add MathML xmlroles based on the position inside the parent. pls move it to separate method and check mathml namespace before calling into it @@ +1002,5 @@ > + mathMLFrame->GetEmbellishData(embellishData); > + if (NS_MATHML_EMBELLISH_IS_FENCE(embellishData.flags)) { > + if (!PrevSibling()) { > + nsAccUtils::SetAccAttr(attributes, nsGkAtoms::xmlroles, > + nsGkAtoms::math_fence_open); do you need math- prefix? @@ +1074,5 @@ > + } break; > + case roles::MATHML_MULTISCRIPTS: { > + // Get the <multiscripts> base. > + nsIFrame* child = parent->GetFrame()->GetFirstPrincipalChild(); > + if (child) { pls traverse DOM
Attachment #8626703 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8626703 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8626794 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8626858 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8626859 -
Attachment description: Add tests for MathML xmlroles. → Part 3 - Add tests for MathML xmlroles.
Attachment #8626859 -
Flags: review?(surkov.alexander)
Updated•9 years ago
|
Attachment #8626859 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(yzenevich)
Comment 11•9 years ago
|
||
Comment on attachment 8626702 [details] [diff] [review] Part 1 - Make nsIMathMLFrame expose the fence and separator properties of operators >+// This bit is set if the core is a fence >+#define NS_MATHML_EMBELLISH_FENCE 0x00000020 >+ >+// This bit is set if the core is a separator >+#define NS_MATHML_EMBELLISH_SEPARATOR 0x00000040 IIUC only flags set in the (!mEmbellishData.coreFrame) block of ProcessOperatorData() will be propagated up the embellished hierarchy through TransmitAutomaticData. That means that NS_MATHML_EMBELLISH_FENCE and NS_MATHML_EMBELLISH_SEPARATOR are only set on the operator itself, which is quite different from NS_MATHML_EMBELLISH_OPERATOR/ACCENT/MOVABLELIMITS which are transmitted throughout the hierarchy. Therefore, please clarify this distinction in the comment. Something like "This bit is set on the core if it is a fence operator." >+ // Expose these properties so that they can be used by the accessibility code. >+ if (NS_MATHML_OPERATOR_IS_FENCE(mFlags)) >+ mEmbellishData.flags |= NS_MATHML_EMBELLISH_FENCE; >+ if (NS_MATHML_OPERATOR_IS_SEPARATOR(mFlags)) >+ mEmbellishData.flags |= NS_MATHML_EMBELLISH_SEPARATOR; Would you be happy to move these blocks to the existing if (NS_MATHML_OPERATOR_IS_FENCE(mFlags)) and if (NS_MATHML_OPERATOR_IS_SEPARATOR(mFlags)) blocks above to keep fence and separator handling in one place? That can be done by adding else blocks to the if (value.EqualsLiteral("false")) blocks.
Attachment #8626702 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8626702 -
Attachment is obsolete: true
Assignee | ||
Comment 13•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5399c8e9f856 I'll wait before asking checking-needed as the use of xml-roles seems controversial. Instead, this can either be in a separate "math-roles" object attribute, done as proposed in bug 1177601 or just by adding MathML relations (bug 1001635).
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8626980 -
Attachment is obsolete: true
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8629398 -
Attachment is obsolete: true
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8626859 -
Attachment is obsolete: true
Assignee | ||
Comment 17•9 years ago
|
||
Alex: what do you think about the options of comment 13? If using xmlroles for MathML roles is controversial, I'm tempted to choose the easiest option to create a separate "math-roles" object attribute.
Flags: needinfo?(surkov.alexander)
Comment 18•9 years ago
|
||
We should also get Jamie's opinion
Flags: needinfo?(surkov.alexander) → needinfo?(jamie)
Assignee | ||
Comment 19•9 years ago
|
||
(In reply to alexander :surkov from comment #18) > We should also get Jamie's opinion OK, also asking Joanie :-)
Flags: needinfo?(jdiggs)
Comment 20•9 years ago
|
||
(In reply to Frédéric Wang (:fredw) from comment #19) > (In reply to alexander :surkov from comment #18) > > We should also get Jamie's opinion > > OK, also asking Joanie :-) The little voice inside my head keeps saying "meh." But I think we want more input, so I've just emailed the W3C public-pfwg list asking for comments: https://lists.w3.org/Archives/Public/public-pfwg/2015Jul/0051.html. The little voice inside my head will live with whatever consensus is achieved there. :)
Flags: needinfo?(jdiggs)
Assignee | ||
Comment 21•9 years ago
|
||
Thanks Joanie. Bug 1163374 which adds basic support for AccessFu is now ready to land but is still blocked by this backend change. At the moment, MathML is just not accessible at all on mobile platforms, which I think is bad, so I'd like to get the patches landed ASAP. Since we are still not really sure about what will be best for Desktop (XML roles, MathML relations...?) I suggest to use a "math-roles" object attribute which will only be used for AccessFu for now. I think that's the easiest way to adapt the current patches, without introducing controversial new "standard" API stuff.
Comment 22•9 years ago
|
||
For me, it goes something like this: 1. xml-roles has always been an ugly hack. IMO, all widget roles should have proper roles in the platform APIs and landmarks should have a separate landmark attribute (I never understood why they were mapped to ARIA role in the first place). Any new concept beyond this needs a new concept in the APIs, whether that be a role, state or attribute. 2. But xml-roles is here to stay. 3. Checking for n relations is slow. 4. Checking for two object attributes is not as slow as (3), but it still sucks. 5. Given (1) (xml-roles is already ugly), I guess I don't think it's worth arguing about making it uglier by adding math roles into the mix.
Flags: needinfo?(jamie)
Assignee | ||
Comment 23•9 years ago
|
||
Thanks, Jamie. So for completeness, the NSAccessibility implementation needs the relation in the other direction: from an object with fraction role, gets the child with numerator or denominator math role. I think adding MathML relations (bug 1001635) will need more work and it's still not clear whether any consumer wants them. Parsing an object attribute from the Objective C code will also be more code/work I suspect so for now, I'm happy with just checking the child position at it is currently done. So if I do not hear strong complaints in the upcoming days, I'll ask to land the patches using the xml-roles attribute or, if necessary, a new "math-role" attribute. So that we at least have something on mobile.
Comment 24•9 years ago
|
||
I'm good to have a special way to expose landmarks, for example, via new 'landmark' object attribute, but I think I would put all roles on the accessible into 'roles' object attribute, perhaps, including landmarks too.
Comment 25•9 years ago
|
||
(In reply to alexander :surkov from comment #24) > I'm good to have a special way to expose landmarks, for example, via new > 'landmark' object attribute, but I think I would put all roles on the > accessible into 'roles' object attribute, perhaps, including landmarks too. FWIW, I think this bird has already flown; everyone is using xml-roles now. I argued about this years ago and lost, and it's probably not worth changing it now. I was just pointing out that there's probably no harm in overloading xml-roles further given that it's already ugly.
Comment 26•9 years ago
|
||
We may expose duping 'roles' object attribute which will be a list of roles the given accessible has.
Assignee | ||
Comment 27•9 years ago
|
||
Can I just go ahead and ask checking-needed or do you want more feedback?
Flags: needinfo?(surkov.alexander)
Comment 28•9 years ago
|
||
(In reply to Frédéric Wang (:fredw) from comment #27) > Can I just go ahead and ask checking-needed or do you want more feedback? xml-roles is fine for now I think, so yes as long as review comments are addressed. btw, you have bunch of whitespaces in the patch, they should go away.
Flags: needinfo?(surkov.alexander)
Assignee | ||
Comment 29•9 years ago
|
||
whitespace fix...
Attachment #8628213 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8634682 -
Attachment description: Make nsIMathMLFrame expose the fence and separator properties of operators → Part 1 - Make nsIMathMLFrame expose the fence and separator properties of operators
Assignee | ||
Comment 30•9 years ago
|
||
Attachment #8629919 -
Attachment is obsolete: true
Assignee | ||
Comment 31•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=858751cadf60
Keywords: checkin-needed
Comment 32•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb8bd6d912e4 https://hg.mozilla.org/integration/mozilla-inbound/rev/e200084c5dcb https://hg.mozilla.org/integration/mozilla-inbound/rev/6718b3da1e8a
Keywords: checkin-needed
Comment 33•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fb8bd6d912e4 https://hg.mozilla.org/mozilla-central/rev/e200084c5dcb https://hg.mozilla.org/mozilla-central/rev/6718b3da1e8a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•