Skip to content

Add scrolling attribute for <iframe>#1325

Merged
zpao merged 1 commit intofacebook:masterfrom
venmo:add-scrolling-attribute
Apr 3, 2014
Merged

Add scrolling attribute for <iframe>#1325
zpao merged 1 commit intofacebook:masterfrom
venmo:add-scrolling-attribute

Conversation

@thomasboyt
Copy link
Copy Markdown

The scrolling attribute was dropped from HTML5 in favor of just setting the overflow property, but unfortunately Chrome doesn't support overflow on iframes yet. This adds scrolling to the list of supported DOM properties so that it can be used when rendering an iframe from React.

I'm not really sure how MUST_USE_PROPERTY or MUST_USE_ATTRIBUTE work, will be happy to set this to one or the other if needed.

@zpao
Copy link
Copy Markdown
Member

zpao commented Apr 1, 2014

Based on that bug, this will likely not be fixed in blink for a couple years :/

MUST_USE_PROPERTY and MUST_USE_ATTRIBUTE effect how changes are applied. For properties, we just do node.scrolling = value, for attributes we use `node.setAttribute('scrolling', value). When unspecified it means both should work (because the DOM is super consistent, lol) and we'll use whichever we think is faster. Currently that's property access. We don't have automated tests for these yet, so the best thing to do is actually use the component and property (I usually modify an example), and make sure when you change the value in render, the DOM is correctly changed.

@zpao
Copy link
Copy Markdown
Member

zpao commented Apr 1, 2014

Also, can you sign the CLA? https://code.facebook.com/cla

@zpao zpao self-assigned this Apr 1, 2014
@thomasboyt
Copy link
Copy Markdown
Author

Signed!

@zpao zpao added CLA signed and removed CLA needed labels Apr 2, 2014
@zpao
Copy link
Copy Markdown
Member

zpao commented Apr 2, 2014

Pulling in now. I confirmed that both node.scrolling = value and node.setAttribute('scrolling', value) work so there's no need to specify.

zpao added a commit that referenced this pull request Apr 3, 2014
Add scrolling attribute for <iframe>
@zpao zpao merged commit 00aa334 into facebook:master Apr 3, 2014
@zpao
Copy link
Copy Markdown
Member

zpao commented Apr 3, 2014

Thanks!

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.

2 participants