Closed Bug 1177765 Opened 9 years ago Closed 9 years ago

Add xmlroles for MathML

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

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".
Attached patch Part 2 - Add xmlroles for MathML (obsolete) — Splinter Review
Attachment #8626703 - Flags: review?(surkov.alexander)
Attached file Testcase
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)
I'll now convert the testcase into mochitest...
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+
Status: NEW → ASSIGNED
Attached patch Part 2 - Add xmlroles for MathML (obsolete) — Splinter Review
Attachment #8626703 - Attachment is obsolete: true
Attached patch Part 2 - Add xmlroles for MathML (obsolete) — Splinter Review
Attachment #8626794 - Attachment is obsolete: true
Attached patch Part 2 - Add xmlroles for MathML (obsolete) — Splinter Review
Attachment #8626858 - Attachment is obsolete: true
Attachment #8626859 - Attachment description: Add tests for MathML xmlroles. → Part 3 - Add tests for MathML xmlroles.
Attachment #8626859 - Flags: review?(surkov.alexander)
Attachment #8626859 - Flags: review?(surkov.alexander) → review+
Flags: needinfo?(yzenevich)
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+
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).
Attached patch Part 2 - Add xmlroles for MathML (obsolete) — Splinter Review
Attachment #8626980 - Attachment is obsolete: true
Attached patch Part 2 - Add xmlroles for MathML (obsolete) — Splinter Review
Attachment #8629398 - Attachment is obsolete: true
Attachment #8626859 - Attachment is obsolete: true
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)
We should also get Jamie's opinion
Flags: needinfo?(surkov.alexander) → needinfo?(jamie)
(In reply to alexander :surkov from comment #18)
> We should also get Jamie's opinion

OK, also asking Joanie :-)
Flags: needinfo?(jdiggs)
(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)
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.
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)
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.
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.
(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.
We may expose duping 'roles' object attribute which will be a list of roles the given accessible has.
Can I just go ahead and ask checking-needed or do you want more feedback?
Flags: needinfo?(surkov.alexander)
(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)
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
Attachment #8629919 - Attachment is obsolete: true
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.

Attachment

General

Created:
Updated:
Size: