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

Add MathML support #12836

Merged
merged 56 commits into from
Jan 23, 2018
Merged

Conversation

adamsilverstein
Copy link
Contributor

@adamsilverstein adamsilverstein commented Jan 16, 2018

Implements MathML support via an Iframe and the CDN hosted MathJax script. Fixes #12800

  • resizes the iframe height after the math formula renders
  • ideally the iframes should also scale their width so smaller formulas could be embedded inline.

Includes a few example of formulas, I wasn't sure how to make these small to begin with, in my testing they started large then resized smaller when the formula rendered.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@adamsilverstein
Copy link
Contributor Author

Hey @googlebot - I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@@ -0,0 +1,69 @@
#
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at Contributing Validator Rules. The format for allowing the script tag has been simplified greatly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will do - thanks for the feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the extensions/amp-mathml/validator-amp-mathml.protoascii file in f6d93f3 - I still need to add some valid and invalid test cases

Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamsilverstein Thanks so much for contributing this. Great initial PR. Left bunch of comments.

Reading resizing, we only resize with and AMP components are block elements by default. User would need to use flex or other parent CSS ways to make this inline. We won't do resizing for width.

3p/mathml.js Outdated
@@ -0,0 +1,68 @@
/**
* Copyright 2017 The AMP HTML Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2018 ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yea; sorry, i started by copying an existing component, I'll clean this up.

3p/mathml.js Outdated
* See the License for the specific language governing permissions and
* limitations under the License.
*/
console.log( 'mathml.js' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't forget to remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:doh:

3p/mathml.js Outdated
import {user} from '../src/log';

/**
* Get the correct script for the gist.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the mathml

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, thanks

3p/mathml.js Outdated
delete data.height;
const div = document.createElement('div');
div.setAttribute('id','mathmlformula');
div.innerHTML = data.formula;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must be div.textContent = otherwise this crease an XSS vulnerability given formula is not sanitized. textContent should be fine for Latex and Ascii Math. Worht testing for MathML, if it does not work, we unfortunately need to remove support for MathML as input and stick to LaText and ASCII Math since we don't have enough utilities to safely sanitize mathml input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, ok i'll change and do some testing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far my test cases look fine after the change to textContent, I'll add some more samples from https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/MathML_Torture_Test to my example page

3p/mathml.js Outdated
const div = document.createElement('div');
div.setAttribute('id','mathmlformula');
div.innerHTML = data.formula;
document.body.appendChild(div);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

global.document.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will change - what is the context/reason behind global.?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may not be the current window in certain situations where an AMP doc is embedded differently.

3p/mathml.js Outdated
document.body.appendChild(div);
MathJax.Hub.Queue( function () {
const rendered = document.getElementById('MathJax-Element-1-Frame');
context.requestResize(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateDimensions is okay in this case ( requestResize is not guaranteed to resize). We make exceptions for updateDimensions per component and this should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also mathml needs to be added to updateDimensionsEnabled_() method in ampcontext-integration.js file for it to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, will add

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aghassemi when adding this I noticed on a line I was touching that the check for 'github' isn't using a strict equality check, see https://github.com/ampproject/amphtml/blob/master/3p/ampcontext-integration.js#L72

This should be ===, right? Should I open a separate pull request for this issue? I don't want to introduce unrelated changes here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our JS compiler handles discrepencies between == and === so it is fine in practices, would be nice for consistency, you can change it here in this PR

/** @override */
buildCallback() {
const formula = this.element.getAttribute('formula');
if (!formula || '' === formula) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's have

  user().assert(
       data.formula,
       'The formula attribute is required for <amp-mathml> %s',
       data.element);

here instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that bit is in mathml.js, i removed the buildCallback entirely, it seems superfluous

super(element);

/** @private {!Element} */
this.container_ = this.win.document.createElement('div');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not used, remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -0,0 +1,27 @@
<!doctype html>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i have an uncommitted out file that i think was generated when i started, I'll check that and commit it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the file, but I am still missing something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this file to extensions/amp-mathml/0.1/test/validator-amp-mathml.html. The out file will be extensions/amp-mathml/0.1/test/validator-amp-mathml.out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here for an example extensions test directory with an .html and .out.

yarn.lock Outdated
@@ -178,6 +178,12 @@ acorn-globals@4.1.0, acorn-globals@^4.0.0:
dependencies:
acorn "^5.0.0"

acorn-globals@^3.1.0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert this file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will do

@adamsilverstein
Copy link
Contributor Author

@aghassemi thanks for the careful review and detailed feedback. I will work on all of the points raised and get back to you when I am ready for another review.

I still have a question about the resizing: is it possible to get the iframe to resize its width as well as its height, currently my listener only changes height. any tips to getting this working?

@adamsilverstein
Copy link
Contributor Author

adamsilverstein commented Jan 22, 2018

@aghassemi You are very welcome, thanks for your assistance along the way. Adding tests later sounds good, please tag me in the PR as I would like to see and learn from how you add these.

I fixed the linting errors and added the OWNERS.yaml file as well as changing the tagname to allcaps as suggested by @honeybadgerdontcare.

Did you see my question above about clearing the valtador errors (#12836 (comment)) - unless the allcaps change in 6fdb6e4 fixes them, I'm not sure how to correct those. On a related note, I'm blocked from running gulp validator locally - when I do, I hit an error I haven't been able to resolve: google.protobuf.descriptor not found. Try "apt-get install python-protobuf" - any suggestions for getting around that, I'm on the latest mac osx (and I already installed protobuf with brew).

@adamsilverstein
Copy link
Contributor Author

@aghassemi I think I have fixed all the warnings other than the validator messages I mentioned above that I am not sure how to clear. Please let me know how to fix or just go for it and get the validator passing.

Thanks!

@adamsilverstein
Copy link
Contributor Author

missed the earlier comment from @honeybadgerdontcare - move the test and out files into the test folder in 98d0039

@honeybadgerdontcare
Copy link
Contributor

@adamsilverstein it looks like you're still missing the .out file. If this is inconvenient please open an issue assigned to me with your current .html test file and I'll take care of the rest. That will get you around any travis/test issues.

Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve protoascii changes. Test file changes can come later.

Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve protoascii changes. Test file changes can come later.

@adamsilverstein
Copy link
Contributor Author

it looks like you're still missing the .out file. If this is inconvenient please open an issue assigned to me with your current .html test file and I'll take care of the rest. That will get you around any travis/test issues.

@honeybadgerdontcare Thanks for your patience with me as I'm learning the ropes around here :)

I thought this was the correct location for the .out file? https://github.com/ampproject/amphtml/pull/12836/files#diff-467d44e03597d8a2c58c30d30be27e60

I'll go ahead and open an issue as you suggested and let you take care of the rest, thank you!

@adamsilverstein
Copy link
Contributor Author

@honeybadgerdontcare I created an issue but don't have the capability to assign it to you: #12973

@honeybadgerdontcare
Copy link
Contributor

I've taken the issue. The problem, or at least the easiest to see, is with the out file. It doesn't include the actual .html contents. See this as an example. Regardless, I can take care of it for you in a separate PR. So remove the test html and out files from this PR.

@adamsilverstein
Copy link
Contributor Author

@honeybadgerdontcare thanks so much for your assistance, I removed the test files in 85bf039.

@adamsilverstein
Copy link
Contributor Author

@aghassemi all checks have passed :)

@aghassemi aghassemi merged commit 33ae37a into ampproject:master Jan 23, 2018
@aghassemi
Copy link
Contributor

@adamsilverstein merged! I am super happy to see large contributions like this from the community. Thanks again Adam.

/cc @mrjoro

@adamsilverstein
Copy link
Contributor Author

Wooohooo! Thanks for all your shepherding @aghassemi I learned a ton about AMP along the way, and this feature will be a big improvement for our client's site.

How long does it take for this feature to make it to production/the wild?

@aghassemi
Copy link
Contributor

@adamsilverstein For production use, ~3 weeks (Validator changes take a bit to make it to prod. A comment will be added to this PR when validation rules are in prod). Folks can start experimenting with it in next Dev Channel (tomorrow) however.

Gregable pushed a commit that referenced this pull request Jan 24, 2018
Gregable added a commit that referenced this pull request Jan 24, 2018
* Add an amp-video test that shows which tagspec is picked for errors.

* Revision bump for #12654

* Revision bump for #12836

* Make sure the light validator doesn't run amp4ads tests.
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
* Add an amp-video test that shows which tagspec is picked for errors.

* Revision bump for ampproject#12654

* Revision bump for ampproject#12836

* Make sure the light validator doesn't run amp4ads tests.
RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018
RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018
* Add an amp-video test that shows which tagspec is picked for errors.

* Revision bump for ampproject#12654

* Revision bump for ampproject#12836

* Make sure the light validator doesn't run amp4ads tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants