- Notifications
You must be signed in to change notification settings - Fork294
Added seamless boolean attribute for iframe#176
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
hoppipolla-critic-bot commentedNov 24, 2014
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 donot 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This one is also missing a comma.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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. |
RitwikGupta commentedNov 1, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
No problem! Should be good to go here. Sorry for the bad commit message there 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you!
craigbarnes commentedApr 30, 2018
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