215083 - (content-url-element) Support CSS3 content property replacement of element (rather than pseudo-element)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[SvendTofte]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030804
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030804

Using the css property "content", to replace an element
  acronym , abbr { content:attr(title); }
doesn't work. Only when using :before or :after, does the content display. Opera
seems to be able to do it, and the W3 working draft seem pretty clear, on how it
should work.

http://www.w3.org/TR/css3-content/#content

Maybe this shouldn't be filed, since the spec isn't a TR yet? If so, sorry for
wasting your time :)

Reproducible: Always

Steps to Reproduce:
1. Go to url

Actual Results:  
Mozilla didn't replace the element content, with the contents of the responding
elements title attribute.

Expected Results:  
It should replace the element content, with the value of it's title attribute.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This spec is nowhere near Candidate Recommendation and is thus not ready for
implementation.  I think it just clutters up bug lists to have bugs on such
things, so marking as wontfix, for now, anyway.

Comment 2

[SvendTofte]

Cool enough, just wanted to make sure what the policy was on yet-TR specs :)

Comment 3

[Bill Mason]

*** Bug 215403 has been marked as a duplicate of this bug. ***

Comment 4

[Anne (:annevk)]

Attachment: Test Case

Sorry for the duplicate. I searched Bugzilla and I didn't found any bug/feature
request about this.

About the test case:
 The text should be: "The rule is applied", as it is in Opera 7.11.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'll reopen, but I don't want to encourage this, because it just makes real bugs
harder to find, and I certainly don't want a bug for every feature we haven't
implemented.

Comment 6

[Justin Wood (:Callek)]

*** Bug 219417 has been marked as a duplicate of this bug. ***

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See also bug 309217, for the css3 content property's fallback lists.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 315391 has been marked as a duplicate of this bug. ***

Comment 11

[Martin Suchan]

Sorry for my duplicate, but anyway. This issue has been opened 7 years ago(!) and it is still unsolved? The usage of 'label' in 'option' tag is well defined since 1999 in HTML 4.01 Specification and should be working, imho:
http://test.suchan.cz/test.html
http://www.w3.org/TR/html401/interact/forms.html#h-17.6
OPTION Attribute definitions
When rendering a menu choice, user agents should use the value of the label attribute of the OPTION element as the choice. If this attribute is not specified, user agents should use the contents of the OPTION element.

Comment 13

[Alexei]

Well, when you fix it?

Comment 15

[j.j.]

Alexei, (In reply to Alexei from comment #13)
> Well, when you fix it?

Not before there is s specification. 
You re-reported this twice no, please stop it! (bug 689088 comment 5)

Comment 16

[Martin Suchan]

At least the 'label' attribute in 'option' tags should be fixed (it's part of basic HTML 4.01 specification) - it works in IE, Chrome, Safari and Opera, so why not in FF?
https://bugzilla.mozilla.org/show_bug.cgi?id=40545
Compare this page in FF and Chrome/IE http://jsfiddle.net/jPKQz/3/

Comment 17

[Alexei]

Firefox is getting worse! Soon come to the point that this browser will be ****.

Comment 18

[Lea Verou]

(In reply to Alexei from comment #17)
> Firefox is getting worse! Soon come to the point that this browser will be
> ****.

O RLY? Which other browser do you think implements this? Lemme give you a hint: Only Opera.

Comment 19

[Martin Suchan]

This bug 215083 blocks(?) bug No. 40545: 'labels' for 'option' tags, which is something implemented in all other browsers except FF ;)

Comment 20

[j.j.]

People, please read
  https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
before adding more comments. (Reading this ideally would prompt you to stop commenting)

(And comments about bug 40545 are mostly unrelated here. Fixing bug 215083 is not the one and only option to fix that bug.)

Comment 21

[Alexei]

Support all browsers: Safari, Opera, Google Chrome and even IE version 10.

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Alexei from comment #21)
> Support all browsers: Safari, Opera, Google Chrome and even IE version 10.

For ::before and ::after, yes. This bug is explicitly about div { content: "Hi"; }, which is currently only supported in Opera.

Comment 23

[philippe (part-time)]

(In reply to Ms2ger from comment #22)
> (In reply to Alexei from comment #21)
> > Support all browsers: Safari, Opera, Google Chrome and even IE version 10.
> 
> For ::before and ::after, yes. This bug is explicitly about div { content:
> "Hi"; }, which is currently only supported in Opera.

WebKit (Safari 5.1, Chrome something) supports div { content: url(path/to/image/png); }; but div { content: 'foo bar'; } is unfortunately not yet supported.

see also this thread: http://lists.w3.org/Archives/Public/www-style/2011Nov/thread.html#msg451

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to philippe from comment #23)
> (In reply to Ms2ger from comment #22)
> > (In reply to Alexei from comment #21)
> > > Support all browsers: Safari, Opera, Google Chrome and even IE version 10.
> > 
> > For ::before and ::after, yes. This bug is explicitly about div { content:
> > "Hi"; }, which is currently only supported in Opera.
> 
> WebKit (Safari 5.1, Chrome something) supports div { content:
> url(path/to/image/png); }; but div { content: 'foo bar'; } is unfortunately
> not yet supported.

I didn't know that; thanks for the correction.

Comment 25

[Alexei]

Among the ones you freaks who do not want to add support for css content.

Comment 26

[Alexei]

Stable draft: http://www.w3.org/TR/css3-content/

Comment 27

[j.j.]

(In reply to Alexei from comment #26)
> Stable draft: http://www.w3.org/TR/css3-content/

This draft is not stable and out of date. Current draft is in URL field.

(Alexei, sorry, some of your comments here and in other bugs aren't helpful at all. You can not speed up the implementation process this way. Please read
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html )

Comment 29

[Jim Michaels]

for reference, this is in css 2.1 TR as well. not sure why 2.1 is in TR right now.
http://www.w3.org/TR/CSS21/generate.html#propdef-content
I see no date on this document.

Comment 30

[Jim Michaels]

by the way, chrome doesn't implement this either.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Jim Michaels from comment #29)
> for reference, this is in css 2.1 TR as well. not sure why 2.1 is in TR
> right now.
> http://www.w3.org/TR/CSS21/generate.html#propdef-content
> I see no date on this document.

No, in CSS 2.1 it's only for the :before and :after pseudo-elements.

Comment 32

[Jim Michaels]

this is the best information I can give you on what I have now, since ff doesn't tell you anything other than "14.0".

------properties-------
navigator.appCodeName = Mozilla
navigator.appName = Netscape
navigator.appVersion = 5.0 (Windows)
navigator.language = en-US
navigator.mimeTypes = [object MimeTypeArray]
navigator.platform = Win32
navigator.oscpu = Windows NT 5.1
navigator.vendor = 
navigator.vendorSub = 
navigator.product = Gecko
navigator.productSub = 20100101
navigator.plugins = [object PluginArray]
navigator.userAgent = Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20100101 Firefox/14.0
navigator.cookieEnabled = true
navigator.onLine = true
navigator.buildID = 20120628060610

Comment 34

[Alexei03a]

Test suite - [http://alexei03a.github.com/cssc/].

Comment 35

[Gordon P. Hemsley [:GPHemsley]]

@dbaron: What would it take to implement this? I note the spec is no longer being maintained, and "should not be used as a guide for implementations". Is there a problem with the spec? Would you be available as a mentor?

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the first step to implementing this would be to figure out what the state of the spec is -- how solid it is, how much working group consensus there is behind it, etc.  I think it was probably reasonably well-specified, but it might not all make sense anymore.

I'd be willing to mentor somebody doing that, I think, though they'd need to be willing to do that without extremely detailed guidance.


Implementation wouldn't be trivial; it would require some to frame construction code, figuring out how it should interact with things like image loading, frame loading, and loading of any other resource types that would be supported (video?).  (I wonder if the feature would work better if, instead of url(), the arguments made it clear whether the resource was an image or frame before it was loaded.  That would certainly be helpful from an implementation perspective, and may well be more efficient.)

Comment 37

[Masaya Iseki[:isk](UTC+9)]

I'm interested in solving this bug.
But I don't have enough knowledge to solve it.

If you don't mind, would you please tell me the overview how to implement it?
I have read the document[1] but this document is too old. 

[1]http://dxr.mozilla.org/mozilla-central/source/layout/doc

Comment 38

[Daniel Holbert [:dholbert]]

FWIW, fantasai rewrote/updated the (2003-era) css-content-3 spec, and announced it back in June on www-style:
 https://lists.w3.org/Archives/Public/www-style/2016Jun/0137.html

Hopefully the modern spec has fewer of the possible-pitfalls that dbaron was hinting at in comment 36, and might be more directly implementable...

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, and since I accidentally cleared the needinfo? when adding the "See Also" in response to dholbert's comment -- I guess the real response to comment 37 is that this isn't a good choice of bug for somebody who hasn't already written some substantial Gecko code before.  Figuring out a reasonable plan for implementing it is at least multiple hours of work if not multiple days.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Daniel Holbert [:dholbert] from comment #38)
> Hopefully the modern spec has fewer of the possible-pitfalls that dbaron was
> hinting at in comment 36, and might be more directly implementable...

As I pointed out in https://github.com/w3c/csswg-drafts/issues/821 , I don't think the spec is even clear enough to *tell* whether those pitfalls are still present.  (For example, does the spec expect url() to load images, videos, iframes, etc., or only images?  What does Blink do there?)

Comment 41

[acterhd]

I also waiting support around 4 years.

Comment 42

[Jonathan Watt [:jwatt]]

Attachment: content:url() testcase

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Transferring webcompat markings from duplicate bug 1372086.

Comment 45

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 46

[Maire Reavy [:mreavy]]

Cameron -- As I mentioned in the Layout standup meeting last week, this is a P1 webcompat bug that I really want to see solved.  Can you assess how you'd approach this?  Would you be available to mentor the assignee of it?  Thanks.

Comment 47

[Emilio Cobos Álvarez (:emilio)]

I could probably take this, given this is frame constructor work.

However I think google is not being fair requesting us to implement this in order to ship the tier1 page on android when there are more suitable replacements for this, and the way this is implemented in both Blink and WebKit is kind of a hack.

Comment 48

[Xidorn Quan [:xidorn] UTC+10]

I've raised several new spec issues after reading the spec and playing with the implementation of Chrome and Safari:
* w3c/csswg-drafts#2656
* w3c/csswg-drafts#2657

It seems to me that Blink and WebKit currently only implement <content-replacement> for element (that a single. That doesn't feel terribly hard to implement in terms of frame constructing. We probably just need a new frame class, or maybe just reuse the frame of <img>, and yield the frame when we meet content property with <content-replacement> value on an element.

It may, though, need some nontrivial work on the style system, due to that we may not be generating frames for elements which are not in a display:none subtree. Maybe it's not that hard, though. We can probably just take <iframe>, <video>, and maybe other replaced element as a reference for how to suppress children.

But it's not clear to me whether we should go this way... Probably need to have some consensus from the working group on the two issues I mentioned above.

If the working group can agree on those issues, ideally we should shrink the feature set of css-content-3 to just include `content: <content-replacement>` for element in addition to what CSS 2 already has, and defer others into the next level...

TBH, I'm not very excited about the current status of the implementation of Blink and WebKit. But if webcompat requires us to do so...

(An alternative approach for the webcompat user story of this bug is to ask Google not to use this experimental feature in their tier-1 UI...)

Comment 49

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #48)
> It seems to me that Blink and WebKit currently only implement
> <content-replacement> for element (that a single.

(that a single url means a <image> replacing the content).

Comment 50

[Cameron McCormack (:heycam)]

I'm happy to mentor someone who wants to look at this.

The first thing to do would be to verify what parts of the spec we actually need to implement.  I suspect it is content:url(...) on certain elements with sizing behavior of some sort.  Bug 1239922 comment 11 mentions this, but it would be good to know what other pages are relying on this feature and to what extent.

Once we've determined what features from the spec we need, we should do some thorough testing to see how Chrome (and other browsers, if any now do support this) behave.  Then, compare this with what the spec requires, make some judgements as to what actually makes sense, and then file issues on the spec accordingly.

Some brief testing in Chrome shows that not all elements can have their contents replaced, for example it works on an <img>, but not on a <button> or an <iframe>:

  http://mcc.id.au/temp/content.html

For the implementation, I think there are two ways we could go.

One approach would be to model this like we do for ::before / ::after generated content, where we create native anonymous content to represent the image.  This would be created via CreateGenConImageContent, which is what we use for `::before { content: url() }`, and which is already set up to create an nsImageFrame and handle image loading due to nsGenConImageContent (the concrete DOM node class we use) implementing nsImageLoadingContent.

We would need to tweak how the sizing works somehow to look at the parent real element's sizing proprerties somehow, which might be tricky.  How do we make `span { content: url(...); width: 200px; }` influence the size of the child gencon element?

We would need to suppress frames for the actual content of the element.

I suspect there would be other issues with using generated content here, e.g. a border doesn't go around an entire generated content image in ::before / ::after, but it seems to work fine in Chrome for content:url() on an element.

(If in the future the spec allowed `span { content: "some text"; }` as well as just an image, then this approach would easily extend to supporting that.  But given the need to make width/height on the element influence the image's size, I don't think the spec will go in that direction.)

The second approach would be to make the element itself get an nsImageFrame, by adjusting nsImageFrame::ShouldCreateImageFrameFor to return true for elements that have a content:url() value.  An nsImageFrame for the element is what we do for <img>.  We wouldn't have any issues with mismatches between the sizes of the real element's frame and the gencon element's frame, since we've just got the one element.

We would need an instance of nsImageLoadingContent somewhere, and I don't think we want to make all HTML elements inherit from this.  Maybe we can create one and stuff it in the DOM element's property table.  nsImageLoadingContent currently makes some assumptions that it is implemented on an nsIContent -- see the do_QueryInterface calls in nsImageLoadingContent.cpp.  But they should probably all be using nsImageLoadingContent::AsContent anyway, which we could override to return our element.  I'm not sure what the best way to manage the lifetime of this nsImageLoadingContent object would be.  Perhaps the nsImageFrame's constructor/destructor could be in charge of calling something on the element to add/remove it.

<img> elements already suppress the frames for any children they might have, but I'm not sure how that happens.  We might still need to do something to make that work here too, or it might work for free.

On balance I think the second approach is going to be easier, just because we don't have to worry about the fact that we have a frame for the real DOM element and one for the gencon element.

Comment 51

[Emilio Cobos Álvarez (:emilio)]

The chromium implementation is:

  https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/layout/layout_object.cc?l=216&rcl=ae6a05c5388e96c3e430d385c38e53e8bd61760d

Note that they still respect display: none or contents because of Element::ShouldCreateLayoutObject.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #48)
> It seems to me that Blink and WebKit currently only implement
> <content-replacement> for element (that a single. That doesn't feel terribly
> hard to implement in terms of frame constructing. We probably just need a
> new frame class, or maybe just reuse the frame of <img>, and yield the frame
> when we meet content property with <content-replacement> value on an element.

That is exactly true.

> It may, though, need some nontrivial work on the style system, due to that
> we may not be generating frames for elements which are not in a display:none
> subtree. Maybe it's not that hard, though. We can probably just take
> <iframe>, <video>, and maybe other replaced element as a reference for how
> to suppress children.

That's not right. We already do that for other replaced elements, it should just work, no need to do anything special there. We just will consider the frame a leaf, and won't find an insertion point if the element is replaced. There's no extra style system hackery required I think.

(In reply to Cameron McCormack (:heycam) from comment #50)
> Once we've determined what features from the spec we need, we should do some
> thorough testing to see how Chrome (and other browsers, if any now do
> support this) behave.  Then, compare this with what the spec requires, make
> some judgements as to what actually makes sense, and then file issues on the
> spec accordingly.

As Xidorn said, WebKit / Blink's impl is pretty minimal.

> Some brief testing in Chrome shows that not all elements can have their
> contents replaced, for example it works on an <img>, but not on a <button>
> or an <iframe>:
> 
>   http://mcc.id.au/temp/content.html

Right, they do it in the equivalent of our FindDisplayData in the frame constructor, that is, once we know an element isn't any special kind of element.

> One approach would be to model this like we do for ::before / ::after
> generated content, where we create native anonymous content to represent the
> image.  This would be created via CreateGenConImageContent, which is what we
> use for `::before { content: url() }`, and which is already set up to create
> an nsImageFrame and handle image loading due to nsGenConImageContent (the
> concrete DOM node class we use) implementing nsImageLoadingContent.
> 
> We would need to tweak how the sizing works somehow to look at the parent
> real element's sizing proprerties somehow, which might be tricky.  How do we
> make `span { content: url(...); width: 200px; }` influence the size of the
> child gencon element?
> 
> We would need to suppress frames for the actual content of the element.

Right, this sounds a no-go, we'd need to treat the content as display: contents, make the generated content all: inherit... Sounds complicated.

> The second approach would be to make the element itself get an nsImageFrame,
> by adjusting nsImageFrame::ShouldCreateImageFrameFor to return true for
> elements that have a content:url() value.  An nsImageFrame for the element
> is what we do for <img>.  We wouldn't have any issues with mismatches
> between the sizes of the real element's frame and the gencon element's
> frame, since we've just got the one element.

Well, we'd need to do it in the equivalent place I pointed up above, but yes, this is basically true.

> We would need an instance of nsImageLoadingContent somewhere, and I don't
> think we want to make all HTML elements inherit from this.  Maybe we can
> create one and stuff it in the DOM element's property table.

Reusing nsImageLoadingContent sounds problematic, given it handles all the DOM events, etc... We should probably just split the relevant bits out of nsImageLoadingContent and use a different kind of abstraction in nsImageFrame. Then nsImageLoadingContent can use that.

> <img> elements already suppress the frames for any children they might have,
> but I'm not sure how that happens.  We might still need to do something to
> make that work here too, or it might work for free.

It works for free. See nsIFrame::IsLeaf.

> On balance I think the second approach is going to be easier, just because
> we don't have to worry about the fact that we have a frame for the real DOM
> element and one for the gencon element.

Agreed. :)

Comment 52

[Maire Reavy [:mreavy]]

Thanks for taking this, Emilio!

Comment 53

[Daniel Holbert [:dholbert]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #48)
> (An alternative approach for the webcompat user story of this bug is to ask
> Google not to use this experimental feature in their tier-1 UI...)

We tried that for Google Docs in bug 1239922, and they had a possible way forward that they were willing to do, but it wasn't a high-priority thing for them at the time. But they've made a decent amount of progress in that case -- specifically:
- Per bug 1239922 comment 9, the best alternate strategy (to avoid depending on this experimental feature & to be sure the images actually showed up when a11y features were enabled) was to convert these icons to SVG.
- And by inspection, it seems that they've largely done that! They now have a SVG sprite-sheet referenced in the CSS alongside their PNG one, and they use the SVG sprite-sheet for the google-docs main toolbar's icons, as noted in bug 1239922 comment 17.
- And there are a few places where they're still using the PNG sprite sheet, but that might be accidental -- I can add a single CSS class to *make* them use the SVG sprite sheet in those places, and that makes this Just Work as far as I can tell, per bug 1239922 comment 17.

It sounds like maybe google docs wasn't the main site in question here, though? (There are references to "Google Search experience for Gecko on Android" here  -- do we have any bugs/notes covering what's broken in that context due to our lack-of-support for this experimental feature?)

Comment 54

[Xidorn Quan [:xidorn] UTC+10]

Yeah, it doesn't seem there is any webcompat issue connected to this bug.

It's probably a question for Mike. (I see he has ni?ed himself in this bug already.)

Comment 55

[Mike Taylor [:miketaylr]]

Yeah, sorry about the delay here. The original reason this got lumped in with Google Tier 1 Search bugs is the following issue (hidden in one of the dupes, bug 1372086):

https://github.com/webcompat/web-bugs/issues/7321

(Note, we only see this bug when spoofing as Chrome, which we did during QA to get the Tier 1 search experience).

However, I'm not sure I'm convinced this is a P1 quality/experience breaker. It seems kind of minor, but I have such low expectations on the mobile web these days my judgement might be skewed. ^__^ But I also don't expect Google Search to fix this in the next year or longer.

Emilio, if we do implement this, I think following the Blink/WebKit hack is best. IIRC, we had compat problems around supporting content in Presto, but I don't remember all the details -- presumably following Blink is safe here.

Comment 56

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 215083: Implement content: url(..) for elements.

Take the review request as a feedback request for now.

Pretty sure there's a bunch of image tracking stuff that could (should?) be done
which is not done. Also need to figure out the right thing to do for animated
images and that.

That being said, this works. Dynamic changes are handled correctly because
content property changes already cause a reframe.

The mImage change for intrinsic sizing also fixes bug 1149357, since
HTMLImageElement mungles the image natural width otherwise. I can (and will)
submit that fix separately.

This implements the same bits as Blink / WebKit do (single content item which is
an image, otherwise gets ignored).

Review commit: https://reviewboard.mozilla.org/r/244430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244430/

Comment 57

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review250448

::: commit-message-12b22:12
(Diff revision 1)
> +images and that.
> +
> +That being said, this works. Dynamic changes are handled correctly because
> +content property changes already cause a reframe.
> +
> +The mImage change for intrinsic sizing also fixes bug 1149357, since

Actually this is not right, this change isn't the correct one for intrinsic sizing. But I'll submit that separately so please ignore that bit altogether.

Comment 58

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244430/diff/1-2/

Comment 59

[Emilio Cobos Álvarez (:emilio)]

I uploaded the resulting patch after both dependent bugs. Here's a green try run as well.

There was a very basic test for this in WPT which is now passing, but I should probably spend some more time testing the edge cases.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d676541f38e7f84db8b9f45fb24df088b60bb4e6

Comment 60

[Emilio Cobos Álvarez (:emilio)]

There's also the interesting case of a broken image which would return false from nsImageFrame::ShouldCreateImageFrameFor (and thus display the alt text), but it has content: url(..).

I think in that case Blink / WebKit would still show the content: url URI. But we wouldn't (we'd just treat it as a broken image) since the logic in nsImageFrame is something like: If this is an nsIImageLoadingContent, then fine, otherwise use the content: url() request. So perhaps we should be more explicit about what does nsImageFrame honor and what doesn't, maybe passing a parameter down or what not.

It probably doesn't matter much but worth a test and a spec issue I guess...

Comment 61

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case for broken images

Here's a test-case for that.

Comment 62

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is it worth thinking about the fallback mechanism that was in the spec until recently (i.e., still in the most recent Working Draft on TR... which means I'm not sure if the working group has accepted the removal or not)?

Comment 63

[Emilio Cobos Álvarez (:emilio)]

(In reply to David Baron :dbaron: ⌚UTC-7 from comment #62)
> Is it worth thinking about the fallback mechanism that was in the spec until
> recently (i.e., still in the most recent Working Draft on TR... which means
> I'm not sure if the working group has accepted the removal or not)?

Perhaps, though it's not clear to me how to go about implementing that right now, it probably would add significant complexity to the patch. We have no way to tell whether the image is known broken or not at that point, and we'd have to persist that state somewhere, which sounds annoying.

Comment 64

[Daniel Holbert [:dholbert]]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review251158

My feedback from a quick skim: this seems generally sane (though I don't know the intricacies of the various nsImageFrame special cases & fallback paths being tweaked here)

::: commit-message-12b22:1
(Diff revision 2)
> +Bug 215083: Implement content: url(..) for elements. r?tnikkel,dholbert

Extreme nit: I noticed you use "url(..)" with 2 dots as a placeholder throughout this patch.  I'm not used to that "two-dot" ellipsis -- is that a standard spelling?  If not, you probably want "url(...)" to be more canonical, I imagine.

Comment 65

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review251158

> Extreme nit: I noticed you use "url(..)" with 2 dots as a placeholder throughout this patch.  I'm not used to that "two-dot" ellipsis -- is that a standard spelling?  If not, you probably want "url(...)" to be more canonical, I imagine.

It's the standard "anything" value in Rust, so I guess I'm more used to that... Can tweak I guess :)

Comment 66

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review255398

::: layout/generic/nsImageFrame.cpp:202
(Diff revision 2)
>      PresShell()->CancelReflowCallback(this);
>      mReflowCallbackPosted = false;
>    }
>  
> +  if (mContentURLRequest) {
> +    mContentURLRequest->CancelAndForgetObserver(NS_BINDING_ABORTED);

Can we just do Cancel here? The observer can stick around after the frame goes away.

::: layout/generic/nsImageFrame.cpp:277
(Diff revision 2)
> -
> -  // We have a PresContext now, so we need to notify the image content node that
> +    // We have a PresContext now, so we need to notify the image content node
> +    // that it can register images.
> -  // it can register images.
> -  imageLoader->FrameCreated(this);
> +    imageLoader->FrameCreated(this);
> +  } else if (auto* proxy = StyleContent()->ContentAt(0).GetImage()) {
> +    proxy->SyncClone(mListener,

Why SyncClone? This seems like asking for danger. We are trying to get rid of SyncClone.

Comment 67

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review255398

> Can we just do Cancel here? The observer can stick around after the frame goes away.

Presumably, yes. Just curious, why would that be preferrable? The observer would be quite useless without anything to notify on.

> Why SyncClone? This seems like asking for danger. We are trying to get rid of SyncClone.

I grabbed this from the imageboxframe / bulletframe / ImageLoader / etc. code:

  https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/layout/xul/nsImageBoxFrame.cpp#277
  https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/layout/style/ImageLoader.cpp#368
  
I thought it was the right thing to do, maybe it's not? I can try to change it to an async clone, but my hintch is that we really want to know the intrinsic size and such immediately and such if the image is already loaded...

Comment 68

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review255730

::: commit-message-12b22:5
(Diff revision 2)
> +Bug 215083: Implement content: url(..) for elements. r?tnikkel,dholbert
> +
> +Take the review request as a feedback request for now.
> +
> +Pretty sure there's a bunch of image tracking stuff that could (should?) be done

So for image tracking I believe these images will get added to the document image tracker here

https://dxr.mozilla.org/mozilla-central/rev/752465b44c793318cef36df46ca5ff00c3d8854a/layout/style/nsStyleStruct.cpp#2208

like all other CSS images, is that correct? This means that they will remain "locked" whenever their tab is the active tab. This differs from all other nsImageFrames which can be discarded when they are not close to being scrolled in the visible viewport. Which is acceptable for now.

And ImageLoader should handle getting animated images registered with the refresh driver with its nsLayoutUtils::De/RegisterImageRequest calls.

Comment 69

[Emilio Cobos Álvarez (:emilio)]

(In reply to Timothy Nikkel (:tnikkel) from comment #68)
> Comment on attachment 8976241 [details]
> Bug 215083: Implement content: url(..) for elements.
> 
> https://reviewboard.mozilla.org/r/244430/#review255730
> 
> ::: commit-message-12b22:5
> (Diff revision 2)
> > +Bug 215083: Implement content: url(..) for elements. r?tnikkel,dholbert
> > +
> > +Take the review request as a feedback request for now.
> > +
> > +Pretty sure there's a bunch of image tracking stuff that could (should?) be done
> 
> So for image tracking I believe these images will get added to the document
> image tracker here
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 752465b44c793318cef36df46ca5ff00c3d8854a/layout/style/nsStyleStruct.cpp#2208
> 
> like all other CSS images, is that correct? This means that they will remain
> "locked" whenever their tab is the active tab. This differs from all other
> nsImageFrames which can be discarded when they are not close to being
> scrolled in the visible viewport. Which is acceptable for now.
> 
> And ImageLoader should handle getting animated images registered with the
> refresh driver with its nsLayoutUtils::De/RegisterImageRequest calls.

Yeah, that makes sense. I was looking at all the image box frame and such which uses list-style-image, which track stuff manually.

But I forgot that that is pretty much the special-case, and we pass Mode::Track here:

  https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/layout/style/ServoBindings.cpp#1615

So that should be fine indeed.

Comment 70

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review259970

Comment 71

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review255398

> Presumably, yes. Just curious, why would that be preferrable? The observer would be quite useless without anything to notify on.

What I meant about the observer is that it still lives so it's okay if (useless) notifications get sent to it. CancelAndForgetObserver removes the request from the load group, which can fire the load event, which can do anything, which we would like to not happen. So for example if we were in the destructor of an observer we would have no other choice and would have to call CancelAndForgetObserver (but we'd like to avoid this case and call).

> I grabbed this from the imageboxframe / bulletframe / ImageLoader / etc. code:
> 
>   https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/layout/xul/nsImageBoxFrame.cpp#277
>   https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/layout/style/ImageLoader.cpp#368
>   
> I thought it was the right thing to do, maybe it's not? I can try to change it to an async clone, but my hintch is that we really want to know the intrinsic size and such immediately and such if the image is already loaded...

SyncClone can send notifications synchronously, which means the observers run, and they can do anything, including getting us into bad situations historically (recursive notifcations for one). So we are trying to get rid of SyncClones. Further, getting the notifications doesn't actually get us anything as we can ask the image for its status at any time we want, and that is what Reflow does. So I don't think it actually helps us show the image any faster.

Comment 72

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review255398

> SyncClone can send notifications synchronously, which means the observers run, and they can do anything, including getting us into bad situations historically (recursive notifcations for one). So we are trying to get rid of SyncClones. Further, getting the notifications doesn't actually get us anything as we can ask the image for its status at any time we want, and that is what Reflow does. So I don't think it actually helps us show the image any faster.

Sure... In this case the observer is under control though, but I agree.

> Further, getting the notifications doesn't actually get us anything as we can ask the image for its status at any time we want, and that is what Reflow does. So I don't think it actually helps us show the image any faster.

Does it, though? As far as I can tell, we don't get an `mImage` until the `OnSizeAvailable` notification runs. Doesn't that mean that our intrinsic size and ratio would be the default until then?

Comment 73

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244430/diff/2-3/

Comment 74

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244430/diff/3-4/

Comment 75

[Emilio Cobos Álvarez (:emilio)]

Moved to Clone / Cancel instead of SyncClone. Though I think SyncClone is closer to what happens for normal images: AddNativeObserver does call all the notifications via ReplayImageStatus. I think I'd prefer to keep SyncClone, but Timothy has the last word there I guess.

I added code to handle the edge cases, and filed a couple spec issues for them (WebKit / Blink handle them erratically): https://github.com/w3c/csswg-drafts/issues/2832 / https://github.com/w3c/csswg-drafts/issues/2831.

I think this is ready for review.

Comment 76

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review255398

> Sure... In this case the observer is under control though, but I agree.
> 
> > Further, getting the notifications doesn't actually get us anything as we can ask the image for its status at any time we want, and that is what Reflow does. So I don't think it actually helps us show the image any faster.
> 
> Does it, though? As far as I can tell, we don't get an `mImage` until the `OnSizeAvailable` notification runs. Doesn't that mean that our intrinsic size and ratio would be the default until then?

Good point! We could just call OnSizeAvailable or NotifyNewCurrentRequest after cloning if the image request has a size.

Comment 77

[Timothy Nikkel (:tnikkel)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #75)
> Moved to Clone / Cancel instead of SyncClone. Though I think SyncClone is
> closer to what happens for normal images: AddNativeObserver does call all
> the notifications via ReplayImageStatus. I think I'd prefer to keep
> SyncClone, but Timothy has the last word there I guess.

Oh yeah, I guess you are right. That was added relatively recently in bug 1103157. So you can view the problem two ways: either you replay all the status whenever a new observer is added or image observers are responsible for checking the status of the image when they add themselves as an observer and acting appropriately.

I prefer the second because it lets us remove SyncClone at some point in the future when we've refactored the code. The issue about the first reflow having the correct intrinsic size and ratio should be fixed by just calling one of the functions that updates that in Init.

Comment 78

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review260182

Comment 79

[Emilio Cobos Álvarez (:emilio)]

(In reply to Timothy Nikkel (:tnikkel) from comment #77)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #75)
> > Moved to Clone / Cancel instead of SyncClone. Though I think SyncClone is
> > closer to what happens for normal images: AddNativeObserver does call all
> > the notifications via ReplayImageStatus. I think I'd prefer to keep
> > SyncClone, but Timothy has the last word there I guess.
> 
> Oh yeah, I guess you are right. That was added relatively recently in bug
> 1103157. So you can view the problem two ways: either you replay all the
> status whenever a new observer is added or image observers are responsible
> for checking the status of the image when they add themselves as an observer
> and acting appropriately.
> 
> I prefer the second because it lets us remove SyncClone at some point in the
> future when we've refactored the code. The issue about the first reflow
> having the correct intrinsic size and ratio should be fixed by just calling
> one of the functions that updates that in Init.

Timothy, mind if I land this patch with SyncClone, then get rid of that and ReplayImageStatus in a different bug? Without SyncClone it causes some test to fail in a very funny way.

No SyncClone: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fe5eaab46672c4cb322e213a5378f48a18e411f
SyncClone: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad332f09de54c10ab21edb30ffb7b84172c8d28e

The reason for at least the style system one is funny: We have "content: url(..)" tests in a couple tests on the property database (https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/layout/style/test/property_database.js#3349).

Without SyncClone, we don't get the right size, so the first read for `line-height: -moz-block-height` gives the block height at the time which doesn't include the image size. The second read happens after the image has already loaded, so we get a different height.

I'm not opposed to do those changes in this bug, but I think it's worth doing it in a separate one. In any case I promise I won't forget about it, I just want to figure out the best way to share code between the current SyncClone callers.

Comment 80

[Daniel Holbert [:dholbert]]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review260706

::: layout/base/nsCSSFrameConstructor.cpp:5801
(Diff revision 4)
>      // Now check for XUL display types
>      if (!data) {
>        data = FindXULDisplayData(display, element, computedStyle);
>      }
>  
> +    if (!data && ShouldCreateImageFrameForContent(*computedStyle)) {
> +      static const FrameConstructionData sImgData =
> +        SIMPLE_FCDATA(NS_NewImageFrameForContentProperty);
> +      data = &sImgData;
> +    }
> +
>      // And general display types

Two things:

(1) Please add a comment above this chunk. (Right now it's implicitly under the "Now check for XUL display types" heading, which is awkward/incorrect.)

Maybe:
 // Now check for 'content: <image-url>' (which makes us
 // ignore the 'display' property).


(2) My suggested ^^ comment is not *quite* true right now, because you've got us honoring XUL 'display' values over this feature, vs. ignoring all other display values.  Is that a meaningful distinction?  Maybe we should just insert this a few lines up, so that this has a more coherent/understandable precedence? (i.e. if you insert it a few lines up, then "content" would beat *all* 'display' values, rather than only beating non-XUL display values.)  I don't imagine we intend to use this feature in XUL anyway, so it probably doesn't really matter, other than maintainability/understandability/simplicity.   So, seems better to have this fully override 'display' -- or, if there's a reason it needs to be where you've got it, maybe call that reason out in the comment?

::: layout/generic/nsImageFrame.h:183
(Diff revision 4)
> +  // The kind of frame we are.
> +  enum class Kind : uint8_t
> +  {
> +    // For an nsImageLoadingContent.
> +    ImageElement,
> +    // For css content: url(..).
> +    ContentProperty,
> +  };

Two things:
 (1) s/frame/image frame/ in the documentation.

 (2) Also: right now, the comments & typenames here implies that this is for all "content"-generated image frames.  But I don't think that's actually true -- we take a different codepath for ::before/::after { content: ... } (which we already support).  I don't think those trigger the codepath that produce this "Kind" -- does it?  (If it does, great! I'm just not sure, and I'm guessing it doesn't based on your extra check for NS_FRAME_GENERATED_CONTENT elsewhere in this patch).

Anyway -- please clarify that, and possibly even rename, if this does not cover "content" for pseudo-elements.


(Trivial demo of pseudo-element 'content' usage that Gecko already supports & which I'm wondering about RE these Kinds: https://jsfiddle.net/4vho7y2d/  )

::: layout/generic/nsImageFrame.h:196
(Diff revision 4)
> +  explicit nsImageFrame(ComputedStyle* aStyle, Kind aKind)
> +    : nsImageFrame(aStyle, kClassID, aKind)

You can drop the "explicit" keyword, since this is now a 2-arg constructor, which doesn't need "explicit".

::: layout/generic/nsImageFrame.h:202
(Diff revision 4)
>  protected:
> -  nsImageFrame(ComputedStyle* aStyle, ClassID aID);
> +  nsImageFrame(ComputedStyle* aStyle, ClassID aId)
> +    : nsImageFrame(aStyle, aId, Kind::ImageElement)
> +  { }

nit: this patch is renaming "aID" to "Id" *only* here in this one constructor in the .h file -- but it's still called aID (capital D) in the other constructor that's defined in the c++ file, and in the constructors within the nsIFrame.h.

So let's revert this renaming here, to avoid introducing inconsistency.

If you happen to feel strongly about changing the "d" to lowercase, let's take that to a separate more-thorough bug. :)

::: layout/generic/nsImageFrame.cpp:434
(Diff revision 4)
> -  nsCOMPtr<nsIImageLoadingContent> imageLoader(do_QueryInterface(mContent));
> -  NS_ASSERTION(imageLoader, "No image loading content?");
> +  if (mKind == Kind::ContentProperty) {
> +    MOZ_ASSERT(aRequest == mContentURLRequest);
> +    return false;
> +  }
>  
> +  nsCOMPtr<nsIImageLoadingContent> imageLoader(do_QueryInterface(mContent));
>    int32_t requestType = nsIImageLoadingContent::UNKNOWN_REQUEST;

Looks like you're removing an assertion (enforcing/documenting that imageLoader must be non-null).

Seems like that assertion would be useful to keep around.  We've got this assertion sprinkled throughout this file and it's a nice assumption to have documented/checked before we dereference the pointer.

(I notice you've strengthened some "if (imageLoader)" null-checks to become fatal assertions elsewhere; that seems good too.)

::: layout/generic/nsImageFrame.cpp:864
(Diff revision 4)
> +
> +  // NOTE(emilio, https://github.com/w3c/csswg-drafts/issues/2832): WebKit
> +  // and Blink behave differently here for content: url(..), for now adapt to
> +  // Blink's behavior.
> +  const bool mayDisplayBrokenIcon =
> +    mKind == Kind::ImageElement &&
> +    !(GetStateBits() & NS_FRAME_GENERATED_CONTENT);
> +
> +  if (!mayDisplayBrokenIcon) {
> +    return;
> +  }
> +

I didn't initially understand what this boolean condition was getting at.

After staring at it for a bit, I think this approximately represents "is this image from a real image element, i.e. and not generated/replaced using the content property".  Is that right?

Might be nice to document that (or whatever meaning it's trying to capture), to help people grok what we're really checking here, beyond just "this is what Blink checks". :)  (This is what got me thinking about Kind::ImageElement vs. Kind::ContentProperty and which bucket ::before/::after generated content falls into, BTW.)

::: layout/generic/nsImageFrame.cpp:887
(Diff revision 4)
> -        if (nsCOMPtr<imgIRequest> currentRequest = GetCurrentRequest()) {
> +  if (nsCOMPtr<imgIRequest> currentRequest = GetCurrentRequest()) {
> -          uint32_t imageStatus;
> +    uint32_t imageStatus;
> -          imageInvalid =
> +    imageInvalid =
> -            NS_SUCCEEDED(currentRequest->GetImageStatus(&imageStatus)) &&
> +      NS_SUCCEEDED(currentRequest->GetImageStatus(&imageStatus)) &&
> -            (imageStatus & imgIRequest::STATUS_ERROR);
> +      (imageStatus & imgIRequest::STATUS_ERROR);
> -        } else if (nsCOMPtr<nsIImageLoadingContent> loader = do_QueryInterface(mContent)) {
> +  } else if (mKind == Kind::ImageElement) {

Note: this change (and this check) seems unnecessary.  mKind *has to be* ImageElement here, I think.

If mKind were anything other than ImageElement, then we'd have set mayDisplayBrokenIcon = false up above, and we'd have already returned up there as a result.

::: layout/generic/nsImageFrame.cpp
(Diff revision 4)
> -  MOZ_ASSERT(imageLoader);
>  

As above, I'm not sure it makes sense to remove MOZ_ASSERT(imageLoader) assertions like this one; they're useful to document expectations/invariants that aren't otherwise obvious.

e.g. here, the do_QueryInterface() is technically allowed to return null -- and we're implicitly assuming that it does not do so, when we dereference it.  And the MOZ_ASSERT is documenting/enforcing that implicit assumption, & making it more explicit. (In an ideal world, it'd also have a 1-liner explanation of *why* we can make that assumption, but it's still useful even without that IMO.)

So: maybe revert this assertion-removal?

Comment 81

[Daniel Holbert [:dholbert]]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review260728

(I guess this is r- from me in its present state, but likely r+ with feedback addressed.)

Comment 82

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review260706

> Two things:
> 
> (1) Please add a comment above this chunk. (Right now it's implicitly under the "Now check for XUL display types" heading, which is awkward/incorrect.)
> 
> Maybe:
>  // Now check for 'content: <image-url>' (which makes us
>  // ignore the 'display' property).
> 
> 
> (2) My suggested ^^ comment is not *quite* true right now, because you've got us honoring XUL 'display' values over this feature, vs. ignoring all other display values.  Is that a meaningful distinction?  Maybe we should just insert this a few lines up, so that this has a more coherent/understandable precedence? (i.e. if you insert it a few lines up, then "content" would beat *all* 'display' values, rather than only beating non-XUL display values.)  I don't imagine we intend to use this feature in XUL anyway, so it probably doesn't really matter, other than maintainability/understandability/simplicity.   So, seems better to have this fully override 'display' -- or, if there's a reason it needs to be where you've got it, maybe call that reason out in the comment?

Guess it's not a big deal overriding XUL display types, but generally I wanted to deal as little as possible with XUL :-).

Will do. Also will mention that we do honor display: none and display: contents, just like Blink or WebKit.

> Two things:
>  (1) s/frame/image frame/ in the documentation.
> 
>  (2) Also: right now, the comments & typenames here implies that this is for all "content"-generated image frames.  But I don't think that's actually true -- we take a different codepath for ::before/::after { content: ... } (which we already support).  I don't think those trigger the codepath that produce this "Kind" -- does it?  (If it does, great! I'm just not sure, and I'm guessing it doesn't based on your extra check for NS_FRAME_GENERATED_CONTENT elsewhere in this patch).
> 
> Anyway -- please clarify that, and possibly even rename, if this does not cover "content" for pseudo-elements.
> 
> 
> (Trivial demo of pseudo-element 'content' usage that Gecko already supports & which I'm wondering about RE these Kinds: https://jsfiddle.net/4vho7y2d/  )

Good point. Any suggestions for a new name? I've renamed to NonGeneratedContentProperty, but not in love with it.

> Looks like you're removing an assertion (enforcing/documenting that imageLoader must be non-null).
> 
> Seems like that assertion would be useful to keep around.  We've got this assertion sprinkled throughout this file and it's a nice assumption to have documented/checked before we dereference the pointer.
> 
> (I notice you've strengthened some "if (imageLoader)" null-checks to become fatal assertions elsewhere; that seems good too.)

Yeah, to be honest I'm not sure how useful these are given we assert on init, I'm ambivalent.

I'll keep them for now, but we may want to do something like adding an asserting function instead, for example.

> Note: this change (and this check) seems unnecessary.  mKind *has to be* ImageElement here, I think.
> 
> If mKind were anything other than ImageElement, then we'd have set mayDisplayBrokenIcon = false up above, and we'd have already returned up there as a result.

Yeah good point, I changed my mind about whose behavior adapt to, thus this code remained :-)

I'll remove.

> As above, I'm not sure it makes sense to remove MOZ_ASSERT(imageLoader) assertions like this one; they're useful to document expectations/invariants that aren't otherwise obvious.
> 
> e.g. here, the do_QueryInterface() is technically allowed to return null -- and we're implicitly assuming that it does not do so, when we dereference it.  And the MOZ_ASSERT is documenting/enforcing that implicit assumption, & making it more explicit. (In an ideal world, it'd also have a 1-liner explanation of *why* we can make that assumption, but it's still useful even without that IMO.)
> 
> So: maybe revert this assertion-removal?

Yeah, fair enough.

Comment 83

[Daniel Holbert [:dholbert]]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review260706

> Good point. Any suggestions for a new name? I've renamed to NonGeneratedContentProperty, but not in love with it.

I don't have any better suggestions; "NonGeneratedContentProperty" seems not-too-terrible, as long as it's got clear documentation for what it means.

Comment 84

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244430/diff/4-5/

Comment 85

[Daniel Holbert [:dholbert]]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review260760

::: layout/generic/nsImageFrame.h:367
(Diff revision 5)
> +  // An image request created for content: url(..).
> +  RefPtr<imgRequestProxy> mContentURLRequest;

(Possibly followup material):  I wonder if this should be stored in the frame's property table, rather than a normal member-var, to avoid bloating "normal" nsImageFrame instances?

(In probably >99% of nsImageFrame instances, this pointer will be unused...  And on pages with lots of images, and extra unused pointer per nsImageFrame is kinda worth worrying about.  So this seems like a good candidate for putting in the property table & having marginally slower access time as a tradeoff for all other images being more compact.)

Comment 86

[Daniel Holbert [:dholbert]]

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review260760

> (Possibly followup material):  I wonder if this should be stored in the frame's property table, rather than a normal member-var, to avoid bloating "normal" nsImageFrame instances?
> 
> (In probably >99% of nsImageFrame instances, this pointer will be unused...  And on pages with lots of images, and extra unused pointer per nsImageFrame is kinda worth worrying about.  So this seems like a good candidate for putting in the property table & having marginally slower access time as a tradeoff for all other images being more compact.)

(If you end up addressing this here, it'd probably be worth doing one more round of review on the changes, to sanity-check the refcounting & property-table usage.)

Comment 87

[Timothy Nikkel (:tnikkel)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #79)
> (In reply to Timothy Nikkel (:tnikkel) from comment #77)
> > (In reply to Emilio Cobos Álvarez [:emilio] from comment #75)
> > > Moved to Clone / Cancel instead of SyncClone. Though I think SyncClone is
> > > closer to what happens for normal images: AddNativeObserver does call all
> > > the notifications via ReplayImageStatus. I think I'd prefer to keep
> > > SyncClone, but Timothy has the last word there I guess.
> > 
> > Oh yeah, I guess you are right. That was added relatively recently in bug
> > 1103157. So you can view the problem two ways: either you replay all the
> > status whenever a new observer is added or image observers are responsible
> > for checking the status of the image when they add themselves as an observer
> > and acting appropriately.
> > 
> > I prefer the second because it lets us remove SyncClone at some point in the
> > future when we've refactored the code. The issue about the first reflow
> > having the correct intrinsic size and ratio should be fixed by just calling
> > one of the functions that updates that in Init.
> 
> Timothy, mind if I land this patch with SyncClone, then get rid of that and
> ReplayImageStatus in a different bug? Without SyncClone it causes some test
> to fail in a very funny way.

I certainly did not (and do not) intend to make removing ReplayImageStatus a blocker for this bug. Can you just call NotifyNewCurrentRequest or OnSizeAvailable directly after calling Clone? That should fix the problem no?

Comment 88

[Emilio Cobos Álvarez (:emilio)]

Attachment: Remove sync clone.

This seems to work locally, how does it look?

Comment 89

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8988922 [details] [diff] [review]
Remove sync clone.

Looks good. Thanks.

Comment 90

[Emilio Cobos Álvarez (:emilio)]

(In reply to Daniel Holbert [:dholbert] from comment #85)
> (Possibly followup material):  I wonder if this should be stored in the
> frame's property table, rather than a normal member-var, to avoid bloating
> "normal" nsImageFrame instances?
> 
> (In probably >99% of nsImageFrame instances, this pointer will be unused... 
> And on pages with lots of images, and extra unused pointer per nsImageFrame
> is kinda worth worrying about.  So this seems like a good candidate for
> putting in the property table & having marginally slower access time as a
> tradeoff for all other images being more compact.)

I agree.

What do you think of doing something like:

struct RareData {
  RefPtr<nsImageMap> mImageMap;
  RefPtr<imgRequestProxy> mContentURLRequest;
};

Or something of the sort, and having a:

  UniquePtr<RareData> mRareData;

member on the image frame that uses to be null?

I can also add two frame properties (one for the image map and one from the content: url thing). I can also pack bits better (i.e., mKind can be a bit, and it can be packed with the other four bool members of nsImageFrame).

WDYT? Worth filing?
Though I generally try to prevent frame properties that only apply to a specific kind of frame.

Also, we cou

Comment 91

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #90)
> Though I generally try to prevent frame properties that only apply to a
> specific kind of frame.
> 
> Also, we cou

Err, I should really stop leaving content under my caret in bugzilla :P. Anyway, ignore this broken sentence.

Comment 92

[Daniel Holbert [:dholbert]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #90)
> What do you think of doing something like:
> 
> struct RareData {
>   RefPtr<nsImageMap> mImageMap;
>   RefPtr<imgRequestProxy> mContentURLRequest;
> };
> 
> Or something of the sort, and having a:
> 
>   UniquePtr<RareData> mRareData;
> 
> member on the image frame that uses to be null?

> I can also add two frame properties (one for the image map and one from the
> content: url thing).

I think frame properties are marginally better, since frame properties are already supposed to effectively be a generic "raredata" pointer, and it's better not do create a different technique to do the same thing unless we really need to. 

> I can also pack bits better (i.e., mKind can be a bit,
> and it can be packed with the other four bool members of nsImageFrame).

This might be worth it (not sure) - I would hope that the uint8_t would already pack nicely with the bools, but I don't know for sure. Anyway, I'm less worried about the incremental uint8_t than I am about the 64-bit pointer (though it's possible they are equally bad if the packing is bad).

> WDYT? Worth filing?

Likely, yeah. Thanks!

Comment 93

[Emilio Cobos Álvarez (:emilio)]

Attachment: CreateContinuingFrame fix.

Just realized that I missed this nsImageFrame construction path and that needs fixing.

Comment 94

[Cameron McCormack (:heycam)]

Comment on attachment 8988953 [details] [diff] [review]
CreateContinuingFrame fix.

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

Seems fine.

Comment 95

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ca43c308ce1
Implement content: url(..) for elements. r=tnikkel,dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/72416dd3422a
Remove sync image request clone. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4d140d2826
Fix CreateContinuingFrame to account for non-nsImageLoadingContent image frames. r=heycam

Comment 96

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3983769db6d
Account for subclasses in the next-in-flow assertion. r=me

Comment 97

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11744 for changes under testing/web-platform/tests

Comment 98

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 99

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2ca43c308ce1
https://hg.mozilla.org/mozilla-central/rev/72416dd3422a
https://hg.mozilla.org/mozilla-central/rev/cd4d140d2826

Comment 100

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 101

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/aea3f3457f15
Account for subclasses in the next-in-flow assertion. r=me a=fix-uplift

Comment 102

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/aea3f3457f15

Comment 103

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c3983769db6d

Comment 104

[Chris Peterson [:cpeterson]]

(In reply to Maire Reavy [:mreavy] Plz needinfo? from comment #46)
> Cameron -- As I mentioned in the Layout standup meeting last week, this is a
> P1 webcompat bug that I really want to see solved.

Emilio, is this fix something we would uplift to Beta 62? It's probably not a high priority uplift because the tier1 Google search page for Android is still blocked by other unfixed compat issues in Beta 62.

Comment 105

[Emilio Cobos Álvarez (:emilio)]

Yeah, probably not worth uplifting. Unless Maire thinks otherwise?

Comment 106

[j.j.]

Please set "Target" field to avoid confusion

Comment 107

[Peter Simonyi]

This might have caused a regression; see bug 1473651.

Comment 108

[Chris Peterson [:cpeterson]]

I am assuming for now that we won't uplift this fix to Beta 62 because it introduced possible regressions (like bug 1473651) and this fix alone is not enough to satisfy Google's requirements for tier 1 search experience on Fennec.

Comment 109

[Boris Zbarsky [:bzbarsky]]

Is there a new bug tracking implementing "content: attr(foo)" for elements?

Comment 110

[Emilio Cobos Álvarez (:emilio)]

Filed bug 1481150, though as I said there I'm not sure what that's supposed to compute to...