Bug 870021 (srcset)

Implement `srcset` attribute on `img`

RESOLVED FIXED in mozilla32

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
6 months ago

People

(Reporter: Mat Marquis, Assigned: johns)

Tracking

(Blocks: 3 bugs, {dev-doc-needed})

unspecified
mozilla32
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [v3.2 try: https://tbpl.mozilla.org/?tree=Try&rev=8bdfa77f7205], URL)

Attachments

(11 attachments, 29 obsolete attachments)

3.67 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
56.28 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
1.92 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.64 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.18 KB, patch
johns
: review+
Details | Diff | Splinter Review
15.91 KB, patch
johns
: review+
Details | Diff | Splinter Review
12.52 KB, patch
johns
: review+
Details | Diff | Splinter Review
5.65 KB, patch
johns
: review+
Details | Diff | Splinter Review
1.68 KB, patch
johns
: review+
Details | Diff | Splinter Review
3.66 KB, patch
johns
: review+
Details | Diff | Splinter Review
3.99 KB, patch
johns
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
The `srcset` attribute is an extension to the existing `img` tag that provides authors to suggest multiple image sources, to be requested/displayed in the event that certain author-specified window height/width or pixel density requirements are met. These suggestions can potentially be overridden by the UA based on environmental conditions, such as user preference for low-resolution images or limited bandwidth.

Specification published by the HTML WG:
http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/Overview.html

While the “resolution” aspect is ready for implementation, the width/height syntax is still in flux and may be revised/removed due to overlap with the `picture` element specification:
http://www.w3.org/TR/html-picture-element/

Implementation is currently underway in WebKit, in the reduced capacity described above:
https://bugs.webkit.org/show_bug.cgi?id=110252

Note that the `picture` element is able to make use of the resolution feature of the `srcset` attribute, but does not strictly require it. The two patterns are complementary, and together fulfill the full list of Use Cases and Requirements for Standardizing Responsive Images as published by the W3C:
http://www.w3.org/TR/2013/WD-respimg-usecases-20130226/
(In reply to Mat Marquis from comment #0)
> While the “resolution” aspect is ready for implementation, the width/height
> syntax is still in flux and may be revised/removed due to overlap with the
> `picture` element specification:
> http://www.w3.org/TR/html-picture-element/

Makes sense. The w/h stuff in srcset is very confusing.

> Implementation is currently underway in WebKit, in the reduced capacity
> described above:
> https://bugs.webkit.org/show_bug.cgi?id=110252

What's the situation with Blink and Trident?
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

4 years ago
Blink have implied that they'd be interested in a srcset implementation (or at least discussing it) in https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/MlE9vYVUlzg/hycQvA2eFn4J

I'll try to get a response from Trident RE their srcset support.
Blocks: 870022

Comment 3

4 years ago
Microsoft have nothing to share ATM with regard to srcset implementation: https://twitter.com/adrianba/status/333734512146145280

Comment 4

4 years ago
WebKit now supports the srcset attribute on img elements.

https://www.webkit.org/blog/2910/improved-support-for-high-resolution-displays-with-the-srcset-image-attribute/
(Reporter)

Comment 5

4 years ago
It should be noted that WebKit’s implementation of `srcset` is limited in scope to resolution only.
Keywords: dev-doc-needed

Comment 6

4 years ago
A related discussion on Blink-dev: https://groups.google.com/a/chromium.org/d/msg/blink-dev/dA8lbqLvaXA/Lp8-M2roqq8J

Conclusion: Blink would start implementation of srcset in its DPR form.

Comment 7

4 years ago
srcset implementation recently landed in Blink behind an experimental flag: https://chromiumcodereview.appspot.com/23861003/
We are planning to implement srcset with DPR selection hopefully before the end of the year. John Schoenick has it on his todo list.

Updated

4 years ago
We should see how `srcN` pans out before proceeding:

http://tabatkins.github.io/specs/respimg/Overview.html
OK, so DPR=devicePixelRatio. Gosh! Why do people make such abbreviations?
As demonstrated in [1] [2], srcset clearly doesn't meet the use cases outlined here [3]. Marking this as WONTFIX and will file a bug to implement src-n [4] instead. 


[1] http://www.xanthir.com/b4Su0
[2] http://www.w3.org/community/respimg/2013/10/14/reasoning-behind-srcn-replacing-srcset-and-picture/
[3] http://usecases.responsiveimages.org/
[4] http://tabatkins.github.io/specs/respimg/Overview.html
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
WONTFIXing is to be done by module owners/peers.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Ms2ger, my bad... I'm new here :) Module owner, please WONTFIX when possible. 

Anyway, src-n bug is here: 
https://bugzilla.mozilla.org/show_bug.cgi?id=936481
src-N is better.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → WONTFIX

Comment 15

3 years ago
src-N is better.

Comment 16

3 years ago
Note that native, default-enabled srcset has landed in Chrome 34 beta.

We've been using srcset on Wikipedia with a JavaScript polyfill for some time, and can confirm that the Chrome 34 native support works as expected for us. Until something actually supports srcN we've got no strong incentive to switch to that when srcset seems to be the only thing actively moving forward...

Comment 17

3 years ago
Now that src-N has died and <picture> revived, is this the right bug to reopen?  The <picture> spec adds srcset to <img> for when you don't need multiple <source>s.

Or is this covered by the existing <picture> bug?
(In reply to Brion Vibber from comment #16)
> Note that native, default-enabled srcset has landed in Chrome 34 beta.
> 
> We've been using srcset on Wikipedia with a JavaScript polyfill for some
> time, and can confirm that the Chrome 34 native support works as expected
> for us. Until something actually supports srcN we've got no strong incentive
> to switch to that when srcset seems to be the only thing actively moving
> forward...

This space moves quickly so your information is a little bit out of date.  

The srcN proposal is dead. Both Moz and Blink are working on <picture>, which makes use of srcset. So srcset was added to Blink to support the implementation of <picture>. 

To quote the post on the Blink blog (see last line!):

"Note that the src attribute is not needed for browsers that support srcset, but it’s good for backwards compatibility. Kudos to external Blink developer Yoav Weiss for implementing and driving consensus for this feature. Stay tuned for the <picture> element, which will also help web developers with responsive design. "

Yoav, who is also in this thread, can confirm - as he implemented srcset on the Blink side and is also driving the picture stuff there :)
(In reply to Tab Atkins Jr. from comment #17)
> Now that src-N has died and <picture> revived, is this the right bug to
> reopen?  The <picture> spec adds srcset to <img> for when you don't need
> multiple <source>s.
> 
> Or is this covered by the existing <picture> bug?

I guess we could reopen this one, as they are able to work independently.
The blink blog post about responsive images:
http://blog.chromium.org/2014/02/chrome-34-responsive-images-and_9316.html

Comment 21

3 years ago
(In reply to Marcos Caceres [:marcosc] from comment #18)

> Yoav, who is also in this thread, can confirm - as he implemented srcset on
> the Blink side and is also driving the picture stuff there :)

Indeed. The srcset implementation in Blink is the DPR switching syntax which an integral part of the picture implementation and is released as an incremental step towards picture.
Now it's a draft on w3c: http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/
(Assignee)

Updated

3 years ago
Assignee: nobody → jschoenick
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Comment 23

3 years ago
(In reply to Mte90 from comment #22)
> Now it's a draft on w3c: http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/

Nope, that's a cut-out of the original HTML version, with the weird w/h behavior.  It's not related to the RICG version, which is the one being implemented.
(Assignee)

Updated

3 years ago
See Also: → bug 936481
(Assignee)

Updated

3 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Updated

3 years ago
Alias: srcset

Comment 24

3 years ago
As a web developer, what is the status of this srcset (both in Mozilla and as a wider picture?).

I have live sites using srcset right now (simply for supplying retina images, so just using the 2x part) - then using a polyfill to load the larger images for browsers not supporting srcset.

Clearly the polyfill approach is not efficient, and means that the sites are now much quicker to load in Chrome on retina devices than other browsers to load.

I'd rather back a technology that will exist in the future, and not just be a Chrome thing, hence my question.

Will Firefox support srcset? If not, what should I use?

Comment 25

3 years ago
srcset is definitely not a "Chrome thing". This issue is assigned to John Schoenick, which AFAIK is actively working on it. While not ideal, the polyfill approach is what enabling us to introduce new features to the platform. My advice is to continue using that polyfill (after making sure that it's as performant as it can be, and is compliant with the latest spec: http://picture.responsiveimages.org). AFAIK (as a non-Mozillian), Firefox support will arrive. Be patient.
Yeah, John's working on <picture> which this bug blocks.  I was going to (likely incorrectly) write here his implementation plan here but I'll let him do that.  Last time we spoke, John told me he had patches for a large percentage of the work here.
Flags: needinfo?(jschoenick)
(Reporter)

Comment 27

3 years ago
(In fact, let the record show that Mozilla was the first to get on-board with the simplified `picture` spec, including the revamped `srcset`.)

Richard: picturefill.responsiveimages.org is the “canonical” polyfill for `picture`/`srcset` as they’re currently being implemented—if by “not efficient” you mean that your current solution is causing a double download in browsers that don’t yet have native support for `srcset`, switching to Picturefill might help with that.

If you mean “not efficient” as in “doomed to be forever polyfilled in everything but Chrome”: like everyone else has said, no worries there. Not only is the Mozilla implementation well underway, but `picture`/`srcset` are “under consideration” for IE (http://status.modern.ie/) and we have high hopes for a WebKit port of the completed Blink code.
(Assignee)

Comment 28

3 years ago
As noted, we are actively developing support for both picture and srcset, and there will be patches up for review shortly. The current plan is to land support, pref'd off initially, in Firefox 32 for both srcset and picture.

In the mean time, I agree with Yoav's advice: A picture/srcset polyfill is the thing to use while waiting for native support everywhere.
Flags: needinfo?(jschoenick)
(Assignee)

Comment 29

3 years ago
Also, we are targeting the version of srcset described by the picture spec [1], not the version in the current WHATWG spec that I don't believe anyone is implementing.

My plan is to land support for "basic" srcset in this bug with only "foo.jpg 1x, bar.jpg 2x" density support, and then land support for both the <picture> element and |sizes| in bug 870022

[1] http://picture.responsiveimages.org/

Comment 30

3 years ago
That's really helpful - thanks to all who replied.
(Assignee)

Comment 31

3 years ago
Created attachment 8417602 [details] [diff] [review]
[WIP v1] Part 1.1 - Teach parser about srcset (needs generation)
(Assignee)

Comment 32

3 years ago
Created attachment 8417603 [details] [diff] [review]
[WIP v1] Part 1.2 - Teach parser about srcset (generated bits)
(Assignee)

Comment 33

3 years ago
Created attachment 8417604 [details] [diff] [review]
[WIP v1] Part 1.3 - Add srcset to HTMLImageElement & atoms
(Assignee)

Comment 34

3 years ago
Created attachment 8417605 [details] [diff] [review]
[WIP v1] Part 2 - Add ResponsiveImageSelector class with basic srcset support
(Assignee)

Comment 35

3 years ago
Created attachment 8417606 [details] [diff] [review]
[WIP v1] Part 3 - Support basic srcset in HTMLImageElement via ResponsiveSelector
(Assignee)

Comment 36

3 years ago
Created attachment 8417607 [details] [diff] [review]
[WIP v1] Part 4 - Move HTMLImageElement::GetNatural{Height,Width} up to ImageLoadingContent (FIXME improper)
(Assignee)

Comment 37

3 years ago
Created attachment 8417608 [details] [diff] [review]
[WIP v1] Part 5 - HTMLImageElement responsive-aware overrides for GetNatural{Width,Height}
(Assignee)

Comment 38

3 years ago
The patches here provide a working pixel-ratio-only srcset implementation for <img>. Support for sizes/picture will be in bug 870022.

There are still a few issues I need to address and tests to be written before this is ready for review
(Assignee)

Comment 39

3 years ago
Try builds:
https://tbpl.mozilla.org/?tree=Try&rev=f6e65ea3afe4
(Assignee)

Comment 40

3 years ago
Created attachment 8426625 [details] [diff] [review]
[v2] Part 1.1 - Teach parser about srcset (needs generation)
Attachment #8417602 - Attachment is obsolete: true
(Assignee)

Comment 41

3 years ago
Created attachment 8426626 [details] [diff] [review]
[v2] Part 1.2 - Teach parser about srcset (generated bits)
Attachment #8417603 - Attachment is obsolete: true
(Assignee)

Comment 42

3 years ago
Created attachment 8426627 [details] [diff] [review]
[v2] Part 1.3 - Add srcset to HTMLImageElement & atoms
Attachment #8417604 - Attachment is obsolete: true
(Assignee)

Comment 43

3 years ago
Created attachment 8426629 [details] [diff] [review]
[v2] Part 2 - Add ResponsiveImageSelector class with basic srcset support
Attachment #8417605 - Attachment is obsolete: true
(Assignee)

Comment 44

3 years ago
Created attachment 8426630 [details] [diff] [review]
[v2] Part 3 - Support basic srcset in HTMLImageElement via ResponsiveSelector
Attachment #8417606 - Attachment is obsolete: true
(Assignee)

Comment 45

3 years ago
Created attachment 8426631 [details] [diff] [review]
[v2] Part 4 - Move HTMLImageElement::GetNatural{Height,Width} up to ImageLoadingContent (FIXME improper)
Attachment #8417607 - Attachment is obsolete: true
(Assignee)

Comment 46

3 years ago
Created attachment 8426632 [details] [diff] [review]
[v2] Part 5 - HTMLImageElement responsive-aware overrides for GetNatural{Width,Height}
Attachment #8417608 - Attachment is obsolete: true
(Assignee)

Comment 47

3 years ago
Created attachment 8426633 [details] [diff] [review]
[v2] Part 6.1 - Teach parser about currentSrc (needs generation)
(Assignee)

Comment 48

3 years ago
Created attachment 8426634 [details] [diff] [review]
[v2] Part 6.2 - Teach parser about currentSrc (generated bits)
(Assignee)

Comment 49

3 years ago
Created attachment 8426635 [details] [diff] [review]
[v2] Part 6.3 - Add getCurrentSrc to HTMLImageElement & atom
(Assignee)

Comment 50

3 years ago
Created attachment 8426636 [details] [diff] [review]
[v2] Part 7 - Pref off srcset behind dom.image.srcset.enabled

Prefixed 'dom.image' susch that it's grouped near the related
dom.image.picture.enabled in the future
(Assignee)

Comment 51

3 years ago
Updated patchset that I believe is review-ready, pending try sanity-run:

[prefs off] https://tbpl.mozilla.org/?tree=Try&rev=1648cec7316b
[prefs on]  https://tbpl.mozilla.org/?tree=Try&rev=3a96e069387a
(Assignee)

Comment 52

3 years ago
Created attachment 8428090 [details] [diff] [review]
[v3] Part 1.1 - Teach parser about srcset (needs generation)
Attachment #8426625 - Attachment is obsolete: true
(Assignee)

Comment 53

3 years ago
Created attachment 8428091 [details] [diff] [review]
[v3] Part 1.2 - Teach parser about srcset (generated bits)
Attachment #8426626 - Attachment is obsolete: true
(Assignee)

Comment 54

3 years ago
Created attachment 8428092 [details] [diff] [review]
[v3] Part 1.3 - Add srcset to HTMLImageElement & atoms
Attachment #8426627 - Attachment is obsolete: true
(Assignee)

Comment 55

3 years ago
Created attachment 8428093 [details] [diff] [review]
[v3] Part 2 - Add ResponsiveImageSelector class with basic srcset support
Attachment #8426629 - Attachment is obsolete: true
(Assignee)

Comment 56

3 years ago
Created attachment 8428094 [details] [diff] [review]
[v3] Part 3 - Support basic srcset in HTMLImageElement via ResponsiveSelector
Attachment #8426630 - Attachment is obsolete: true
(Assignee)

Comment 57

3 years ago
Created attachment 8428096 [details] [diff] [review]
[v3] Part 4 - Move HTMLImageElement::GetNatural{Height,Width} up to ImageLoadingContent
Attachment #8426631 - Attachment is obsolete: true
(Assignee)

Comment 58

3 years ago
Created attachment 8428097 [details] [diff] [review]
[v3] Part 5.1 - HTMLImageElement responsive-aware overrides for GetNatural{Width,Height}
Attachment #8426632 - Attachment is obsolete: true
(Assignee)

Comment 59

3 years ago
Created attachment 8428098 [details] [diff] [review]
[v3] Part 6.1 - Teach parser about currentSrc (needs generation)
Attachment #8426633 - Attachment is obsolete: true
(Assignee)

Comment 60

3 years ago
Created attachment 8428099 [details] [diff] [review]
[v3] Part 6.2 - Teach parser about currentSrc (generated bits)
Attachment #8426634 - Attachment is obsolete: true
(Assignee)

Comment 61

3 years ago
Created attachment 8428100 [details] [diff] [review]
[v3] Part 6.3 - Add getCurrentSrc to HTMLImageElement & atom
Attachment #8426635 - Attachment is obsolete: true
(Assignee)

Comment 62

3 years ago
Created attachment 8428101 [details] [diff] [review]
[v3] Part 7 - Pref off srcset behind dom.image.srcset.enabled
Attachment #8426636 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Whiteboard: [v3 try: https://tbpl.mozilla.org/?tree=Try&rev=d8f0547158ce]
(Assignee)

Comment 63

3 years ago
Try push for v3: https://tbpl.mozilla.org/?tree=Try&rev=d8f0547158ce
(Assignee)

Comment 64

3 years ago
Comment on attachment 8428090 [details] [diff] [review]
[v3] Part 1.1 - Teach parser about srcset (needs generation)

Just the manual bits, translation re-run in next patch
Attachment #8428090 - Flags: review?(hsivonen)
(Assignee)

Comment 65

3 years ago
Comment on attachment 8428091 [details] [diff] [review]
[v3] Part 1.2 - Teach parser about srcset (generated bits)

Results of translate_from_snapshot w/previous patch
Attachment #8428091 - Flags: review?(hsivonen)
(Assignee)

Comment 66

3 years ago
Comment on attachment 8428092 [details] [diff] [review]
[v3] Part 1.3 - Add srcset to HTMLImageElement & atoms

:jst volunteered to review the DOM bits for this!

This patch just enables srcset on img, no function yet
Attachment #8428092 - Flags: review?(jst)
(Assignee)

Comment 67

3 years ago
Comment on attachment 8428093 [details] [diff] [review]
[v3] Part 2 - Add ResponsiveImageSelector class with basic srcset support

Add ResponsiveImageSelector to handle sourcesets and picking the proper image. The idea is that in the future other responsive content might want to re-use this, and HTMLImageElement can conditionally instantiate it when needed to avoid overhead on the majority of non-responsive images.

I exposed this as mozilla::dom::ResponsiveImageSelector, let me know if you think it should live elsewhere.
Attachment #8428093 - Flags: review?(jst)
I think this warrants an intent to ship thread...
(Assignee)

Comment 69

3 years ago
Comment on attachment 8428094 [details] [diff] [review]
[v3] Part 3 - Support basic srcset in HTMLImageElement via ResponsiveSelector

Handle srcset in HTMLImageElement by instantiating a responsive image selector. This adds LoadSelectedImage() to avoid lots of redundant find-the-current-source calls.

We currently handle most changes synchronously, but once this is landed we can see what needs to be put into microtasks to ensure we're aligned with the spec re: image data. This is also still somewhat in flux, see:
https://github.com/ResponsiveImagesCG/picture-element/issues/152

This also doesn't address dynamically responding to density changes, which will also be a following.
Attachment #8428094 - Flags: review?(jst)
(Assignee)

Comment 70

3 years ago
Comment on attachment 8428092 [details] [diff] [review]
[v3] Part 1.3 - Add srcset to HTMLImageElement & atoms

I suppose this technically also needs sr, though this will be pref'd off initially (part 7)
Attachment #8428092 - Flags: superreview?(jst)
(Assignee)

Comment 71

3 years ago
Comment on attachment 8428096 [details] [diff] [review]
[v3] Part 4 - Move HTMLImageElement::GetNatural{Height,Width} up to ImageLoadingContent

ImageFrame needs to ask content if it has a special intrinsic width. Right now it does essentially what HTMLImageElement::GetNatural{Width/Height} does, however, it expects ImageLoadingContent to be its content partner. So lets move GetNaturalFoo up to ImageLoadingContent with the generic functions, then override them in HTMLImageElement.

If this is confusing, we could also rename them on ImageLoadingContent to e.g. intrinsicWidth, to avoid shadowing them on nsIDOMHTMLImageElement
Attachment #8428096 - Flags: review?(jst)
(Assignee)

Comment 72

3 years ago
Comment on attachment 8428097 [details] [diff] [review]
[v3] Part 5.1 - HTMLImageElement responsive-aware overrides for GetNatural{Width,Height}

This overrides GetNaturalWidth/Height to take the responsive image's density into account.
Attachment #8428097 - Flags: review?(jst)
(Assignee)

Comment 73

3 years ago
Comment on attachment 8428098 [details] [diff] [review]
[v3] Part 6.1 - Teach parser about currentSrc (needs generation)

javasrc changes to add currentSrc. Autogenerated pieces in next patch.
Attachment #8428098 - Flags: review?(hsivonen)
(Assignee)

Comment 74

3 years ago
Comment on attachment 8428097 [details] [diff] [review]
[v3] Part 5.1 - HTMLImageElement responsive-aware overrides for GetNatural{Width,Height}

(This should actually be 5.1, I missed the layout patch in this patchset)
Attachment #8428097 - Attachment description: [v3] Part 5 - HTMLImageElement responsive-aware overrides for GetNatural{Width,Height} → [v3] Part 5.1 - HTMLImageElement responsive-aware overrides for GetNatural{Width,Height}
(Assignee)

Comment 75

3 years ago
Created attachment 8428132 [details] [diff] [review]
[v3.1] Part 5.2 - Ask content to compute its intrinsic size in nsImageFrame

(Trying again on the right bug this time)

So with a >1.0 density image, our 'default' size ('intrinsic size' in the spec) is not the same as the "real" intrinsic size of the image layout sees. ImageFrame already assumes its content peer is ImageLoadingContent, so ask ImageLoadingContent for its NaturalWidth/Height and use it in place of the real intrinsic size for ComputeSize.

r?roc because he last reviewed changes to this code, but feel free to pass this off. Should be reviewed the the caveat that I have no idea what I'm doing in layout land.
Attachment #8428132 - Flags: review?(roc)
(Assignee)

Comment 76

3 years ago
Comment on attachment 8428099 [details] [diff] [review]
[v3] Part 6.2 - Teach parser about currentSrc (generated bits)

make translate_from_snapshot results of previous patch
Attachment #8428099 - Flags: review?(hsivonen)
(Assignee)

Comment 77

3 years ago
Comment on attachment 8428100 [details] [diff] [review]
[v3] Part 6.3 - Add getCurrentSrc to HTMLImageElement & atom

Expose currentSrc on HTMLImageElement. Pref'd off in part 7.
Attachment #8428100 - Flags: superreview?(jst)
Attachment #8428100 - Flags: review?(jst)
(Assignee)

Comment 78

3 years ago
Comment on attachment 8428101 [details] [diff] [review]
[v3] Part 7 - Pref off srcset behind dom.image.srcset.enabled

Disable the webidl magic and logic that looks at srcset behind dom.image.srcset.enabled (named such that it's close to dom.image.picture.enabled in the future)
Attachment #8428101 - Flags: review?(jst)
(Assignee)

Comment 79

3 years ago
Re-pushed to try since part 5.2 was inadvertently dropped from previous push:
https://tbpl.mozilla.org/?tree=Try&rev=777e61bb306a
Whiteboard: [v3 try: https://tbpl.mozilla.org/?tree=Try&rev=d8f0547158ce] → [v3.1 try: https://tbpl.mozilla.org/?tree=Try&rev=777e61bb306a]
(Assignee)

Updated

3 years ago
Attachment #8428132 - Attachment description: Part 5.2 - Ask content to compute its intrinsic size in nsImageFrame → [v3.1] Part 5.2 - Ask content to compute its intrinsic size in nsImageFrame
(Assignee)

Comment 80

3 years ago
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #68)
> I think this warrants an intent to ship thread...

Part 7 specs this off currently, will open a bug tracking remaining issues and send an intent to ship when we plan to flip it on.

Intent to implement was at:
https://groups.google.com/forum/#!msg/mozilla.dev.platform/p8xK79MtPVw/XvzPjAvg-mgJ

(the picture spec encompasses the 'new' srcset as well as <picture> element)
If it isn't too difficult, it would be a little nice to have it prefed off at all intermediate points, or bisection could be weird.
(In reply to comment #80)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #68)
> > I think this warrants an intent to ship thread...
> 
> Part 7 specs this off currently, will open a bug tracking remaining issues and
> send an intent to ship when we plan to flip it on.

Oh, right you are.  Please carry on...  :-)
Attachment #8428132 - Flags: review?(roc) → review+
Attachment #8428090 - Flags: review?(hsivonen) → review+
Attachment #8428091 - Flags: review?(hsivonen) → review+
Comment on attachment 8428098 [details] [diff] [review]
[v3] Part 6.1 - Teach parser about currentSrc (needs generation)

What's this about? AFAICT, currentSrc is a DOM-only attribute and there is no corresponding currentsrc markup attribute. What am I missing?
(Assignee)

Comment 84

3 years ago
Comment on attachment 8428098 [details] [diff] [review]
[v3] Part 6.1 - Teach parser about currentSrc (needs generation)

(In reply to Henri Sivonen (:hsivonen) from comment #83)
> Comment on attachment 8428098 [details] [diff] [review]
> [v3] Part 6.1 - Teach parser about currentSrc (needs generation)
> 
> What's this about? AFAICT, currentSrc is a DOM-only attribute and there is
> no corresponding currentsrc markup attribute. What am I missing?

You're right, was mirroring webidl changes to parser and not thinking critically :-P
Attachment #8428098 - Attachment is obsolete: true
Attachment #8428098 - Flags: review?(hsivonen)
(Assignee)

Updated

3 years ago
Attachment #8428099 - Attachment is obsolete: true
Attachment #8428099 - Flags: review?(hsivonen)
(Assignee)

Comment 85

3 years ago
Created attachment 8429435 [details] [diff] [review]
[v3.1.99] Part 6 - Add getCurrentSrc to HTMLImageElement

Dropped the unneccessary atom, webidl-only
Attachment #8428100 - Attachment is obsolete: true
Attachment #8428100 - Flags: superreview?(jst)
Attachment #8428100 - Flags: review?(jst)
Attachment #8429435 - Flags: superreview?(jst)
Attachment #8429435 - Flags: review?(jst)
(Assignee)

Comment 86

3 years ago
Created attachment 8431111 [details] [diff] [review]
[v3.2] Part 5.3 - nsImageFrame should take orientation into account when getting natural size from content

So 5.2 didn't account for image orientation and broke some tests :( it was inadvertently dropped from my earlier try push, sorry for the double review!

I'm not sure if any of this would be better cached than re-fetched in ComputeSizes, let me know if you don't think this is the best approach.
Attachment #8431111 - Flags: review?(roc)
(Assignee)

Comment 87

3 years ago
And with part 5.3 try failures are fixed:

https://tbpl.mozilla.org/?tree=Try&rev=bc3d92a09faa
Whiteboard: [v3.1 try: https://tbpl.mozilla.org/?tree=Try&rev=777e61bb306a] → [v3.2 try: https://tbpl.mozilla.org/?tree=Try&rev=bc3d92a09faa]
(Assignee)

Updated

3 years ago
Blocks: 1017875

Updated

3 years ago
Attachment #8428092 - Flags: superreview?(jst)
Attachment #8428092 - Flags: superreview+
Attachment #8428092 - Flags: review?(jst)
Attachment #8428092 - Flags: review+
Comment on attachment 8428093 [details] [diff] [review]
[v3] Part 2 - Add ResponsiveImageSelector class with basic srcset support

- In ResponsiveImageSelector::SetCandidatesFromSourceSet(const nsAString & aSrcSet):

+  while (iter != end) {

Maybe iter < end for extra warm fuzzies (or assert, mostly to catch stuff if this code changes down the road and someone gets the iterators into a bad state)?

- In ResponsiveImageSelector::SetDefaultSource(const nsAString & aSpec):

+{
+  if (aSpec.IsEmpty()) {
+    SetDefaultSource(nullptr);
+    return NS_OK;
+  }

Should this move below the error checking that's done right below this for added consistency?

- In ResponsiveImageSelector::GetBestCandidateIndex():

+  nsIDocument* doc = mContent ? mContent->OwnerDoc() : nullptr;
+  nsIPresShell *shell = doc ? doc->GetShell() : nullptr;
+  nsPresContext *pctx = shell ? shell->GetPresContext() : nullptr;
+
+  if (!shell) {
+    MOZ_ASSERT(false, "Unable to find document prescontext");

s/!shell/!pctx/ here.

- In ResponsiveImageCandidate::SetParamaterFromDescriptor(const nsAString & aDescriptor):

+  while (iter != end) {

Same here, check or assert that iter < end.

- In ResponsiveImageCandidate::HasSameParameter(const ResponsiveImageCandidate & aOther):

+  if (aOther.mType != mType) {
+    return false;
+  } else if (mType == eCandidateType_Default) {
+    return true;
+  } else if (mType == eCandidateType_Density) {
+    return aOther.mValue.mDensity == mValue.mDensity;
+  } else if (mType == eCandidateType_Invalid) {
+    MOZ_ASSERT(false, "Comparing invalid candidates?");
+    return true;
+  }

Remove the else-after-returns there. Same thing in ResponsiveImageCandidate::Density().

- In class ResponsiveImageCandidate:

+  union {
+    double mDensity;
+  } mValue;

This is a bit silly, but there's more members coming, so all good :)

r=jst
Attachment #8428093 - Flags: review?(jst) → review+
Attachment #8431111 - Flags: review?(roc) → review+
Comment on attachment 8428094 [details] [diff] [review]
[v3] Part 3 - Support basic srcset in HTMLImageElement via ResponsiveSelector

- In HTMLImageElement::MaybeLoadImage():

   // Note, check LoadingEnabled() after LoadImage call.
-  nsAutoString uri;
-  if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, uri) &&
-      (NS_FAILED(LoadImage(uri, false, true)) ||
-       !LoadingEnabled())) {
+  // XXX(johns): Why? ^

Very good question, but please remove the XXX comment.

r=jst
Attachment #8428094 - Flags: review?(jst) → review+

Updated

3 years ago
Attachment #8428096 - Flags: review?(jst) → review+
Comment on attachment 8428097 [details] [diff] [review]
[v3] Part 5.1 - HTMLImageElement responsive-aware overrides for GetNatural{Width,Height}

- In HTMLImageElement::GetNaturalHeight(uint32_t* aNaturalHeight):

+  double density;
+  if (mResponsiveSelector) {

Looks like density can be moved inside the if check. Same thing in HTMLImageElement::GetNaturalWidth(uint32_t* aNaturalWidth).

Also, this, or rather the previous patch (where I didn't catch this change) makes HTMLImageElement::NaturalHeight() call into HTMLImageElement::GetNaturalHeight() rather than the reverse which adds a virtual method call when called through WebIDL. Please reverse that order back to what it was. Same thing for (Get)NaturalWidth.

r=jst
Attachment #8428097 - Flags: review?(jst) → review+
Comment on attachment 8428101 [details] [diff] [review]
[v3] Part 7 - Pref off srcset behind dom.image.srcset.enabled

Please merge this patch with the first patch that exposes the srcset attribute (in WebIDL) so we don't first expose it and later on remove it by preffing it off (for no real reason AFAICT).

r=jst
Attachment #8428101 - Flags: review?(jst) → review+

Updated

3 years ago
Attachment #8429435 - Flags: superreview?(jst)
Attachment #8429435 - Flags: superreview+
Attachment #8429435 - Flags: review?(jst)
Attachment #8429435 - Flags: review+
(Assignee)

Comment 92

3 years ago
Created attachment 8431799 [details] [diff] [review]
[v3.3] Part 1.3 - Add srcset to HTMLImageElement & atoms. r=jst, sr=jst

Folded in pref
Attachment #8428092 - Attachment is obsolete: true
Attachment #8431799 - Flags: superreview+
Attachment #8431799 - Flags: review+
(Assignee)

Comment 93

3 years ago
Created attachment 8431801 [details] [diff] [review]
[v3.3] Part 2 - Add ResponsiveImageSelector class with basic srcset support. r=jst

Addressed nits. const_iter doesn't have a < operator, but ++ asserts if passing end anyway
Attachment #8428093 - Attachment is obsolete: true
Attachment #8431801 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8431801 - Attachment description: Part 2 - Add ResponsiveImageSelector class with basic srcset support. r=jst → [v3.3] Part 2 - Add ResponsiveImageSelector class with basic srcset support. r=jst
(Assignee)

Comment 94

3 years ago
Created attachment 8431804 [details] [diff] [review]
[v3.3] Part 3 - Support basic srcset in HTMLImageElement via ResponsiveSelector. r=jst

Address nit, fold in pref (former part 7)
Attachment #8428094 - Attachment is obsolete: true
Attachment #8431804 - Flags: review+
(Assignee)

Comment 95

3 years ago
Created attachment 8431806 [details] [diff] [review]
[v3.3] Part 4 - Move HTMLImageElement::GetNatural{Height,Width} up to ImageLoadingContent. r=jst

Address review notes
Attachment #8428096 - Attachment is obsolete: true
Attachment #8431806 - Flags: review+
(Assignee)

Comment 96

3 years ago
Created attachment 8431809 [details] [diff] [review]
[v3.3] Part 5.1 - HTMLImageElement responsive-aware overrides for GetNatural{Width,Height}. r=jst

Rebase on changed part 4
Attachment #8428097 - Attachment is obsolete: true
Attachment #8431809 - Flags: review+
(Assignee)

Comment 97

3 years ago
Created attachment 8431811 [details] [diff] [review]
[v3.3] Part 6 - Add getCurrentSrc to HTMLImageElement. r=jst, sr=jst

Folded in part 7 pref
Attachment #8429435 - Attachment is obsolete: true
Attachment #8431811 - Flags: superreview+
Attachment #8431811 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8428101 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Whiteboard: [v3.2 try: https://tbpl.mozilla.org/?tree=Try&rev=bc3d92a09faa] → [v3.2 try: https://tbpl.mozilla.org/?tree=Try&rev=8bdfa77f7205]
(Assignee)

Updated

3 years ago
Blocks: 1018389
(Assignee)

Comment 98

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8a4dc1220e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ea35e4133e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb47e85401b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/91f19f74704c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e440a816661
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d119f014e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/b56811f80128
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a326b265f41
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba422a2934f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/255f01c1a924
https://hg.mozilla.org/mozilla-central/rev/9a8a4dc1220e
https://hg.mozilla.org/mozilla-central/rev/9ea35e4133e5
https://hg.mozilla.org/mozilla-central/rev/eb47e85401b2
https://hg.mozilla.org/mozilla-central/rev/91f19f74704c
https://hg.mozilla.org/mozilla-central/rev/4e440a816661
https://hg.mozilla.org/mozilla-central/rev/a0d119f014e1
https://hg.mozilla.org/mozilla-central/rev/b56811f80128
https://hg.mozilla.org/mozilla-central/rev/4a326b265f41
https://hg.mozilla.org/mozilla-central/rev/ba422a2934f5
https://hg.mozilla.org/mozilla-central/rev/255f01c1a924
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment on attachment 8431811 [details] [diff] [review]
[v3.3] Part 6 - Add getCurrentSrc to HTMLImageElement. r=jst, sr=jst

This just landed exposed by default (though only to script, not to C++ code), because the [Pref] annotation is not actually on the member you're trying to pref off...

Shouldn't we have landed a test here?  Presumably that would have caught the problem.

(And also, shouldn't this be an NS_IMETHODIMP which does the pref check and then calls the void WebIDL method?)
Flags: needinfo?(jschoenick)
(Assignee)

Comment 101

3 years ago
Created attachment 8432885 [details] [diff] [review]
Followup, fix currentSrc visibility when pref'd off

... And will do similar for <picture> before attempting to land 870022
Attachment #8432885 - Flags: review?(bzbarsky)
Comment on attachment 8432885 [details] [diff] [review]
Followup, fix currentSrc visibility when pref'd off

You don't want or need the [Pref] on the partial interface itself, so remove it from there, please.  And outent the [Pref] on the member to line up with other annotations...

r=me
Attachment #8432885 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 103

3 years ago
Created attachment 8433601 [details] [diff] [review]
Followup, fix currentSrc visibility when pref'd off. r=bz
Attachment #8432885 - Attachment is obsolete: true
Attachment #8433601 - Flags: review+
(Assignee)

Comment 104

3 years ago
Followup landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6a2a30e9d48
Flags: needinfo?(jschoenick)
https://hg.mozilla.org/mozilla-central/rev/c6a2a30e9d48

Comment 106

3 years ago
Testcase:

http://jsbin.com/tozeluja/1/edit?html,output
Documentation will involve, at a minimum:

* Add srcset to the list of attributes on https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Img
* Update https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement to cover srcset and getCurrentSrc() (and any other missing items) properly.

Ideally, you would also:

* Add appropriate information to https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement.Image about how srcset affects the results.

A superstar would also:

* Add a new article to the HTML guide about working with images in HTML, which would broadly cover how to present images. This would be a big job though.

References:
* http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/
* https://bugzilla.mozilla.org/show_bug.cgi?id=870021
(Assignee)

Updated

3 years ago
Blocks: 1023516
(Assignee)

Updated

3 years ago
Blocks: 1023514
relnote-firefox: --- → ?
Added in the release notes for 32 with the wording "srcset attribute on img implemented"
relnote-firefox: ? → 32+
Sylvestre, you should clarify that it's behind a pref dom.image.srcset.enabled. Bug 1018389 is to flip it on by default.

ntim, when you nominate bugs for relnote that are preffed off, please make that clear otherwise it may confuse developers.
Flags: needinfo?(sledru)
Well, I am not taking it then. Thanks for the information.
relnote-firefox: 32+ → ?
Flags: needinfo?(sledru)
(Assignee)

Comment 111

3 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #110)
> Well, I am not taking it then. Thanks for the information.

Yeah I think this isn't relnote-worthy until it's flipped on: bug 1018389
relnote-firefox: ? → ---

Updated

3 years ago
Depends on: 1025833
(Assignee)

Updated

3 years ago
No longer depends on: 1025833
This issue should be marked as blocking bug 802882 as html5test.com will test for this attribute.

Sebastian
(Assignee)

Updated

3 years ago
Blocks: 1037643

Comment 113

3 years ago
Is support for the w/h height syntax in img srcset included in this bug, or is that being tracked elsewhere? w/h is now in Blink stable.
(Assignee)

Comment 114

3 years ago
(In reply to Ben from comment #113)
> Is support for the w/h height syntax in img srcset included in this bug, or
> is that being tracked elsewhere? w/h is now in Blink stable.

sizes support is implemented but guarded behind the <picture> pref, so it will be pref'd on in bug 1017875. The current implementation doesn't support h, bug 1080177 is on file for that, but it's unlikely to be shipped until it is spec'd. (Blink stable also doesn't support h, outside of the 'future-compat-h' steps in the parsing algo)
(Assignee)

Updated

3 years ago
Depends on: 1083072
Blocks: 1141380
Blocks: 1149357

Updated

2 years ago
Depends on: 1169196
Depends on: 1230110

Updated

6 months ago
Blocks: 1311357
You need to log in before you can comment on or make changes to this bug.