Add xmlroles for MathML

RESOLVED FIXED in Firefox 42

Status

()

Core
Disability Access APIs
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

Trunk
mozilla42
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(4 attachments, 9 obsolete attachments)

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
(Assignee)

Description

3 years ago
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

3 years ago
Created attachment 8626702 [details] [diff] [review]
Part 1 - Make nsIMathMLFrame expose the fence and separator properties of operators
Attachment #8626702 - Flags: review?(karlt)
(Assignee)

Comment 2

3 years ago
Created attachment 8626703 [details] [diff] [review]
Part 2 - Add xmlroles for MathML
Attachment #8626703 - Flags: review?(surkov.alexander)
(Assignee)

Comment 3

3 years ago
Created attachment 8626705 [details]
Testcase
(Assignee)

Comment 4

3 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

3 years ago
I'll now convert the testcase into mochitest...

Comment 6

3 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

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 7

3 years ago
Created attachment 8626794 [details] [diff] [review]
Part 2 - Add xmlroles for MathML
Attachment #8626703 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Created attachment 8626858 [details] [diff] [review]
Part 2 - Add xmlroles for MathML
Attachment #8626794 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Created attachment 8626859 [details] [diff] [review]
Part 3 - Add tests for MathML xmlroles.
(Assignee)

Comment 10

3 years ago
Created attachment 8626980 [details] [diff] [review]
Part 2 - Add xmlroles for MathML
Attachment #8626858 - Attachment is obsolete: true
(Assignee)

Updated

3 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

3 years ago
Attachment #8626859 - Flags: review?(surkov.alexander) → review+
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 12

3 years ago
Created attachment 8628213 [details] [diff] [review]
Part 1 - Make nsIMathMLFrame expose the fence and separator properties of operators
Attachment #8626702 - Attachment is obsolete: true
(Assignee)

Comment 13

3 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

3 years ago
Created attachment 8629398 [details] [diff] [review]
Part 2 - Add xmlroles for MathML
Attachment #8626980 - Attachment is obsolete: true
(Assignee)

Comment 15

3 years ago
Created attachment 8629919 [details] [diff] [review]
Part 2 - Add xmlroles for MathML
Attachment #8629398 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
Created attachment 8629920 [details] [diff] [review]
Part 3 - Add tests for MathML xmlroles.
Attachment #8626859 - Attachment is obsolete: true
(Assignee)

Comment 17

3 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

3 years ago
We should also get Jamie's opinion
Flags: needinfo?(surkov.alexander) → needinfo?(jamie)
(Assignee)

Comment 19

3 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

3 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

3 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

3 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

3 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

3 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

3 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

3 years ago
We may expose duping 'roles' object attribute which will be a list of roles the given accessible has.
(Assignee)

Comment 27

3 years ago
Can I just go ahead and ask checking-needed or do you want more feedback?
Flags: needinfo?(surkov.alexander)

Comment 28

3 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

3 years ago
Created attachment 8634682 [details] [diff] [review]
Part 1 - Make nsIMathMLFrame expose the fence and separator properties of operators

whitespace fix...
Attachment #8628213 - Attachment is obsolete: true
(Assignee)

Updated

3 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

3 years ago
Created attachment 8634684 [details] [diff] [review]
Part 2 - Add xmlroles for MathML
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
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.