1037643 - <img srcset> and <picture> load ordering not in line with spec

Status: RESOLVED FIXED

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

Description

[John Schoenick [:johns]]

Currently the responsive modes of <img> take most actions synchronously, whereas some should queue a task.

We also don't track responsive state at all before BindToTree for this reason

Comment 1

[John Schoenick [:johns]]

Attachment: Part 1 - Add ResponsiveImageSelector::SelectImage

Adds a call to explicitly force image image selection to happen, optionally blowing away cached selection

Comment 2

[John Schoenick [:johns]]

Attachment: Part 2 - Line up with new spec for <img srcset> and <picture> mutations

Pegging :bz for this, since he's probably most familiar with <img> spec things, but let me know if you think someone else should be on the hook.

This attempts to make us match the new "update the image data" algorithm for all "relevant mutations" in the spec, at least as they pertain to responsive mode -- srcset and <picture>. It doesn't give us full last/pending/next load symantics, which I'm going to work on in bug 1076583.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=24711
https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data
https://html.spec.whatwg.org/multipage/embedded-content.html#relevant-mutations

This also leaves us doing the older synchronous behavior for basic img.src, only using the new algorithm when we have srcset or <picture> in most cases, since the new "update the image data" steps 3 through 5.3 still replace some of the synchronous behavior. Using both behaviors adds a minor edge case in that setting img.src followed by img.srcset via script on an empty <img> may trigger two loads where the spec would have us only do one, but otherwise mostly works for making picture/srcset behavior sane without significant change on basic img content. I opened Bug 1076583 for the remaining non-picture work.

This also rips out a good deal of logic landed with picture originally, since the old algorithm it was written against only updated the necessary state, whereas the new algorithm essentially re-runs all of the selection steps whenever any mutation event occurs. This is much simpler, and also much saner from the web author's point of view, in that mutating a <picture> will cause it to re-select its ideal image source, full stop.

We also can now move on to the next <source> if the current one doesn't parse correctly, so ideally we would keep ResponsiveImageSelectors around for all sources we're aware of to avoid re-parsing previous ones to see if they are now valid when mutations occur. I'm going to open a follow-up to do that, but let me know if you think it should land with this.

(These patches are rebased on the ones in bug 1076587, which are probably needed to apply cleanly)

Comment 3

[John Schoenick [:johns]]

Attachment: img src/srcset mutation tests

Comment 4

[John Schoenick [:johns]]

Attachment: <picture> mutation tests

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 8498610 [details] [diff] [review]
Part 1 - Add ResponsiveImageSelector::SelectImage

- In ResponsiveImageSelector.h

+  // Get the url and density for the selected best candidate. That
+  // these implicitly cause an image to be selected if necessary.

Fix the comment, i.e. remove 'That'.

r=jst

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag here; I need to read through the spec bits a bit.  :(

Comment 7

[John Schoenick [:johns]]

Attachment: Part 3 - Be more explicit about when we're falling back to synchronous behavior

Split to part 3 to not complicate reviewing, but this could be folded into part 2 --

Working on mixed content blocking for <picture> i found a few cases where our behavior was confused because we were only checking if (mResponsiveSelector) without considering a image task might be queued. This just makes the interim behavior paths more explicit behind InResponsiveMode()

HaveSrcsetOrInPicture is split out because it is used in the mixed content blocking patches, which explicitly have "if the image has a srcset attribute or a parent element of <picture>" condition (Bug 1055750)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag here.... I'll try to get to this tomorrow; need to sort through the spec bits.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8498617 [details] [diff] [review]
Part 2 - Line up with new spec for <img srcset> and <picture> mutations

>+++ b/content/html/content/src/HTMLImageElement.cpp
> HTMLImageElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,

>+  if (nsContentUtils::IsImageSrcSetDisabled()) {
>+    return NS_OK;

That can't be right.  With this change, if I do:

  var img = new Image();
  img.setAttribute("foo", "bar");
  alert(img.getAttribute("foo"));

I won't get "bar", will I?  Please fix, and add a test that would have caught this.

> HTMLImageElement::UnbindFromTree(bool aDeep, bool aNullParent)

Posting these events when tearing down a document seems a bit annoying.    Could we skip doing the QueueImageLoadTask in that case?  Followup might be ok here as long as it gets uplifted to whatever branches this lands on.

>+HTMLImageElement::QueueImageLoadTask()
>+  if (!LoadingEnabled() || !doc || !doc->IsActive()) {

I thought the documentation on nsIDocument::IsActive is pretty clear:

   this does NOT match the "active document" concept in the WHATWG spec.

Though neither does what it suggests using; the closest thing is nsPIDOMWindow::HasActiveDocument, but with our document instead of mDoc.

We should add a better way of doing this test and use it here.

Also, no need to null-check "doc".  It can't ever be null.

Please document that this is not used in the cases when we just have an src, so the fact that those cases need to pick a selected source synchronously is ok.

> HTMLImageElement::PictureSourceSrcsetChanged(nsIContent *aSourceNode,
>+  MOZ_ASSERT(isSelf || IsPreviousSibling(aSourceNode, AsContent()),

Why AsContent() instead of "this"?

Same for all the other places where AsContent() is used here.

>+  nsIContent *currentSrc = \

Why the backslash?  Shouldn't be needed.  This also happens several times.

>+HTMLImageElement::UpdateResponsiveSource()
>+    candidateSource = thisContent;

That's a confusing way to write:

  candidateSource = this;

>+  MOZ_ASSERT(candidateSource,
>+             "Did not hit ourselves before reaching end of siblings?");

I don't think this assert (an some other asserts here) is necessarily valid when XBL is involved.... (e.g. consider when "this" is an anon subtree root in the binding, so our parent is the bound element).

>+++ b/content/html/content/src/HTMLPictureElement.cpp
>+      if (nextSibling->Tag() == nsGkAtoms::img) {
>+        HTMLImageElement *img = static_cast<HTMLImageElement*>
(nextSibling.get());

how about using  NS_IMPL_FROMCONTENT_HTML_WITH_TAG in the header and then here:

  if (HTMLImageElement* img = HTMLImageElement::FromContent(nextSibling)) {
....

r+, but please do fix the above issues.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8498618 [details] [diff] [review]
img src/srcset mutation tests

r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8498619 [details] [diff] [review]
<picture> mutation tests

A very skimmy r+.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8501357 [details] [diff] [review]
Part 3 - Be more explicit about when we're falling back to synchronous behavior

r=me

Comment 13

[John Schoenick [:johns]]

Attachment: Part 1 - Add ResponsiveImageSelector::SelectImage. r=jst

Addressed review comment, carrying over r=jst

Comment 14

[John Schoenick [:johns]]

Attachment: Part 4 - Cycle collect HTMLImageElement/ResponsiveImageSelector

So it turns out not nulling out mResponsiveSelector in UnbindFromTree creates a cycle, and HTMLImageElement is CC'd (inherited) but not explicitly CC'd

Comment 15

[Andrew McCreight [:mccr8]]

Comment on attachment 8504991 [details] [diff] [review]
Part 4 - Cycle collect HTMLImageElement/ResponsiveImageSelector

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

::: content/base/src/ResponsiveImageSelector.cpp
@@ +15,5 @@
>  #include "nsIMediaList.h"
>  #include "nsRuleNode.h"
>  #include "nsRuleData.h"
>  
> +#include "nsCycleCollectionParticipant.h"

nit: you don't really need this, as you are including it via the header of this file, IMO.

@@ +23,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +// Cycle collection

nit: don't really need this comment

::: content/html/content/src/HTMLImageElement.cpp
@@ +27,5 @@
>  #include "nsIDOMWindow.h"
>  #include "nsFocusManager.h"
>  #include "mozilla/dom/HTMLFormElement.h"
>  #include "nsAttrValueOrString.h"
> +#include "nsCycleCollectionParticipant.h"

nit: you don't really need this, as you are including it via the header of this file, IMO.

@@ +116,5 @@
>  
>  NS_IMPL_ADDREF_INHERITED(HTMLImageElement, Element)
>  NS_IMPL_RELEASE_INHERITED(HTMLImageElement, Element)
>  
> +// Cycle collection

nit: you don't really need the comment here.

::: content/html/content/src/HTMLImageElement.h
@@ +37,5 @@
>            const Optional<uint32_t>& aWidth,
>            const Optional<uint32_t>& aHeight,
>            ErrorResult& aError);
>  
> +  // CC

nit: you don't need this comment

Comment 16

[John Schoenick [:johns]]

Attachment: Part 1.99 - Add nsIDocument::IsCurrentActiveDocument

From addressing the review comments for part 2 -- this bit should probably be re-reviewed by bz. Also, should we rev the IID for this class when adding a non-virtual?

Comment 17

[John Schoenick [:johns]]

Attachment: Part 3 - Be more explicit about when we're falling back to synchronous behavior r=bz

Updated comment in QueueImageLoadTask to document that it is not used yet in the img.src case (from part 2 review comments)

Comment 18

[John Schoenick [:johns]]

Attachment: Part 2 - Line up with new spec for <img srcset> and <picture> mutations. r=bz

Addressed comments as noted below, carrying over r+ but let me know if you'd like further changes

(In reply to Boris Zbarsky [:bz] from comment #9)
> Comment on attachment 8498617 [details] [diff] [review]
> Part 2 - Line up with new spec for <img srcset> and <picture> mutations
>
> >+++ b/content/html/content/src/HTMLImageElement.cpp
> > HTMLImageElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>
> >+  if (nsContentUtils::IsImageSrcSetDisabled()) {
> >+    return NS_OK;
>
> That can't be right.  With this change, if I do:
>
>   var img = new Image();
>   img.setAttribute("foo", "bar");
>   alert(img.getAttribute("foo"));
>
> I won't get "bar", will I?  Please fix, and add a test that would have
> caught this.

.. if you have | dom.disable_image_src_set | set, which is unused (except maybe in seamonkey?) and being removed in Bug 773429!

I moved the bailout to the right spot, but I don't think we should be adding tests for dom.disable_image_src_set if it's waiting for someone to remove it

> > HTMLImageElement::UnbindFromTree(bool aDeep, bool aNullParent)
>
> Posting these events when tearing down a document seems a bit annoying.
> Could we skip doing the QueueImageLoadTask in that case?  Followup might be
> ok here as long as it gets uplifted to whatever branches this lands on.

I filed Bug 1082828 for this, and put it on my list for picture followups

> >+HTMLImageElement::QueueImageLoadTask()
> >+  if (!LoadingEnabled() || !doc || !doc->IsActive()) {
>
> I thought the documentation on nsIDocument::IsActive is pretty clear:
>
>    this does NOT match the "active document" concept in the WHATWG spec.
>
> Though neither does what it suggests using; the closest thing is
> nsPIDOMWindow::HasActiveDocument, but with our document instead of mDoc.
>
> We should add a better way of doing this test and use it here.

I added part 1.99 for this and asked for separate review since its adding new nsIDocument fun. Changed this patch to use IsCurrentActiveDocument from that patch

> Also, no need to null-check "doc".  It can't ever be null.

Dropped

> Please document that this is not used in the cases when we just have an src,
> so the fact that those cases need to pick a selected source synchronously is
> ok.

Part 3 adds more comments about the img.src case, updated it with a better description for QueueImageLoadTask

> > HTMLImageElement::PictureSourceSrcsetChanged(nsIContent *aSourceNode,
> >+  MOZ_ASSERT(isSelf || IsPreviousSibling(aSourceNode, AsContent()),
>
> Why AsContent() instead of "this"?
>
> Same for all the other places where AsContent() is used here.

I had assumed, for some reason, we couldn't simply downcast to nsIContent. I'm not sure why. I removed some of the superfluousness here.

> >+  nsIContent *currentSrc = \
>
> Why the backslash?  Shouldn't be needed.  This also happens several times.

Delimiting uncommon linebreak points, just a style thing for readability (but removed since its not in line with mozilla style either way)

> >+HTMLImageElement::UpdateResponsiveSource()
> >+    candidateSource = thisContent;
>
> That's a confusing way to write:
>
>   candidateSource = this;

Also fixed

> >+  MOZ_ASSERT(candidateSource,
> >+             "Did not hit ourselves before reaching end of siblings?");
>
> I don't think this assert (an some other asserts here) is necessarily valid
> when XBL is involved.... (e.g. consider when "this" is an anon subtree root
> in the binding, so our parent is the bound element).

Removed assert and just null out mResponsiveSelector here (like we would have if we hit <img> with still no candidate). If we run into other asserts with odd XBL usage on <picture>, it would probably be worth the bug to make sure whatever something is doing with XBL is what we actually expect...

> >+++ b/content/html/content/src/HTMLPictureElement.cpp
> >+      if (nextSibling->Tag() == nsGkAtoms::img) {
> >+        HTMLImageElement *img = static_cast<HTMLImageElement*>
> (nextSibling.get());
>
> how about using  NS_IMPL_FROMCONTENT_HTML_WITH_TAG in the header and then
> here:
>
>   if (HTMLImageElement* img = HTMLImageElement::FromContent(nextSibling)) {
> ....

That is much cleaner, done

Comment 19

[John Schoenick [:johns]]

Comment on attachment 8498617 [details] [diff] [review]
Part 2 - Line up with new spec for <img srcset> and <picture> mutations

[ obsolete w/ comment 18 ]

Comment 20

[John Schoenick [:johns]]

Comment on attachment 8501357 [details] [diff] [review]
Part 3 - Be more explicit about when we're falling back to synchronous behavior

[ obsolete w/ comment 17 ]

Comment 21

[John Schoenick [:johns]]

Attachment: Part 4 - Cycle collect HTMLImageElement/ResponsiveImageSelector r=mccr8

Addressed review comments

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8505013 [details] [diff] [review]
Part 1.99 - Add nsIDocument::IsCurrentActiveDocument

>+   * differentiate the *current* activate document from any active

s/Activate/Active/

>+      (inner->IsCurrentInnerWindow() ||

No, this isn't right.  We could have inner->IsCurrentInnerWindow(), but inner->GetDoc() != this, in which case this is not the active document.

>+        outer->GetCurrentInnerWindow()->GetDoc() == this));

On the other hand, this bit is, I think, not needed.  If our inner window is not current we can't be the active doc.  The case when a window is non-current but has an active document is when the document has gotten a new inner window, so the old inner still has an active doc but isn't current anymore.

So I think this test should just be:

  inner && inner->IsCurrentInnerWindow() && inner->GetDoc() == this

r=me with that

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Also, should we rev the IID for this class when adding a non-virtual?

No.  The vtable is no changing, so no need.

> .. if you have | dom.disable_image_src_set | set

Ah, fair.  OK.

The rest of comment 18 sounds great.

Comment 24

[John Schoenick [:johns]]

Attachment: Part 1.99 - Add nsIDocument::IsCurrentActiveDocument. r=bz

Addressed review comments

Comment 25

[John Schoenick [:johns]]

Attachment: Part 2 - Line up with new spec for <img srcset> and <picture> mutations. r=bz

Minor fix found by try: the QueueImageLoadTask() in UnbindFromTree() was missing a IsPictureEnabled check, causing unexpected pass on some platform tests

Comment 26

[John Schoenick [:johns]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/987133583ef3
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c739aa459fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/f43030eb22a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/73a28e6fbe84
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e0376b11638
https://hg.mozilla.org/integration/mozilla-inbound/rev/f81dc8f11838
https://hg.mozilla.org/integration/mozilla-inbound/rev/56cee6c6cd5c

Try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8af04bb0dc51

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/987133583ef3
https://hg.mozilla.org/mozilla-central/rev/4c739aa459fb
https://hg.mozilla.org/mozilla-central/rev/f43030eb22a4
https://hg.mozilla.org/mozilla-central/rev/73a28e6fbe84
https://hg.mozilla.org/mozilla-central/rev/7e0376b11638
https://hg.mozilla.org/mozilla-central/rev/f81dc8f11838
https://hg.mozilla.org/mozilla-central/rev/56cee6c6cd5c