Bug 870021 (srcset)

Implement `srcset` attribute on `img`

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: mat, Assigned: johns)

Tracking

(Blocks 2 bugs, {dev-doc-needed})

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

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

6 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

6 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.

Comment 3

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

Comment 5

6 years ago
It should be noted that WebKit’s implementation of `srcset` is limited in scope to resolution only.

Comment 6

6 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

6 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.
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
Closed: 6 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
Closed: 6 years ago6 years ago
Resolution: --- → WONTFIX

Comment 15

6 years ago
src-N is better.

Comment 16

5 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

5 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.

Comment 21

5 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.
Assignee

Updated

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

Comment 23

5 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

5 years ago
See Also: → 936481
Assignee

Updated

5 years ago
Status: REOPENED → ASSIGNED
Assignee

Updated

5 years ago
Alias: srcset

Comment 24

5 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

5 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

5 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

5 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

5 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

5 years ago
That's really helpful - thanks to all who replied.
Assignee

Comment 38

5 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 40

5 years ago
Attachment #8417602 - Attachment is obsolete: true
Assignee

Comment 41

5 years ago
Attachment #8417603 - Attachment is obsolete: true
Assignee

Comment 42

5 years ago
Attachment #8417604 - Attachment is obsolete: true
Assignee

Comment 50

5 years ago
Prefixed 'dom.image' susch that it's grouped near the related
dom.image.picture.enabled in the future
Assignee

Comment 51

5 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 54

5 years ago
Attachment #8426627 - Attachment is obsolete: true
Assignee

Comment 60

5 years ago
Attachment #8426634 - Attachment is obsolete: true
Assignee

Comment 61

5 years ago
Attachment #8426635 - Attachment is obsolete: true
Assignee

Comment 62

5 years ago
Attachment #8426636 - Attachment is obsolete: true
Assignee

Updated

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

Comment 64

5 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

5 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

5 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

5 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)

Comment 68

5 years ago
I think this warrants an intent to ship thread...
Assignee

Comment 69

5 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

5 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

5 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

5 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

5 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

5 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

5 years ago
(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

5 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

5 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

5 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

5 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

5 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

5 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.

Comment 82

5 years ago
(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 #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

5 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

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

Comment 85

5 years ago
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

5 years ago
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

5 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

5 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+
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+
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+
Attachment #8429435 - Flags: superreview?(jst)
Attachment #8429435 - Flags: superreview+
Attachment #8429435 - Flags: review?(jst)
Attachment #8429435 - Flags: review+
Assignee

Comment 92

5 years ago
Folded in pref
Attachment #8428092 - Attachment is obsolete: true
Attachment #8431799 - Flags: superreview+
Attachment #8431799 - Flags: review+
Assignee

Comment 93

5 years ago
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

5 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

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

Comment 97

5 years ago
Folded in part 7 pref
Attachment #8429435 - Attachment is obsolete: true
Attachment #8431811 - Flags: superreview+
Attachment #8431811 - Flags: review+
Assignee

Updated

5 years ago
Attachment #8428101 - Attachment is obsolete: true
Assignee

Updated

5 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

5 years ago
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

5 years ago
... 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 104

5 years ago
Followup landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6a2a30e9d48
Flags: needinfo?(jschoenick)
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

5 years ago
Blocks: srcset-tests
Assignee

Updated

5 years ago
Blocks: 1023514

Updated

5 years ago
relnote-firefox: --- → ?
Added in the release notes for 32 with the wording "srcset attribute on img implemented"
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.
Flags: needinfo?(sledru)
Assignee

Comment 111

5 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: ? → ---
Depends on: 1025833
Assignee

Updated

5 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

5 years ago
Blocks: 1037643

Comment 113

5 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

5 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

5 years ago
Depends on: 1083072

Updated

4 years ago
Depends on: 1169196
Depends on: 1230110
Blocks: 1311357
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.