The default bug view has changed. See this FAQ.
Bug 870022 (picture)

Implement `picture` element

RESOLVED FIXED in mozilla33

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: Mat Marquis, Assigned: johns)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [v3.3 try: https://tbpl.mozilla.org/?tree=Try&rev=03b4e71563b5], URL)

Attachments

(15 attachments, 58 obsolete attachments)

11.76 KB, patch
jst
: review+
Details | Diff | Splinter Review
6.16 KB, patch
jst
: review+
Details | Diff | Splinter Review
3.63 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
5.58 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
8.01 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
5.43 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
12.60 KB, patch
johns
: review+
Details | Diff | Splinter Review
4.78 KB, patch
johns
: review+
Details | Diff | Splinter Review
3.54 KB, patch
johns
: review+
Details | Diff | Splinter Review
15.62 KB, patch
johns
: review+
Details | Diff | Splinter Review
60.47 KB, patch
johns
: review+
Details | Diff | Splinter Review
10.65 KB, patch
johns
: review+
Details | Diff | Splinter Review
26.47 KB, patch
johns
: review+
Details | Diff | Splinter Review
25.07 KB, patch
johns
: review+
Details | Diff | Splinter Review
4.94 KB, patch
johns
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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/
A small data point: 
http://blog.yoav.ws/2013/05/How-Big-Is-Art-Direction
Depends on: 870021
(Reporter)

Comment 2

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

4 years ago
+1 would love to see this implemented.

Comment 4

4 years ago
Let's make it happen!

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

4 years ago
Status: NEW → UNCONFIRMED
Ever confirmed: false
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Updated

3 years ago
Assignee: nobody → jschoenick
Keywords: dev-doc-needed
(Assignee)

Comment 5

3 years ago
I'm working on the implementation for this now - I hope to have something stood up in a few weeks.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

3 years ago
Alias: picture
(Assignee)

Comment 6

3 years ago
Created attachment 8417609 [details] [diff] [review]
[WIP v1] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage
(Assignee)

Comment 7

3 years ago
Created attachment 8417610 [details] [diff] [review]
[WIP v1] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar
(Assignee)

Comment 8

3 years ago
Created attachment 8417612 [details] [diff] [review]
[WIP v1] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing
(Assignee)

Comment 9

3 years ago
Created attachment 8417613 [details] [diff] [review]
[WIP v1] Part 3.2 - Add sizes support to ResponsiveImageSelector
(Assignee)

Comment 10

3 years ago
Created attachment 8417614 [details] [diff] [review]
[WIP v1] Part 4.1 - Add sizes to HTMLImageElement & atoms
(Assignee)

Comment 11

3 years ago
Created attachment 8417615 [details] [diff] [review]
[WIP v1] Part 4.2 - Teach parser about sizes attr (needs generate)
(Assignee)

Comment 12

3 years ago
Created attachment 8417616 [details] [diff] [review]
[WIP v1] Part 4.3 - Teach parser about sizes attr (generated bits)
(Assignee)

Comment 13

3 years ago
Created attachment 8417617 [details] [diff] [review]
[WIP v1] Part 5.1 - Add HTMLPictureElement & atom
(Assignee)

Comment 14

3 years ago
Created attachment 8417618 [details] [diff] [review]
[WIP v1] Part 5.2 - Add sizes/srcset to HTMLSourceElement
(Assignee)

Comment 15

3 years ago
Created attachment 8417619 [details] [diff] [review]
[WIP v1] Part 5.3 - Teach parser about <picture> (non-generated bits)
(Assignee)

Comment 16

3 years ago
Created attachment 8417620 [details] [diff] [review]
[WIP v1] Part 5.4 - Teach parser about <picture> (generated bits)
(Assignee)

Comment 17

3 years ago
Created attachment 8417621 [details] [diff] [review]
[WIP v1] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement
(Assignee)

Comment 18

3 years ago
Created attachment 8417622 [details] [diff] [review]
[WIP v1] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes
(Assignee)

Comment 19

3 years ago
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.
(Assignee)

Comment 20

3 years ago
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
(Assignee)

Comment 21

3 years ago
Created attachment 8426638 [details] [diff] [review]
[v2] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage
Attachment #8417609 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
Created attachment 8426639 [details] [diff] [review]
[v2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar
Attachment #8417610 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
Created attachment 8426641 [details] [diff] [review]
[v2] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing
Attachment #8417612 - Attachment is obsolete: true
(Assignee)

Comment 24

3 years ago
Created attachment 8426642 [details] [diff] [review]
[v2] Part 3.2 - Add sizes support to ResponsiveImageSelector
Attachment #8417613 - Attachment is obsolete: true
(Assignee)

Comment 25

3 years ago
Created attachment 8426643 [details] [diff] [review]
[v2] Part 4.1 - Add sizes to HTMLImageElement & atoms
Attachment #8417614 - Attachment is obsolete: true
(Assignee)

Comment 26

3 years ago
Created attachment 8426644 [details] [diff] [review]
[v2] Part 4.2 - Teach parser about sizes attr (non-generated bits)
Attachment #8417615 - Attachment is obsolete: true
(Assignee)

Comment 27

3 years ago
Created attachment 8426645 [details] [diff] [review]
[v2] Part 4.2 - Teach parser about sizes attr (generated bits)
(Assignee)

Comment 28

3 years ago
Created attachment 8426647 [details] [diff] [review]
[v2] Part 5.1 - Add HTMLPictureElement & atom
Attachment #8417617 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
Created attachment 8426649 [details] [diff] [review]
[v2] Part 5.2 - Add sizes/srcset to HTMLSourceElement
Attachment #8417618 - Attachment is obsolete: true
(Assignee)

Comment 30

3 years ago
Created attachment 8426650 [details] [diff] [review]
[v2] Part 5.3 - Teach parser about <picture> (non-generated bits)
Attachment #8417619 - Attachment is obsolete: true
(Assignee)

Comment 31

3 years ago
Created attachment 8426651 [details] [diff] [review]
[v2] Part 5.4 - Teach parser about <picture> (generated bits)
Attachment #8417620 - Attachment is obsolete: true
(Assignee)

Comment 32

3 years ago
Created attachment 8426652 [details] [diff] [review]
[v2] Part 5.5 - Add <picture> to nsHTMLEditUtils
(Assignee)

Comment 33

3 years ago
Created attachment 8426653 [details] [diff] [review]
[v2] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement
Attachment #8417621 - Attachment is obsolete: true
(Assignee)

Comment 34

3 years ago
Created attachment 8426654 [details] [diff] [review]
[v2] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes
Attachment #8417622 - Attachment is obsolete: true
(Assignee)

Comment 35

3 years ago
Created attachment 8426655 [details] [diff] [review]
[v2] Part 8 - Pref picture off behind dom.image.picture.enabled
(Assignee)

Comment 36

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 37

3 years ago
Created attachment 8428102 [details] [diff] [review]
[v3] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage
Attachment #8426638 - Attachment is obsolete: true
(Assignee)

Comment 38

3 years ago
Created attachment 8428103 [details] [diff] [review]
[v3] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar
Attachment #8426639 - Attachment is obsolete: true
(Assignee)

Comment 39

3 years ago
Created attachment 8428104 [details] [diff] [review]
[v3] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing
Attachment #8426641 - Attachment is obsolete: true
(Assignee)

Comment 40

3 years ago
Created attachment 8428105 [details] [diff] [review]
[v3] Part 3.2 - Add sizes support to ResponsiveImageSelector
Attachment #8426642 - Attachment is obsolete: true
(Assignee)

Comment 41

3 years ago
Created attachment 8428106 [details] [diff] [review]
[v3] Part 4.1 - Add sizes to HTMLImageElement & atoms
Attachment #8426643 - Attachment is obsolete: true
(Assignee)

Comment 42

3 years ago
Created attachment 8428107 [details] [diff] [review]
[v3] Part 4.2 - Teach parser about sizes attr (non-generated bits)
Attachment #8426644 - Attachment is obsolete: true
(Assignee)

Comment 43

3 years ago
Created attachment 8428108 [details] [diff] [review]
[v3] Part 4.3 - Teach parser about sizes attr (generated bits)
Attachment #8417616 - Attachment is obsolete: true
Attachment #8426645 - Attachment is obsolete: true
(Assignee)

Comment 44

3 years ago
Created attachment 8428109 [details] [diff] [review]
[v3] Part 5.1 - Add HTMLPictureElement & atom
Attachment #8426647 - Attachment is obsolete: true
(Assignee)

Comment 45

3 years ago
Created attachment 8428110 [details] [diff] [review]
[v3] Part 5.2 - Add sizes/srcset to HTMLSourceElement
Attachment #8426649 - Attachment is obsolete: true
(Assignee)

Comment 46

3 years ago
Created attachment 8428111 [details] [diff] [review]
[v3] Part 5.3 - Teach parser about <picture> (non-generated bits)
(Assignee)

Comment 47

3 years ago
Created attachment 8428112 [details] [diff] [review]
[v3] Part 5.4 - Teach parser about <picture> (generated bits)
Attachment #8426650 - Attachment is obsolete: true
Attachment #8426651 - Attachment is obsolete: true
(Assignee)

Comment 48

3 years ago
Created attachment 8428113 [details] [diff] [review]
[v3] Part 5.5 - Add <picture> to nsHTMLEditUtils
Attachment #8426652 - Attachment is obsolete: true
(Assignee)

Comment 49

3 years ago
Created attachment 8428114 [details] [diff] [review]
[v3] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement
Attachment #8426653 - Attachment is obsolete: true
(Assignee)

Comment 50

3 years ago
Created attachment 8428115 [details] [diff] [review]
[v3] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes
Attachment #8426654 - Attachment is obsolete: true
(Assignee)

Comment 51

3 years ago
Created attachment 8428116 [details] [diff] [review]
[v3] Part 8 - Pref picture off behind dom.image.picture.enabled
Attachment #8426655 - Attachment is obsolete: true
(Assignee)

Comment 52

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

Comment 53

3 years ago
Created attachment 8428131 [details] [diff] [review]
[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.
Attachment #8428131 - Flags: review?(roc)
(Assignee)

Comment 54

3 years ago
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*.
Attachment #8428131 - Attachment is obsolete: true
Attachment #8428131 - Flags: review?(roc)
(Assignee)

Comment 55

3 years ago
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
Whiteboard: [v3 try: https://tbpl.mozilla.org/?tree=Try&rev=17b503f372f9] → [v3.1 try: https://tbpl.mozilla.org/?tree=Try&rev=263bdc53b84d]
(Assignee)

Comment 56

3 years ago
Created attachment 8431113 [details] [diff] [review]
[v3.2] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage
Attachment #8428102 - Attachment is obsolete: true
(Assignee)

Comment 57

3 years ago
Created attachment 8431114 [details] [diff] [review]
[v3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar
Attachment #8428103 - Attachment is obsolete: true
(Assignee)

Comment 58

3 years ago
Created attachment 8431116 [details] [diff] [review]
[v3.2] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing
Attachment #8428104 - Attachment is obsolete: true
(Assignee)

Comment 59

3 years ago
Created attachment 8431117 [details] [diff] [review]
[v3.2] Part 3.2 - Add sizes support to ResponsiveImageSelector
Attachment #8428105 - Attachment is obsolete: true
(Assignee)

Comment 60

3 years ago
Created attachment 8431118 [details] [diff] [review]
[v3.2] Part 4.1 - Add sizes to HTMLImageElement & atoms
Attachment #8428106 - Attachment is obsolete: true
(Assignee)

Comment 61

3 years ago
Created attachment 8431119 [details] [diff] [review]
[v3.2] Part 4.2 - Teach parser about sizes attr (non-generated bits)
Attachment #8428107 - Attachment is obsolete: true
(Assignee)

Comment 62

3 years ago
Created attachment 8431120 [details] [diff] [review]
[v3.2] Part 4.3 - Teach parser about sizes attr (generated bits)
Attachment #8428108 - Attachment is obsolete: true
(Assignee)

Comment 63

3 years ago
Created attachment 8431121 [details] [diff] [review]
[v3.2] Part 5.1 - Add HTMLPictureElement & atom
Attachment #8428109 - Attachment is obsolete: true
(Assignee)

Comment 64

3 years ago
Created attachment 8431122 [details] [diff] [review]
[v3.2] Part 5.2 - Add sizes/srcset to HTMLSourceElement
Attachment #8428110 - Attachment is obsolete: true
(Assignee)

Comment 65

3 years ago
Created attachment 8431123 [details] [diff] [review]
[v3.2] Part 5.3 - Teach parser about <picture> (non-generated bits)
Attachment #8428111 - Attachment is obsolete: true
(Assignee)

Comment 66

3 years ago
Created attachment 8431124 [details] [diff] [review]
[v3.2] Part 5.4 - Teach parser about <picture> (generated bits)
Attachment #8428112 - Attachment is obsolete: true
(Assignee)

Comment 67

3 years ago
Created attachment 8431126 [details] [diff] [review]
[v3.2] Part 5.5 - Add <picture> to nsHTMLEditUtils
Attachment #8428113 - Attachment is obsolete: true
(Assignee)

Comment 68

3 years ago
Created attachment 8431127 [details] [diff] [review]
[v3.2] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement
Attachment #8428114 - Attachment is obsolete: true
(Assignee)

Comment 69

3 years ago
Created attachment 8431128 [details] [diff] [review]
[v3.2] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes
Attachment #8428115 - Attachment is obsolete: true
(Assignee)

Comment 70

3 years ago
Created attachment 8431129 [details] [diff] [review]
[v3.2] Part 8 - Pref picture off behind dom.image.picture.enabled
Attachment #8428116 - Attachment is obsolete: true
(Assignee)

Comment 71

3 years ago
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
Whiteboard: [v3.1 try: https://tbpl.mozilla.org/?tree=Try&rev=263bdc53b84d] → [v3.2 try: https://tbpl.mozilla.org/?tree=Try&rev=b79964d32edc]
(Assignee)

Comment 72

3 years ago
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.
Attachment #8431113 - Flags: review?(jst)
(Assignee)

Updated

3 years ago
Blocks: 1017875
(Assignee)

Updated

3 years ago
Blocks: 1017878
(Assignee)

Comment 73

3 years ago
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)
Attachment #8431114 - Flags: review?(bzbarsky)
(Assignee)

Comment 74

3 years ago
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
Attachment #8431116 - Flags: review?(jst)
(Assignee)

Comment 75

3 years ago
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.
Attachment #8431117 - Flags: review?(jst)
(Assignee)

Comment 76

3 years ago
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
Attachment #8431118 - Flags: superreview?(jst)
Attachment #8431118 - Flags: review?(jst)
(Assignee)

Comment 77

3 years ago
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
Attachment #8431119 - Flags: review?(hsivonen)
(Assignee)

Comment 78

3 years ago
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
Attachment #8431120 - Flags: review?(hsivonen)
(Assignee)

Comment 79

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

Adds stub HTMLPictureElement
Attachment #8431121 - Flags: superreview?(jst)
Attachment #8431121 - Flags: review?(jst)
(Assignee)

Comment 80

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

Adds srcset and sizes to <source>
Attachment #8431122 - Flags: superreview?(jst)
Attachment #8431122 - Flags: review?(jst)
(Assignee)

Comment 81

3 years ago
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
Attachment #8431123 - Flags: review?(hsivonen)
(Assignee)

Comment 82

3 years ago
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
Attachment #8431124 - Flags: review?(hsivonen)
(Assignee)

Comment 83

3 years ago
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
Attachment #8431128 - Flags: review?(jst)
(Assignee)

Comment 84

3 years ago
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
Attachment #8431129 - Flags: review?(jst)
(Assignee)

Comment 85

3 years ago
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.)
Attachment #8431127 - Flags: review?(cpearce)
(Assignee)

Comment 86

3 years ago
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
Attachment #8431126 - Flags: review?(ehsan)
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.
Attachment #8431126 - Flags: review?(ehsan) → review-
Comment on attachment 8431113 [details] [diff] [review]
[v3.2] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage

Looks good! r=jst
Attachment #8431113 - Flags: review?(jst) → review+

Updated

3 years ago
Attachment #8431116 - Flags: review?(jst) → review+
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
Attachment #8431117 - Flags: review?(jst) → review+
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?
Attachment #8431119 - Flags: review?(hsivonen) → review+
Attachment #8431120 - Flags: review?(hsivonen) → review+
Attachment #8431124 - Flags: review?(hsivonen) → review+
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.)
Attachment #8431123 - Flags: review?(hsivonen) → review+
(Assignee)

Comment 92

3 years ago
(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 on attachment 8431118 [details] [diff] [review]
[v3.2] Part 4.1 - Add sizes to HTMLImageElement & atoms

># HG changeset patch
># User John Schoenick <jschoenick@mozilla.com>
>
>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
Attachment #8431118 - Flags: superreview?(jst)
Attachment #8431118 - Flags: superreview+
Attachment #8431118 - Flags: review?(jst)
Attachment #8431118 - Flags: review+

Updated

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

Updated

3 years ago
Attachment #8431122 - Flags: superreview?(jst)
Attachment #8431122 - Flags: superreview+
Attachment #8431122 - Flags: review?(jst)
Attachment #8431122 - Flags: review+
> it could be inlined

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

Updated

3 years ago
Attachment #8431128 - Flags: review?(jst) → review+
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
Attachment #8431129 - Flags: review?(jst) → review+
Attachment #8431127 - Flags: review?(cpearce) → review+
(Assignee)

Comment 96

3 years ago
Created attachment 8434452 [details] [diff] [review]
[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
Attachment #8431126 - Attachment is obsolete: true
Attachment #8434452 - Flags: review?(ehsan)
(Assignee)

Comment 97

3 years ago
Created attachment 8434464 [details] [diff] [review]
[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)
Attachment #8431114 - Attachment is obsolete: true
Attachment #8431114 - Flags: review?(bzbarsky)
Attachment #8434464 - Flags: review?(bzbarsky)
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!
Attachment #8434452 - Flags: review?(ehsan) → review+
(Assignee)

Comment 99

3 years ago
Created attachment 8434499 [details] [diff] [review]
[v3.3] Part 3.2 - Add sizes support to ResponsiveImageSelector. r=jst

Addressed review comment
Attachment #8431117 - Attachment is obsolete: true
Attachment #8434499 - Flags: review+
(Assignee)

Comment 100

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

Folded pref (part 8) in
Attachment #8431118 - Attachment is obsolete: true
Attachment #8434521 - Flags: superreview+
Attachment #8434521 - Flags: review+
(Assignee)

Comment 101

3 years ago
Created attachment 8434523 [details] [diff] [review]
[v3.3] Part 5.1 - Add HTMLPictureElement & atom. r=jst, sr=jst

Folded pref (part 8) in
Attachment #8431121 - Attachment is obsolete: true
Attachment #8434523 - Flags: superreview+
Attachment #8434523 - Flags: review+
(Assignee)

Comment 102

3 years ago
Created attachment 8434525 [details] [diff] [review]
[v3.3] Part 5.2 - Add sizes/srcset to HTMLSourceElement. r=jst, sr=jst

Folded pref (Part 8) in
Attachment #8431122 - Attachment is obsolete: true
Attachment #8434525 - Flags: superreview+
Attachment #8434525 - Flags: review+
(Assignee)

Comment 103

3 years ago
Created attachment 8434527 [details] [diff] [review]
Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes. r=jst

Folded in pref (Part 8)
Attachment #8431128 - Attachment is obsolete: true
Attachment #8431129 - Attachment is obsolete: true
Attachment #8434527 - Flags: review+
(Assignee)

Comment 104

3 years ago
Created attachment 8434530 [details] [diff] [review]
Add test for dom.image.picture.enabled

Test to ensure dom.image.picture.enabled properly disables everything
(Assignee)

Updated

3 years ago
Attachment #8434530 - Flags: review?(bzbarsky)
(Assignee)

Updated

3 years ago
Blocks: 1023519
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.
Attachment #8434464 - Flags: review?(bzbarsky) → review+
Comment on attachment 8434530 [details] [diff] [review]
Add test for dom.image.picture.enabled

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

Comment 107

3 years ago
Created attachment 8445536 [details] [diff] [review]
[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.
Attachment #8434464 - Attachment is obsolete: true
Attachment #8445536 - Flags: review+
(Assignee)

Comment 108

3 years ago
Created attachment 8445537 [details] [diff] [review]
[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
Attachment #8431120 - Attachment is obsolete: true
Attachment #8445537 - Flags: review+
(Assignee)

Comment 109

3 years ago
Created attachment 8445538 [details] [diff] [review]
[v3.3.1] Part 5.1 - Add HTMLPictureElement & atom. r=jst, sr=jst

rebased for nsINodeInfo rename, carrying over r,sr=jst
Attachment #8434523 - Attachment is obsolete: true
Attachment #8445538 - Flags: review+
(Assignee)

Comment 110

3 years ago
Created attachment 8445539 [details] [diff] [review]
[v3.3.1] Part 5.4 - Teach parser about <picture> (generated bits). r=hsivonen

Regenerated due to other landings, carrying over r=hsivonen
Attachment #8431124 - Attachment is obsolete: true
Attachment #8445539 - Flags: review+
(Assignee)

Comment 111

3 years ago
Created attachment 8445540 [details] [diff] [review]
[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
Attachment #8434527 - Attachment is obsolete: true
Attachment #8445540 - Flags: review+
(Assignee)

Comment 112

3 years ago
Created attachment 8445542 [details] [diff] [review]
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)
Attachment #8434530 - Attachment is obsolete: true
Attachment #8445542 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8445537 - Attachment description: Part 4.3 - Teach parser about sizes attr (generated bits). r=hsivonen → [v3.3.1] Part 4.3 - Teach parser about sizes attr (generated bits). r=hsivonen
(Assignee)

Updated

3 years ago
Attachment #8445536 - Attachment description: [v3.3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar. → [v3.3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar. r=bz
> I updated the comment, but let me know if there's a better way to do this.

Seems fine, thanks.
(Assignee)

Comment 114

3 years ago
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
Whiteboard: [v3.2 try: https://tbpl.mozilla.org/?tree=Try&rev=b79964d32edc] → [v3.3 try: https://tbpl.mozilla.org/?tree=Try&rev=03b4e71563b5]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1030606
(Assignee)

Updated

3 years ago
Blocks: 1037643

Comment 116

3 years ago
Has this landed? I'm running FF 34 and I still can't see this working
(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.
Depends on: 1134131
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.
(Assignee)

Comment 119

11 months ago
(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)
You need to log in before you can comment on or make changes to this bug.