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

Provide better HTML5 support and add workaround for line numbers #68

Closed
sidkshatriya opened this issue May 24, 2016 · 2 comments
Closed
Assignees
Labels

Comments

@sidkshatriya
Copy link
Contributor

sidkshatriya commented May 24, 2016

While working on #62 I encountered problems related to the AMP library's support for HTML5. The main issue is that we're using the inbuilt HTML parser of PHP DOM (libxml version is 2.9.0).

The following does not parse properly for instance:

<audio controls autoplay>
    <p>Your browser does not support the <strong>audio</strong> tag</p>
    <source src="abc.wav" type="audio/wav">
    <source src="xyz.mp3" type="audio/mpeg">
</audio>

(The above HTML actually parses without any fatal errors but the parse tree is not correct).

This HTML needs to be changed to:

<audio controls autoplay>
    <p>Your browser does not support the <strong>audio</strong> tag</p>
    <source src="abc.wav" type="audio/wav" />
    <source src="xyz.mp3" type="audio/mpeg" />
</audio>

Notice that <source> was changed to <source /> so that the PHP dom parser knows that the tag has ended.

We can't expect users to do this while entering HTML. Users should be able to provide valid HTML5 in additional to traditional HTML.

The solution is to not use the default parser but the mastermind/html5-php library to parse the HTML5 to build the DOMDocument. (We're already using this library for HTML output)

The use of mastermind for input is a seemingly small change but causes a huge problem: We cannot point out line numbers when we find validation issues. DOMNode::getLineNo() always returns zero.

I filed an issue on the mastermind/html5-php. Sadly there has been no response on it (yet).

I have been able to make my own workaround though (See filed issue for workaround details).

Task for this ticket:

  • Make the changes in the AMP Library to use the mastermind/html5-php library for input HTML parsing. Add workaround for line numbers.
@sidkshatriya sidkshatriya self-assigned this May 24, 2016
This was referenced May 24, 2016
@sidkshatriya
Copy link
Contributor Author

This is quite an intrusive change though its basically done.

I wont be merging this to master yet. I want to do more testing and make sure things are robust enough.

@sidkshatriya
Copy link
Contributor Author

Merged to master. Closing.

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

No branches or pull requests

1 participant