Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support math in Chrome #7184

Merged
merged 2 commits into from Jun 9, 2017
Merged

Support math in Chrome #7184

merged 2 commits into from Jun 9, 2017

Conversation

@jcsteh
Copy link
Contributor

@jcsteh jcsteh commented May 18, 2017

Link to issue number:

No specific issue. However, lack of support for math in Chrome was mentioned in #5555 (comment). Chromium issue 426650 is the issue I filed against Chrome requesting the bits we needed to make this work.

Summary of the issue:

Math is currently supported in Firefox and IE, but not Chrome. This PR makes it work in Chrome.

Description of how this pull request fixes the issue:

Now that the Chrome issue linked above is fixed (and has been for a few months), we can use the same math retrieval code we use for Firefox. Since both Firefox and Chrome use NVDAObjects.IAccessible.ia2Web, I moved the math code from NVDAObjects.IAccessible.mozilla into ia2Web. In addition to moving the code, we also catch COMError because this isn't yet in Chrome stable and because this can fail in Firefox too if the ISimpleDOM COM proxy isn't registered.

Testing performed:

Tested the following test case in Firefox nightly (55.0a1 (2017-05-16)), Chrome canary (60.0.3102.0) and Chrome stable (58.0.3029.110):
data:text/html,<math><mfrac><mn>2</mn><mi>x</mi></mfrac></math>text
In Firefox and Chrome stable, I got "2 over x text" as expected.
In Chrome stable, I got just "text", which is expected because math isn't supported there.

Known issues with pull request:

Math isn't supported in Chrome stable at present, but we handle it gracefully and this will just start working once Chrome 59 is released and users are updated. Chrome 59 is due on 6 June, which is more than two months before NVDA 2017.3 will be released, so I didn't see any need to mention the Chrome version number in the User Guide.

Change log entry:

In New Features:

- Mathematical content (provided as MathML) is now supported in Google Chrome. (#7184)
In Chrome, we use exactly the same code to retrieve MathML as we do in Firefox, so move the math code from NVDAObjects.IAccessible.mozilla into NVDAObjects.IAccessible.ia2Web (which is used by both Firefox and Chrome). We also catch COMError because this isn't yet in Chrome stable and because this can fail in Firefox too if the ISimpleDOM COM proxy isn't registered.
Copy link
Member

@feerrenrut feerrenrut left a comment

Overall this looks good, and the description on this PR is excellent!

try:
node = self.IAccessibleObject.QueryInterface(ISimpleDOMNode)
# Try the data-mathml attribute.
attr = node.attributesForNames(1, (BSTR * 1)("data-mathml"), (c_short * 1)(0,))

This comment has been minimized.

@feerrenrut

feerrenrut May 25, 2017
Member

Honestly, I dont understand this line.

This comment has been minimized.

@jcsteh

jcsteh May 25, 2017
Author Contributor

It's partly knowing the ISimpleDOMNode interface (for which there is an idl) and partly knowing ctypes/comtypes. attributesForNames can retrieve multiple attributes. It takes three arguments: an attribute count, an array of BSTRs specifying the attribute names and an array of shorts specifying the namespace id for each attribute (which for our purposes is 0). Because comtypes doesn't quite understand that we're dealing with arrays here, we need to explicitly create the arrays. BSTR * 1 creates a type for an array of BSTR with length 1 (akin to writing BSTR foo[1] in C++). Similar situation for c_short * 1; it's akin to short foo[1] in C++.

I guess I could simplify this by defining these as variables first, but it seemed like a pointless waste of variables at the time. The question is whether this is just something that requires knowledge of ctypes/comtypes. Does the following actually make it understandable to you? If so, I'm happy to change it.

attrNames = (BSTR * 1)("data-mathml")
namespaceIds = (c_short * 1)(0)
attrVal = node.attributesForNames(1, attrNames, namespaceIds)

This comment has been minimized.

@feerrenrut

feerrenrut May 25, 2017
Member

I think it makes it slightly better, in that I know what those bits are supposed to represent. I wasn't aware of the type * 1 syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants