Added seamless boolean attribute for iframe #176
Conversation
|
Critic review: https://critic.hoppipolla.co.uk/r/3275 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 |
|
@RitwikGupta I apologize that this has been sitting around for so long. Can you take some time to update it? If not, let me know and I can do that. Thank you! |
|
That should resolve the merge conflicts. My CHANGES commit may be incorrect, sorry about that! Unsure of the format. |
|
This is failing for Python 3.4 test runs with weirdo errors. I'm going to try some Travis fiddling and if that doesn't work, look into it more carefully. |
html5lib/constants.py
Outdated
| "input": frozenset(("disabled", "readonly", "required", "autofocus", "checked", "ismap")), | ||
| "select": frozenset(("disabled", "readonly", "autofocus", "multiple")), | ||
| "output": frozenset(("disabled", "readonly")), | ||
| "iframe": frozenset(("seamless")), |
There was a problem hiding this comment.
Two things:
First, you changed all the lists to tuples. I'm not sure why you did that. One problem tuples has is that in order to make something a one-item tuple, you need a comma. Forgetting it causes problems ...
Which brings me to the second thing--this one is missing that comma. So this is creating a frozenset of ['s', 'e', 'a', 'm', 'l', 'e', 's', 's'] which is not what we want.
Can you fix this? I'd also prefer it if you switched these back to lists.
There was a problem hiding this comment.
We switched them all over to lists ages ago to avoid the missing-comma issue.
html5lib/constants.py
Outdated
| "details": frozenset(("open",)), | ||
| "datagrid": frozenset(("multiple", "disabled")), | ||
| "command": frozenset(("hidden", "disabled", "checked", "default")), | ||
| "hr": frozenset(("noshade")), |
There was a problem hiding this comment.
This one is also missing a comma.
There was a problem hiding this comment.
This is a regression, and why we moved to lists. Presumably just copy/paste from the old branch?
There was a problem hiding this comment.
This was my fault, I chose the wrong revision when fixing the merge conflicts. Will fix!
|
Travis fiddling didn't help. We're hitting this issue here: That issue is unrelated to this PR, though, so once this PR is set to go, I'll merge it. @RitwikGupta If you could fix that comma issue, that'd be super. Thank you for working on this! |
|
I switched over to the lists. I omitted the comma here as stated by @gsnedders, so this should be good to go. |
|
Everything looks good, except there's a new merge conflict. @RitwikGupta Can you fix the merge conflict? If not, I can get to it. I'm really sorry about this. |
|
No problem! Should be good to go here. Sorry for the bad commit message there 😄 |
|
It seems this attribute has been removed from the spec. It was never supported in Firefox or IE and has been removed from Chrome. |
Followed guidelines (I think) for the CHANGES and AUTHORS file