Skip to content

add picture element and related attributes #1848

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

Merged
merged 3 commits into from
Sep 8, 2014
Merged

Conversation

fgnass
Copy link
Contributor

@fgnass fgnass commented Jul 16, 2014

This PR adds support for the picture element as well as the related media and sizes attributes. The picture element recently landed in Chrome 37 (behind a flag) and can be polyfilled in a wide range of browsers using picturefill.

@zpao
Copy link
Member

zpao commented Jul 16, 2014

Nice. We'll also want to make sure we transform <picture> so we'll need an entry here for now: https://github.com/facebook/react/blob/master/vendor/fbtransform/transforms/xjs.js#L111

Can media and sizes be set with node.media = newValue? Or do we need to use node.setAttribute('media', newValue)? Can you just confirm that you've tested modifying a picture element and seen the changes reflected in the DOM

@syranide
Copy link
Contributor

It's enough to open the console and do 'sizes' in document.createElement('picture') and same for media.

PS. Also, the answer seems to be no, they're attributes only.

@fgnass
Copy link
Contributor Author

fgnass commented Jul 16, 2014

Whether media and sizes can be set as properties depends on the browser. In future browsers that natively support the picture element these properties should be available. Until this is widely supported we have to resort to using attributes.

Chrome stable (which supports srcset but not <picture>) for example will issue the following warning when you try to access the media property of a <source> element:

'HTMLSourceElement.media' is deprecated. This attribute doesn't do anything.

I guess the property will be de-deprecated when picture-support lands in Chrome.

@longlho
Copy link

longlho commented Jul 25, 2014

+1 for this :)

@syranide
Copy link
Contributor

syranide commented Aug 2, 2014

@zpao Btw, it should've been asked before srcSet was added I guess. But before we go ahead with this and solidify it even more; shouldn't srcSet be handled the same way as style and "soon" classList? I.e, shouldn't it only accept Set (and perhaps Array as we are lenient with style right now) as opposed to a string.

Or perhaps it shouldn't because because http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/#additions-to-the-img-element says it's a DOMString, which is kind of weird? It would seem intuitive to treat it like a Set.

@zpao
Copy link
Member

zpao commented Aug 6, 2014

@syranide - not a bad idea, might be good to evolve to that. For the time being though, let's keep it as a string since that's the only API in the DOM (style, classList are exposed on a more granular level).

@fgnass - if these can only be set/get via attributes, then we need to make sure we specify MUST_USE_ATTRIBUTE in the config. Please test a React example using your changes in a browser that supports <picture>.

@fgnass
Copy link
Contributor Author

fgnass commented Aug 20, 2014

@zpao here is a gist that uses the original version (without MUST_USE_ATTRIBUTE) that works as expected in Chrome Canary. There is a button at the end of the page that loads the picturefill script. Once picturefill is loaded the example also works in browsers without native picture support.

@zpao
Copy link
Member

zpao commented Aug 21, 2014

@fgnass Did you test changing any of these attribute values? That's the real test. Initial render doesn't go through the path the needs to set node.property or node.setAttribute(), it only makes some html that's used for innerHTML.

@fgnass
Copy link
Contributor Author

fgnass commented Aug 21, 2014

@zpao In Canary everything works fine. In a polyfilled Chrome (stable) changing img.srcSet works but source.srcSet doesn't. Same for source.media.

I guess the most sane behaviour would be if React used the property if it exisited, e.g. 'srcset' in document.createElement('source') and the attribute otherwise. On the other hand it seems to be safe to always use the attribute, even in browsers with native support.

So I'd suggest to use MUST_USE_ATTRIBUTE for all the three props: srcSet, sizes and media. Should I update the PR? And if so, should I also rebase to 0.11.x as the configuration is now done in a different file?

@zpao
Copy link
Member

zpao commented Aug 21, 2014

if attributes work everywhere let's just do that.

And there's no need to rebase (unless you want to rebase against master). All the files should still be in the same place...

@fgnass
Copy link
Contributor Author

fgnass commented Aug 21, 2014

Done. And sorry for my confusion – I thought I did the change in DefaultDOMPropertyConfig.js but just realised that it was already in HTMLDOMPropertyConfig.js :)

@syranide
Copy link
Contributor

@zpao Hmm, an idea would be to have a CAN_USE_PROPERTY flag, it would check that the property exists for a specific tagName else fall back to use attribute. This would allow properties to be used when available and when browsers lack support it uses attributes (so polyfills/etc still work, etc). I'm guessing this flag could be applied pretty much across the board... #1512? :)

@zpao zpao mentioned this pull request Sep 3, 2014
4 tasks
@zpao
Copy link
Member

zpao commented Sep 3, 2014

@syranide Keep those ideas in the discussions they belong, otherwise they get lost :)

zpao added a commit that referenced this pull request Sep 8, 2014
add picture element and related attributes
@zpao zpao merged commit 79ca0c7 into facebook:master Sep 8, 2014
zpao added a commit that referenced this pull request Sep 16, 2014
add picture element and related attributes
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.

4 participants