Skip to content

Conversation

darobin
Copy link

@darobin darobin commented Dec 9, 2013

As per http://darobin.github.io/html-ruby/. This change is being landed in the W3C specification, apply with caution.

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/457

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@gsnedders
Copy link
Member

There are tests in html5lib/html5lib-tests#27.

Choose a reason for hiding this comment

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

I think you'll need to change this line to reflect recent bug fix at W3C bug 26074.

aYukiSekiguchi and others added 2 commits August 12, 2014 14:24
Update parser to follow HTML5 spec change at:
w3c/html@c61397b
@darobin
Copy link
Author

darobin commented Sep 1, 2014

So, the tests in html5lib/html5lib-tests#27 have now landed; I guess this might be ready for acceptance?

@gsnedders
Copy link
Member

We need to do something about having tests pass before anything else lands (expected failures or something need to be implemented given we won't always be in-sync with html5lib-tests…), meh. :(

@kojiishi
Copy link

hm, not sure why it fails so many, maybe I don't understand how to read the result, but one thing I can tell is that one of the failures is because a pull request for test expectation file update is still pending.
@darobin can you look at this pull request?

@gsnedders
Copy link
Member

Most of them are broken because master is broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong. You need two distinct groups. Or you need to also branch the self.tree.openElements[-1].name != "ruby" line further. What you're doing now doesn't match other implementations.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure that this PR is up to date with the more recent changes to the spec anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't.

@gsnedders
Copy link
Member

I'm just going to close this as outdated and deal with this with the larger spec update.

@gsnedders gsnedders closed this May 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants