870022 - (picture) Implement `picture` element

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mat Marquis]

The `picture` element is a markup pattern that allows developers to declare multiple sources for an image using `media` attributes on `source` elements, similar to the `video` element. This allows authors to specify requirements for when/if image sources are requested/displayed.

Specification published by the HTML WG:
http://www.w3.org/TR/html-picture-element/

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/

Comment 1

[Marcos Caceres [:marcosc]]

A small data point: 
http://blog.yoav.ws/2013/05/How-Big-Is-Art-Direction

Comment 2

[Mat Marquis]

We ran an open survey, and 42% of the 264 responses (at time of posting this) said that they are making use of “art direction” with their current “responsive images” solution.

This survey is largely comprised of attendees of last week’s Mobilism conference and developers following http://twitter.com/smashingmag (702,117 followers), so I assume it represents a fairly wide cross-section of the developer community. It’s also worth noting that developers heavily favor the `picture` element (or equivalent polyfill).

https://docs.google.com/forms/d/1LIbd9wiM7M_m-rUB85WAiGT-h8QUY-PRJwjiBRiuY4Y/viewanalytics

Comment 3

[Dave Newton]

+1 would love to see this implemented.

Comment 4

[Johnny Martin]

Let's make it happen!

Comment 5

[John Schoenick [:johns]]

I'm working on the implementation for this now - I hope to have something stood up in a few weeks.

Comment 6

[John Schoenick [:johns]]

Attachment: [WIP v1] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage

Comment 7

[John Schoenick [:johns]]

Attachment: [WIP v1] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar

Comment 8

[John Schoenick [:johns]]

Attachment: [WIP v1] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing

Comment 9

[John Schoenick [:johns]]

Attachment: [WIP v1] Part 3.2 - Add sizes support to ResponsiveImageSelector

Comment 10

[John Schoenick [:johns]]

Attachment: [WIP v1] Part 4.1 - Add sizes to HTMLImageElement & atoms

Comment 11

[John Schoenick [:johns]]

Attachment: [WIP v1] Part 4.2 - Teach parser about sizes attr (needs generate)

Comment 12

[John Schoenick [:johns]]

Attachment: [WIP v1] Part 4.3 - Teach parser about sizes attr (generated bits)

Comment 13

[John Schoenick [:johns]]

Attachment: [WIP v1] Part 5.1 - Add HTMLPictureElement & atom

Comment 14

[John Schoenick [:johns]]

Attachment: [WIP v1] Part 5.2 - Add sizes/srcset to HTMLSourceElement

Comment 15

[John Schoenick [:johns]]

Attachment: [WIP v1] Part 5.3 - Teach parser about <picture> (non-generated bits)

Comment 16

[John Schoenick [:johns]]

Attachment: [WIP v1] Part 5.4 - Teach parser about <picture> (generated bits)

Comment 17

[John Schoenick [:johns]]

Attachment: [WIP v1] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement

Comment 18

[John Schoenick [:johns]]

Attachment: [WIP v1] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes

Comment 19

[John Schoenick [:johns]]

These patches provide a basically working <picture> element as well as sizes attr support when applied on top of those in bug 870021. I need to finish reviewing my list-o-issues and add a pref toggle and tests before these are ready for review.

Try builds at:
https://tbpl.mozilla.org/?tree=Try&rev=041e5fbff244

Note that this implementation does not fully match the spec yet, including error-handling and various edge case behaviors that were in flux spec-wise. The plan is to land this behind a pref, then file bugs for conformance issues blocking preffing on by default.

Comment 20

[John Schoenick [:johns]]

Missing header + fatal clang warning busted a few builds, woo!

Repushed try for just failed OS X / Win32 w/fix:
https://tbpl.mozilla.org/?tree=Try&rev=4a1342079d79

Comment 21

[John Schoenick [:johns]]

Attachment: [v2] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage

Comment 22

[John Schoenick [:johns]]

Attachment: [v2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar

Comment 23

[John Schoenick [:johns]]

Attachment: [v2] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing

Comment 24

[John Schoenick [:johns]]

Attachment: [v2] Part 3.2 - Add sizes support to ResponsiveImageSelector

Comment 25

[John Schoenick [:johns]]

Attachment: [v2] Part 4.1 - Add sizes to HTMLImageElement & atoms

Comment 26

[John Schoenick [:johns]]

Attachment: [v2] Part 4.2 - Teach parser about sizes attr (non-generated bits)

Comment 27

[John Schoenick [:johns]]

Attachment: [v2] Part 4.2 - Teach parser about sizes attr (generated bits)

Comment 28

[John Schoenick [:johns]]

Attachment: [v2] Part 5.1 - Add HTMLPictureElement & atom

Comment 29

[John Schoenick [:johns]]

Attachment: [v2] Part 5.2 - Add sizes/srcset to HTMLSourceElement

Comment 30

[John Schoenick [:johns]]

Attachment: [v2] Part 5.3 - Teach parser about <picture> (non-generated bits)

Comment 31

[John Schoenick [:johns]]

Attachment: [v2] Part 5.4 - Teach parser about <picture> (generated bits)

Comment 32

[John Schoenick [:johns]]

Attachment: [v2] Part 5.5 - Add <picture> to nsHTMLEditUtils

Comment 33

[John Schoenick [:johns]]

Attachment: [v2] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement

Comment 34

[John Schoenick [:johns]]

Attachment: [v2] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes

Comment 35

[John Schoenick [:johns]]

Attachment: [v2] Part 8 - Pref picture off behind dom.image.picture.enabled

Comment 36

[John Schoenick [:johns]]

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

Comment 37

[John Schoenick [:johns]]

Attachment: [v3] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage

Comment 38

[John Schoenick [:johns]]

Attachment: [v3] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar

Comment 39

[John Schoenick [:johns]]

Attachment: [v3] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing

Comment 40

[John Schoenick [:johns]]

Attachment: [v3] Part 3.2 - Add sizes support to ResponsiveImageSelector

Comment 41

[John Schoenick [:johns]]

Attachment: [v3] Part 4.1 - Add sizes to HTMLImageElement & atoms

Comment 42

[John Schoenick [:johns]]

Attachment: [v3] Part 4.2 - Teach parser about sizes attr (non-generated bits)

Comment 43

[John Schoenick [:johns]]

Attachment: [v3] Part 4.3 - Teach parser about sizes attr (generated bits)

Comment 44

[John Schoenick [:johns]]

Attachment: [v3] Part 5.1 - Add HTMLPictureElement & atom

Comment 45

[John Schoenick [:johns]]

Attachment: [v3] Part 5.2 - Add sizes/srcset to HTMLSourceElement

Comment 46

[John Schoenick [:johns]]

Attachment: [v3] Part 5.3 - Teach parser about <picture> (non-generated bits)

Comment 47

[John Schoenick [:johns]]

Attachment: [v3] Part 5.4 - Teach parser about <picture> (generated bits)

Comment 48

[John Schoenick [:johns]]

Attachment: [v3] Part 5.5 - Add <picture> to nsHTMLEditUtils

Comment 49

[John Schoenick [:johns]]

Attachment: [v3] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement

Comment 50

[John Schoenick [:johns]]

Attachment: [v3] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes

Comment 51

[John Schoenick [:johns]]

Attachment: [v3] Part 8 - Pref picture off behind dom.image.picture.enabled

Comment 52

[John Schoenick [:johns]]

Try push for v3: https://tbpl.mozilla.org/?tree=Try&rev=17b503f372f9

Comment 53

[John Schoenick [:johns]]

Attachment: [v3.1] Part 5.2 - Ask content to compute its intrinsic size in nsImageFrame

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.

Comment 54

[John Schoenick [:johns]]

Comment on attachment 8428131 [details] [diff] [review]
[v3.1] Part 5.2 - Ask content to compute its intrinsic size in nsImageFrame

Annnd attached to the wrong bug Looking for 87002*1*.

Comment 55

[John Schoenick [:johns]]

Updated try, patchset in bug 870021 this is based on inadvertently dropped an important change in previous push:
https://tbpl.mozilla.org/?tree=Try&rev=263bdc53b84d

Comment 56

[John Schoenick [:johns]]

Attachment: [v3.2] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage

Comment 57

[John Schoenick [:johns]]

Attachment: [v3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar

Comment 58

[John Schoenick [:johns]]

Attachment: [v3.2] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing

Comment 59

[John Schoenick [:johns]]

Attachment: [v3.2] Part 3.2 - Add sizes support to ResponsiveImageSelector

Comment 60

[John Schoenick [:johns]]

Attachment: [v3.2] Part 4.1 - Add sizes to HTMLImageElement & atoms

Comment 61

[John Schoenick [:johns]]

Attachment: [v3.2] Part 4.2 - Teach parser about sizes attr (non-generated bits)

Comment 62

[John Schoenick [:johns]]

Attachment: [v3.2] Part 4.3 - Teach parser about sizes attr (generated bits)

Comment 63

[John Schoenick [:johns]]

Attachment: [v3.2] Part 5.1 - Add HTMLPictureElement & atom

Comment 64

[John Schoenick [:johns]]

Attachment: [v3.2] Part 5.2 - Add sizes/srcset to HTMLSourceElement

Comment 65

[John Schoenick [:johns]]

Attachment: [v3.2] Part 5.3 - Teach parser about <picture> (non-generated bits)

Comment 66

[John Schoenick [:johns]]

Attachment: [v3.2] Part 5.4 - Teach parser about <picture> (generated bits)

Comment 67

[John Schoenick [:johns]]

Attachment: [v3.2] Part 5.5 - Add <picture> to nsHTMLEditUtils

Comment 68

[John Schoenick [:johns]]

Attachment: [v3.2] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement

Comment 69

[John Schoenick [:johns]]

Attachment: [v3.2] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes

Comment 70

[John Schoenick [:johns]]

Attachment: [v3.2] Part 8 - Pref picture off behind dom.image.picture.enabled

Comment 71

[John Schoenick [:johns]]

Try with failures from inadvertently dropped patch being re-included all fixed and some sizes media-query-matching weirdness fixed due to two separate accidentally inverted conditions almost but not quite canceling each other out:

https://tbpl.mozilla.org/?tree=Try&rev=b79964d32edc

Comment 72

[John Schoenick [:johns]]

Comment on attachment 8431113 [details] [diff] [review]
[v3.2] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage

This splits out the spec-compliant integer parsing from nsAttrValue. Let me know if the enum flags are overkill, we could split this into multiple functions for the different use-cases.

Comment 73

[John Schoenick [:johns]]

Comment on attachment 8431114 [details] [diff] [review]
[v3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar

@bz flagging you because touched similar CSS parser stuff recently, but let me know if there's someone more appropriate

This adds basic parser support for the picture 'sizes' grammar:
http://picture.responsiveimages.org/#parse-sizes-attr

This is already slightly out of date with the spec and doesn't handle error paths correctly, I filed bug 1017878 blocking pref'ing any of this on to bring it in sync with the spec + tests, so this should be mostly ensuring it is the right approach and not doing anything stupid (Which it may be, I have not touched the CSS parser before)

Comment 74

[John Schoenick [:johns]]

Comment on attachment 8431116 [details] [diff] [review]
[v3.2] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing

Adds a width type to ResponsiveImageCandidate and update the SetParamaterFromDescriptor parser, used by ResponsiveImageSelector in next piece

Comment 75

[John Schoenick [:johns]]

Comment on attachment 8431117 [details] [diff] [review]
[v3.2] Part 3.2 - Add sizes support to ResponsiveImageSelector

Add SetSizesFromDescriptor which uses the new CSS parser grammar to get a list of lengths and their associated queries, store them, use them to compute final densities when computed-from-width types are added.

This is already slightly out of date with the spec, I opened bug 1017878 blocking prefing this on updating it, specifically error handling.

Comment 76

[John Schoenick [:johns]]

Comment on attachment 8431118 [details] [diff] [review]
[v3.2] Part 4.1 - Add sizes to HTMLImageElement & atoms

Exposes HTMLImageElement::Sizes, pref'd off in part 8

Comment 77

[John Schoenick [:johns]]

Comment on attachment 8431119 [details] [diff] [review]
[v3.2] Part 4.2 - Teach parser about sizes attr (non-generated bits)

Add sizes attr to javasrc, generated bits in next patch

Comment 78

[John Schoenick [:johns]]

Comment on attachment 8431120 [details] [diff] [review]
[v3.2] Part 4.3 - Teach parser about sizes attr (generated bits)

Results of make translate_from_snapshot w/previous patch

Comment 79

[John Schoenick [:johns]]

Comment on attachment 8431121 [details] [diff] [review]
[v3.2] Part 5.1 - Add HTMLPictureElement & atom

Adds stub HTMLPictureElement

Comment 80

[John Schoenick [:johns]]

Comment on attachment 8431122 [details] [diff] [review]
[v3.2] Part 5.2 - Add sizes/srcset to HTMLSourceElement

Adds srcset and sizes to <source>

Comment 81

[John Schoenick [:johns]]

Comment on attachment 8431123 [details] [diff] [review]
[v3.2] Part 5.3 - Teach parser about <picture> (non-generated bits)

Adds picture element to parser, non-generated parts

Comment 82

[John Schoenick [:johns]]

Comment on attachment 8431124 [details] [diff] [review]
[v3.2] Part 5.4 - Teach parser about <picture> (generated bits)

Regenerated bits from make translate_from_snapshot

Comment 83

[John Schoenick [:johns]]

Comment on attachment 8431128 [details] [diff] [review]
[v3.2] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes

Get events from <source> srcset/sizes changing and maybe switch what element mResponsiveSelector is pulling from. This doesn't handle all the microtask-ordering in the spec or dynamically responding to MQ changes, which will be follow-ups blocking pref-on

Comment 84

[John Schoenick [:johns]]

Comment on attachment 8431129 [details] [diff] [review]
[v3.2] Part 8 - Pref picture off behind dom.image.picture.enabled

The changes to block this behind a pref in one place, but can be folded into above patches before landing to avoid causing bisect pain

Comment 85

[John Schoenick [:johns]]

Comment on attachment 8431127 [details] [diff] [review]
[v3.2] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement

@cpearce because he reviewed this code when it was added, but feel free to pass to more appropriate reviewer.

Since <source media> is now used by picture parents as well, this exposes HTMLSourceElement::MatchesCurrentMedia and uses it from HTMLMediaElement instead of handling the contents of the attr inline.

(<source> keeps its medialist around because it will need to dynamically respond to changes in a future patch, but <video> will not make use of dynamic source switching.)

Comment 86

[John Schoenick [:johns]]

Comment on attachment 8431126 [details] [diff] [review]
[v3.2] Part 5.5 - Add <picture> to nsHTMLEditUtils

@ehsan the modules page leads me to believe you own this, but let me know if that's out of date

Comment 87

[(no longer active)]

Comment on attachment 8431126 [details] [diff] [review]
[v3.2] Part 5.5 - Add <picture> to nsHTMLEditUtils

Review of attachment 8431126 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/html/nsHTMLEditUtils.cpp
@@ +734,5 @@
>         GROUP_SELECT_CONTENT | GROUP_OPTIONS, GROUP_LEAF),
>    ELEM(output, true, true, GROUP_SPECIAL, GROUP_INLINE_ELEMENT),
>    ELEM(p, true, false, GROUP_BLOCK | GROUP_P, GROUP_INLINE_ELEMENT),
>    ELEM(param, false, false, GROUP_OBJECT_CONTENT, GROUP_NONE),
> +  ELEM(picture, true, true, GROUP_SPECIAL, GROUP_INLINE_ELEMENT),

So first of all, picture cannot contain itself.  Also I think you want to create a new group for the contents of this element since GROUP_INLINE_ELEMENT doesn't match what the spec says for this.  Please see the comments around line 516 in this file and also nsHTMLEditUtils::CanContain to see how this setup works.

Comment 88

[Johnny Stenback (:jst)]

Comment on attachment 8431113 [details] [diff] [review]
[v3.2] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage

Looks good! r=jst

Comment 89

[Johnny Stenback (:jst)]

Comment on attachment 8431117 [details] [diff] [review]
[v3.2] Part 3.2 - Add sizes support to ResponsiveImageSelector

- In ResponsiveImageSelector::ComputeFinalWidthForCurrentViewport(int32_t *aWidth):

+  for (i = 0; i < numSizes; i++) {
+    if (pctx && mSizeQueries[i]->Matches(pctx, nullptr)) {

You return early above if pctx is null, so the pctx condition is redundant here.

r=jst

Comment 90

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8431114 [details] [diff] [review]
[v3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar

I'll try to read this in detail tomorrow, but why do we need the separate GatherSourceSizeList function?

Comment 91

[Henri Sivonen (:hsivonen)]

Comment on attachment 8431123 [details] [diff] [review]
[v3.2] Part 5.3 - Teach parser about <picture> (non-generated bits)

(We really ought to get rid of nsElementTable, etc, at some point.)

Comment 92

[John Schoenick [:johns]]

(In reply to Boris Zbarsky [:bz] from comment #90)
> Comment on attachment 8431114 [details] [diff] [review]
> [v3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes>
> grammar
> 
> I'll try to read this in detail tomorrow, but why do we need the separate
> GatherSourceSizeList function?

I was just following the pattern in the file where ParseSourceSizeList is the external API wrapper, it could be inlined

Comment 93

[Johnny Stenback (:jst)]

Comment on attachment 8431118 [details] [diff] [review]
[v3.2] Part 4.1 - Add sizes to HTMLImageElement & atoms

># HG changeset patch
># User John Schoenick <[e-mail]>
>
>Bug 870022 - Part 4.1 - Add sizes to HTMLImageElement & atoms
>
>diff --git a/content/html/content/src/HTMLImageElement.cpp b/content/html/content/src/HTMLImageElement.cpp
>index 39b9246..15a46a7 100644
>--- a/content/html/content/src/HTMLImageElement.cpp
>+++ b/content/html/content/src/HTMLImageElement.cpp
>@@ -82,16 +82,17 @@ NS_IMPL_ELEMENT_CLONE(HTMLImageElement)
> 
> NS_IMPL_STRING_ATTR(HTMLImageElement, Name, name)
> NS_IMPL_STRING_ATTR(HTMLImageElement, Align, align)
> NS_IMPL_STRING_ATTR(HTMLImageElement, Alt, alt)
> NS_IMPL_STRING_ATTR(HTMLImageElement, Border, border)
> NS_IMPL_INT_ATTR(HTMLImageElement, Hspace, hspace)
> NS_IMPL_BOOL_ATTR(HTMLImageElement, IsMap, ismap)
> NS_IMPL_URI_ATTR(HTMLImageElement, LongDesc, longdesc)
>+NS_IMPL_STRING_ATTR(HTMLImageElement, Sizes, sizes)
> NS_IMPL_STRING_ATTR(HTMLImageElement, Lowsrc, lowsrc)
> NS_IMPL_URI_ATTR(HTMLImageElement, Src, src)
> NS_IMPL_STRING_ATTR(HTMLImageElement, Srcset, srcset)
> NS_IMPL_STRING_ATTR(HTMLImageElement, UseMap, usemap)
> NS_IMPL_INT_ATTR(HTMLImageElement, Vspace, vspace)
> 
> bool
> HTMLImageElement::IsSrcsetEnabled()
>diff --git a/content/html/content/src/HTMLImageElement.h b/content/html/content/src/HTMLImageElement.h
>index ee46b0d..9c28e02 100644
>--- a/content/html/content/src/HTMLImageElement.h
>+++ b/content/html/content/src/HTMLImageElement.h
>@@ -154,16 +154,20 @@ public:
>   void SetAlign(const nsAString& aAlign, ErrorResult& aError)
>   {
>     SetHTMLAttr(nsGkAtoms::align, aAlign, aError);
>   }
>   void SetLongDesc(const nsAString& aLongDesc, ErrorResult& aError)
>   {
>     SetHTMLAttr(nsGkAtoms::longdesc, aLongDesc, aError);
>   }
>+  void SetSizes(const nsAString& aSizes, ErrorResult& aError)
>+  {
>+    SetHTMLAttr(nsGkAtoms::sizes, aSizes, aError);
>+  }
>   void SetBorder(const nsAString& aBorder, ErrorResult& aError)
>   {
>     SetHTMLAttr(nsGkAtoms::border, aBorder, aError);
>   }
> 
>   int32_t X();
>   int32_t Y();
>   // Uses XPCOM GetLowsrc.
>diff --git a/dom/interfaces/html/nsIDOMHTMLImageElement.idl b/dom/interfaces/html/nsIDOMHTMLImageElement.idl
>index 65d9084..1419ed7 100644
>--- a/dom/interfaces/html/nsIDOMHTMLImageElement.idl
>+++ b/dom/interfaces/html/nsIDOMHTMLImageElement.idl
>@@ -11,22 +11,23 @@
>  *
>  * This interface is trying to follow the DOM Level 2 HTML specification:
>  * http://www.w3.org/TR/DOM-Level-2-HTML/
>  *
>  * with changes from the work-in-progress WHATWG HTML specification:
>  * http://www.whatwg.org/specs/web-apps/current-work/
>  */
> 
>-[uuid(939f4ea1-cb8d-49d0-a4e1-23bce758f4af)]
>+[uuid(e83e726a-0aef-4292-938b-253fec691e2f)]
> interface nsIDOMHTMLImageElement : nsISupports
> {
>            attribute DOMString        alt;
>            attribute DOMString        src;
>            attribute DOMString        srcset;
>+           attribute DOMString        sizes;
>            attribute DOMString        crossOrigin;
>            attribute DOMString        useMap;
>            attribute boolean          isMap;
>            attribute unsigned long    width;
>            attribute unsigned long    height;
>   readonly attribute unsigned long    naturalWidth;
>   readonly attribute unsigned long    naturalHeight;
>   readonly attribute boolean          complete;
>diff --git a/dom/webidl/HTMLImageElement.webidl b/dom/webidl/HTMLImageElement.webidl
>index 94c714b..9f26477 100644
>--- a/dom/webidl/HTMLImageElement.webidl
>+++ b/dom/webidl/HTMLImageElement.webidl
>@@ -55,16 +55,18 @@ partial interface HTMLImageElement {
> 
>   [TreatNullAs=EmptyString,SetterThrows] attribute DOMString border;
> };
> 
> // [Update me: not in whatwg spec yet]
> // http://picture.responsiveimages.org/#the-img-element
> [Pref="dom.image.srcset.enabled"]
> partial interface HTMLImageElement {
>+           [SetterThrows]
>+           attribute DOMString sizes;
>            readonly attribute DOMString? currentSrc;
> };
> 
> // Mozilla extensions.
> partial interface HTMLImageElement {
>            attribute DOMString lowsrc;
> 
>   // These attributes are offsets from the closest view (to mimic

Comment 94

[Boris Zbarsky [:bzbarsky]]

> it could be inlined

Unless we think we'll have other callers of GatherSourceSizeList, I think we should inline it.

Comment 95

[Johnny Stenback (:jst)]

Comment on attachment 8431129 [details] [diff] [review]
[v3.2] Part 8 - Pref picture off behind dom.image.picture.enabled

Looks good, but please re-order these changes so that we don't land pieces of this enabled before we land the pref for letting us disable it. r=jst

Comment 96

[John Schoenick [:johns]]

Attachment: [v3.3] Part 5.5 - Add <picture> to nsHTMLEditUtils

Okay, slightly more careful attempt at this. Mimiced some of the other GROUP_*_CONTENT classes and added source and img to GROUP_PICTURE_CONTENT. (Looks like <video> was never properly specified to contain <source> either :( )

Let me know if this looks proper

Comment 97

[John Schoenick [:johns]]

Attachment: [v3.3] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar

(In reply to Boris Zbarsky [:bz] from comment #94)
> > it could be inlined
>
> Unless we think we'll have other callers of GatherSourceSizeList, I think we
> should inline it.

Inlined GatherSourceSizeList into ParseSourceSizeList (no other differences)

Comment 98

[(no longer active)]

Comment on attachment 8434452 [details] [diff] [review]
[v3.3] Part 5.5 - Add <picture> to nsHTMLEditUtils

Review of attachment 8434452 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Comment 99

[John Schoenick [:johns]]

Attachment: [v3.3] Part 3.2 - Add sizes support to ResponsiveImageSelector. r=jst

Addressed review comment

Comment 100

[John Schoenick [:johns]]

Attachment: [v3.3] Part 4.1 - Add sizes to HTMLImageElement & atoms. r=jst, sr=jst

Folded pref (part 8) in

Comment 101

[John Schoenick [:johns]]

Attachment: [v3.3] Part 5.1 - Add HTMLPictureElement & atom. r=jst, sr=jst

Folded pref (part 8) in

Comment 102

[John Schoenick [:johns]]

Attachment: [v3.3] Part 5.2 - Add sizes/srcset to HTMLSourceElement. r=jst, sr=jst

Folded pref (Part 8) in

Comment 103

[John Schoenick [:johns]]

Attachment: Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes. r=jst

Folded in pref (Part 8)

Comment 104

[John Schoenick [:johns]]

Attachment: Add test for dom.image.picture.enabled

Test to ensure dom.image.picture.enabled properly disables everything

Comment 105

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8434464 [details] [diff] [review]
[v3.3] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar

I'm sorry about the terrible lag.

>+CSSParserImpl::ParseSourceSizeList(const nsAString& aBuffer,
>+                           bool aHTMLMode);

Are there any callers that pass false?  If not, why do we need that argument?

In fact, as far as I can tell mHTMLMediaMode is write-only and we can just remove it.  Followup bug on that?

Also, document what the return value means and that aQueries and aValues are out params that will be overwritten (as opposed to appended to).

>+      hitError = true;
>+      break;

Yeah, this definitely doesn't seem to match the current spec.  The spec says to basically recover at the net toplevel comma, right?

I'm OK with a followup here if you want to, but this is really the guts of the patch, so I'm not sure how worthwhile it is to review this bit if it'll all be gutted anyway.  :(

>+      // Empty conditions (e.g. just a bare value) should be treated as always
>+      // matching.
>+      query->SetNegated();

Why does "always matching" have anything to do with SetNegated?

>+    // Since we are only trying to consume a single condition, which precludes
>+    // media types and not/only, this should be the same as reaching immediate
>+    // EOF (no condition to parse)

Again, pretty different from what the spec says now.

>+    if (!singleCondition &&
>+        eCSSToken_Symbol == mToken.mType && mToken.mSymbol == ',') {

Why the !singleCondition bit?

r=me as far as it goes, but we really need to fix the parsing to follow the spec.

Comment 106

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8434530 [details] [diff] [review]
Add test for dom.image.picture.enabled

r=me

Comment 107

[John Schoenick [:johns]]

Attachment: [v3.3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar. r=bz

(In reply to Boris Zbarsky [:bz] from comment #105)
> Comment on attachment 8434464 [details] [diff] [review]
> [v3.3] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes>
> grammar
>
> Are there any callers that pass false?  If not, why do we need that argument?
>
> In fact, as far as I can tell mHTMLMediaMode is write-only and we can just
> remove it.  Followup bug on that?

The comment mentions that it's for "parsing media lists for HTML attributes, where we have to ignore CSS comments", but I didn't catch that it was in fact write-only. Filed bug 1029867

> Also, document what the return value means and that aQueries and aValues are
> out params that will be overwritten (as opposed to appended to).

Done

> >+      hitError = true;
> >+      break;
>
> Yeah, this definitely doesn't seem to match the current spec.  The spec says
> to basically recover at the net toplevel comma, right?
>
> I'm OK with a followup here if you want to, but this is really the guts of
> the patch, so I'm not sure how worthwhile it is to review this bit if it'll
> all be gutted anyway.  :(

The spec at the time bailed at parse error, and has gone through a few changes since. There should be no differences with well formatted strings, and people are very interested in testing this on nightly, hence the land-and-follow-up approach.

> >+      // Empty conditions (e.g. just a bare value) should be treated as always
> >+      // matching.
> >+      query->SetNegated();
>
> Why does "always matching" have anything to do with SetNegated?

A query with no expressions never matches, and thus a negated-empty-query matches anything. I updated the comment, but let me know if there's a better way to do this.

> >+    if (!singleCondition &&
> >+        eCSSToken_Symbol == mToken.mType && mToken.mSymbol == ',') {
>
> Why the !singleCondition bit?

If this is single condition mode, we want to check if the next item is "and" otherwise UngetToken() and break. We could add another | if (singleCondition) { UngetToken(); } | check inside this block, but the check it's doing is superfluous for singleCondition, so this seemed cleaner.

Comment 108

[John Schoenick [:johns]]

Attachment: [v3.3.1] Part 4.3 - Teach parser about sizes attr (generated bits). r=hsivonen

Re-generated since things landed underneith. Carrying over r=hsivonen

Comment 109

[John Schoenick [:johns]]

Attachment: [v3.3.1] Part 5.1 - Add HTMLPictureElement & atom. r=jst, sr=jst

rebased for nsINodeInfo rename, carrying over r,sr=jst

Comment 110

[John Schoenick [:johns]]

Attachment: [v3.3.1] Part 5.4 - Teach parser about <picture> (generated bits). r=hsivonen

Regenerated due to other landings, carrying over r=hsivonen

Comment 111

[John Schoenick [:johns]]

Attachment: [v3.3.1] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes. r=jst

rebased for nsINodeInfo rename, no functional changes, carrying over r=jst

Comment 112

[John Schoenick [:johns]]

Attachment: Add test for dom.image.picture.enabled. r=bz

s/blue.png/red.png/ because some other test in this directory is checking load counts of blue.png (which isn't strictly proper)

Comment 113

[Boris Zbarsky [:bzbarsky]]

> I updated the comment, but let me know if there's a better way to do this.

Seems fine, thanks.

Comment 114

[John Schoenick [:johns]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b27161e4101
https://hg.mozilla.org/integration/mozilla-inbound/rev/179a0eb50196
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cb4d46a9873
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a0ce7ea232e
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc34ee9ac52c
https://hg.mozilla.org/integration/mozilla-inbound/rev/34eebb890abd
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4c6e158007
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8a9d628260
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a8d5c15dcf8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed37e3099398
https://hg.mozilla.org/integration/mozilla-inbound/rev/500bca3a7429
https://hg.mozilla.org/integration/mozilla-inbound/rev/86ec95551fd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2c5a92f08f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0dc979d76a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/6260cb355dcd

Full try push:
https://tbpl.mozilla.org/?tree=Try&rev=03b4e71563b5

Follow up push after review changes for good measure:
https://tbpl.mozilla.org/?tree=Try&rev=87dc9e128802

Comment 115

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6260cb355dcd
https://hg.mozilla.org/mozilla-central/rev/f0dc979d76a2
https://hg.mozilla.org/mozilla-central/rev/4a2c5a92f08f
https://hg.mozilla.org/mozilla-central/rev/86ec95551fd5
https://hg.mozilla.org/mozilla-central/rev/500bca3a7429
https://hg.mozilla.org/mozilla-central/rev/ed37e3099398
https://hg.mozilla.org/mozilla-central/rev/0a8d5c15dcf8
https://hg.mozilla.org/mozilla-central/rev/ea8a9d628260
https://hg.mozilla.org/mozilla-central/rev/5e4c6e158007
https://hg.mozilla.org/mozilla-central/rev/34eebb890abd
https://hg.mozilla.org/mozilla-central/rev/bc34ee9ac52c
https://hg.mozilla.org/mozilla-central/rev/9a0ce7ea232e
https://hg.mozilla.org/mozilla-central/rev/5cb4d46a9873
https://hg.mozilla.org/mozilla-central/rev/179a0eb50196
https://hg.mozilla.org/mozilla-central/rev/3b27161e4101

Comment 116

[nick.lemouton]

Has this landed? I'm running FF 34 and I still can't see this working

Comment 117

[Andrew McCreight [:mccr8]]

(In reply to nick.lemouton from comment #116)
> Has this landed? I'm running FF 34 and I still can't see this working

It has landed, but it is preffed off by default.  See bug 1017875 for remaining work needed to enable it.

Comment 118

[Henri Sivonen (:hsivonen)]

It looks like the addition of PICTURE to ElementName.java was not done by actually re-running ElementName.java's self-regeneration code and, as a result, the sort order is wrong. ಠ_ಠ I guess we're lucky if binary search has still managed to find the right item.

Anyway, if weird <picture> parsing behavior shows up in old builds, the table will get sorted correctly as a side effect of bug 1266495.

Comment 119

[John Schoenick [:johns]]

(In reply to Henri Sivonen (:hsivonen) from comment #118)
> It looks like the addition of PICTURE to ElementName.java was not done by
> actually re-running ElementName.java's self-regeneration code and, as a
> result, the sort order is wrong. ಠ_ಠ I guess we're lucky if binary search
> has still managed to find the right item.
> 
> Anyway, if weird <picture> parsing behavior shows up in old builds, the
> table will get sorted correctly as a side effect of bug 1266495.

Doesn't look conspicuous at all!
>      252135604,
> +    251841204,
>      252317348,

(this was certainly my fault, I don't remember exactly what I did, but I suspect rebase/merge fail)

Comment 122

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/picture