Frédéric Wang (:fredw) Reporter
	Description • 9 years ago

While testing Orca MathML support on Wikipedia, I noticed that special alphabets such as fraktur are not supported. Although Orca has support for math alphanum characters when explicit Unicode values are used such as in

data:text/html;charset=UTF-8,<math><mi>&#x1D504;</mi><mi>&#x1D505;</mi></math>

the equivalent construction with the mathvariant attribute (used in Wikipedia, itex2MML etc)

data:text/html;charset=UTF-8,<math><mstyle mathvariant=fraktur"><mi>A</mi><mi>B</mi></math>

does not work.

Since we are mapping the mathvariant attribute to the CSS -moz-mathvariant property, it seems to me that the most obvious way to expose mathvariant to ATs would be map -moz-mathvariant to a text attribute, just like we do for e.g. font-family in

https://dxr.mozilla.org/mozilla-central/source/accessible/base/TextAttrs.h#289

	alexander :surkov (:asurkov)
	Comment 1 • 9 years ago

Fraktur is rather a font style (https://en.wikipedia.org/wiki/Fraktur) and probably should be exposed as a font style text attribute.

	Frédéric Wang (:fredw) Reporter
	Comment 2 • 9 years ago

I don't know the difference between "font style text attribute" or just "text attribute", but I'm fine as long as screen readers can read the mathvariant...

Here is the reference on the MathML spec:

http://www.w3.org/TR/MathML/chapter3.html#presm.commatt

mathvariant: "normal" | "bold" | "italic" | "bold-italic" | "double-struck" | "bold-fraktur" | "script" | "bold-script" | "fraktur" | "sans-serif" | "bold-sans-serif" | "sans-serif-italic" | "sans-serif-bold-italic" | "monospace" | "initial" | "tailed" | "looped" | "stretched"

default: normal (except on <mi>) 

"Specifies the logical class of the token. Note that this class is more than styling, it typically conveys semantic intent; see the discussion below."

	alexander :surkov (:asurkov)
	Comment 3 • 9 years ago

each of them should be mapped to certain attributes, probably some of them are. I'd be good to check that.

	Frédéric Wang (:fredw) Reporter
	Comment 4 • 9 years ago

So the point is that MathML's mathvariant is supposed to convey semantic and so is different from CSS style. For example <mi mathvariant="bold">x</mi> is equivalent to using the Unicode character "U+1D431 x MATHEMATICAL BOLD SMALL X" but is different from x with using style font-weight=bold.

Internally, we store the mathvariant as a CSS property with an enumeration of integer values: https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleConsts.h#605

	alexander :surkov (:asurkov)
	Comment 7 • 9 years ago

Comment on attachment 8634304 [details] [diff] [review]
Add accessibility tests for transformed text

Review of attachment 8634304 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/text/a11y.ini
@@ +9,5 @@
>  [test_hypertext.html]
>  [test_lineboundary.html]
>  [test_passwords.html]
>  [test_selection.html]
> +[test_transform.html]

you may put them into test_gettext.html, having transform.html for mathvariant may look a strange design

::: accessible/tests/mochitest/text/test_transform.html
@@ +121,5 @@
> +    <mi id="mathvariant2">x</mi>
> +    <mi id="mathvariant3" mathvariant="normal">x</mi>
> +    <mtext id="mathvariant4" mathvariant="bold">x</mtext>
> +    <mtext id="mathvariant5" mathvariant="italic">x</mtext>
> +    <mtext id="mathvariant6" mathvariant="bold-italic">x</mtext>

I would prefer more self-descriptive IDs like mathvariant_bolditalic

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 9 • 9 years ago

Comment on attachment 8634303 [details] [diff] [review]
Make nsTextFrame::GetRenderedText take mathvariant into account

Review of attachment 8634303 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/MathMLTextRunFactory.h
@@ +21,5 @@
>        mFontInflation(aFontInflation),
>        mSSTYScriptLevel(aSSTYScriptLevel) {}
>  
> +  static uint32_t MathVariant(uint32_t aCh, uint8_t aMathVar);
> +  

delete trailing whitespace

	Frédéric Wang (:fredw) Reporter
	Comment 11 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2994983b7ed

	Frédéric Wang (:fredw) Reporter
	Comment 12 • 9 years ago

GetRenderedText is used only in the accessibility code and in nsTextFrame::AccessibleType:

https://dxr.mozilla.org/mozilla-central/search?q=getrenderedtext

Attachment 8634570 [details] [diff] will work in most cases, except in the two calls of HyperTextAccessible.cpp, where we want to convert between the offsets of the original and transformed strings. I get a crash when trying to navigate inside a mathvariant char with Orca.

The current conversion code relies on gfxSkipChars & gfxSkipCharsIterator which assumes that we may skip characters (e.g. whitespace) from the original string. The problem with mathvariant is that it generally transforms a BMP char to a surrogate pairs, so this increases the length of the original string. It seems to me that we'll need to rewrite a new mechanism that handles this case, so that might be a bit more involved....

@Alex: what do you think/suggest?

	alexander :surkov (:asurkov)
	Comment 13 • 9 years ago

I don't know much about GetRenderedText implementation, but it sounds ok if you make a special case for mathvariant char conversions. 

It's good idea to ask :roc for feedback.

	Frédéric Wang (:fredw) Reporter
	Comment 14 • 9 years ago

@roc: see comment 12. Any suggestion for a proper way to manage these original/transformed strings and the corresponding offset conversion, so that we can expose that to Assistive Technologies?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 15 • 9 years ago

I suggest we extend GetRenderedText to also return an array mapping offsets in the output string to offsets in the DOM string.

	Frédéric Wang (:fredw) Reporter
	Comment 16 • 9 years ago

So reading the code again, the two HyperTextAccessible functions do not actually cache the gfxSkipChars/ gfxSkipCharsIterator output parameters and only use them to map from the in-offset argument to the out-offset argument. So we probably actually do not need the skipchars classes or even an array, here. Also, aSkippedStartOffset is always zero and aSkippedMaxLength is only set to 1 to test whether the rendered string is nonempty.

So I'm suggesting to do something like

GetRenderedText(nsAString* aRenderedText, bool* aIsNonEmpty = null, bool aContentToRendered = true, uint32_t aInOffset = 0, uint32_t* aOutOffset = nullptr);

and there are three possibilities:

- if aRenderedText != nullptr, this will convert the returned the rendered text.
- if aIsNonEmpty != nullptr this will returned whether the rendered string is nonempty
- if aOutOffset != nullptr this will aInOffset to aOutOffset using aContentToRendered to decide whether it's content=>rendered or rendered=>content.

Thoughts?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 17 • 9 years ago

That interface looks a bit too complicated. I think it would be simpler to just output the array.

	Frédéric Wang (:fredw) Reporter
	Comment 18 • 9 years ago

WIP progress patch following roc's suggestion.

This seems to return the correct offsets in general but there are some tricky cases with whitespace that would need to be debugged more carefully.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ff76b0cb8d4

	Frédéric Wang (:fredw) Reporter
	Comment 19 • 9 years ago

I'm unassigning myself until I find time to come back to this.

BTW, I forgot to say that the crash mentioned in comment 12 no longer happens and Orca correctly reads https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/a11y#mathvariant

	Trevor Saunders (:tbsaunde)
	Comment 20 • 9 years ago

(In reply to Frédéric Wang (:fredw) from comment #18)
> Created attachment 8637242 [details] [diff] [review]
> Make nsTextFrame::GetRenderedText take mathvariant into account - WIP
> 
> WIP progress patch following roc's suggestion.
> 
> This seems to return the correct offsets in general but there are some
> tricky cases with whitespace that would need to be debugged more carefully.

yeah, whitespace is already kind of buggy.  I tried to write some tests to  check what our current behavior is.  That never got committed, but the test is still in bug 1080870.  The results are pretty strange!