Closed Bug 1166138 Opened 5 years ago Closed 4 years ago

<img srcset> does not react to resize/viewport changes

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: johns, Assigned: Nika, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [lang=c++])

Attachments

(1 file, 5 obsolete files)

Per bug 1135812 comment 45 - we also need to trigger reselection of responsive images for simple <img srcset> sources when the viewport changes. This should mostly just be hooking up bug 1135812 to the proper responsive-mode-entry events in HTMLImageElement
Assignee: nobody → josh
I think the simplest way to do this is to replace | mResponsiveImageSelector = null | lines with a ClearResponsiveSelector(), then have TryCreateResponsiveSelector/ClearResponsiveSelector cover entry/exit from this mode.

Maybe double check that we have a responsive selector when doing e.g.

> <picture>
>   <img src="foo.jpg">
> </picture>

or other cases that are responsive but only using src, but I think we always should (maybe MOZ_ASSERT( !HaveSrcsetOrInPicture() || mResponsiveSelector ) in the image load task for good measure)
This seems to have been removed as a known issue in version 38.1. Does this mean it has been resolved?
No. That sounds like a mistake.
(In reply to Josh Matthews [:jdm] from comment #3)
> No. That sounds like a mistake.

Yes it must be the issue is still present in all later versions including 39 but is no longer mentioned as a known issue in the release notes.
I'm struggling to find time to work on this, so I'd be happy to help anybody else do so. Comment 1 is a good description; the relevant code is in dom/html/HTMLImageElement.cpp .
Mentor: josh
Whiteboard: [lang=c++]
Note: mResponsiveSelector, not mResponsiveImageSelector.
Here's my first take at getting this to work

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=363c60ae53d2
Attachment #8655008 - Flags: review?(josh)
Assignee: josh → michael
Updated tests and new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc0bb6ca2022
Attachment #8655008 - Attachment is obsolete: true
Attachment #8655008 - Flags: review?(josh)
Attachment #8655462 - Flags: review?(josh)
Comment on attachment 8655462 [details] [diff] [review]
Make img srcset react to resize/viewport changes

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

Can you add the assertion suggested in comment 1, too?

::: dom/html/HTMLImageElement.cpp
@@ +596,2 @@
>        aDocument->AddResponsiveContent(this);
> +      mInDocResponsiveContent = true;

I think we can remove this block if we move the adding bit to TryCreateResponsiveSelector.

@@ +998,5 @@
>      // source.
>      mResponsiveSelector->SetCandidatesFromSourceSet(aNewValue);
>    }
>  
> +  if (!mInDocResponsiveContent) {

Let's do this in TryCreateResponsiveSelector instead, after we've definitely created a selector and are doing responsive things.

::: dom/html/HTMLImageElement.h
@@ +358,5 @@
>  
>    static void MapAttributesIntoRule(const nsMappedAttributes* aAttributes,
>                                      nsRuleData* aData);
>  
> +  bool mInDocResponsiveContent;

nit: swap this and the next member for more efficient storage.

::: dom/html/test/test_bug1166138.html
@@ +25,5 @@
> +    var imgdef = "http://mochi.test:8888/tests/dom/html/test/file_bug1166138_def.png";
> +
> +    // Spins the event loop twice to wait for things to have cleared off of the event loop
> +    function spin() {
> +      return new Promise((a) => SimpleTest.executeSoon(() => SimpleTest.executeSoon(a)));

Why don't we wait for a load event instead? That seems less fragile to me.

::: dom/tests/mochitest/general/test_img_mutations.html
@@ +32,5 @@
>        ok(expectingLoads > 0, "expected load");
>        if (expectingLoads > 0) {
>          expectingLoads--;
>        }
> +      if (!expectingLoads && !expectingErrors && afterExpectCallback) {

I'd prefer the loud error we get if afterExpectCallback is null, rather than the silent timeout.

@@ +42,5 @@
>        ok(expectingErrors > 0, "expected error");
>        if (expectingErrors > 0) {
>          expectingErrors--;
>        }
> +      if (!expectingLoads && !expectingErrors && afterExpectCallback) {

Same here.

@@ +158,5 @@
>        info("test 8");
>        document.body.appendChild(img);
>        is(img.currentSrc, testPNG100, "Should still have testPNG100 as current request");
>  
> +      expectEvents(1, 0, nextTest);

It is not clear to me why we get a load event here, given step 5 of https://html.spec.whatwg.org/multipage/embedded-content.html#img-environment-changes and the block at http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLImageElement.cpp?force=1#1087

@@ +176,1 @@
>            expectEvents(1, 0, function() {

I think it's safer to write this as
expectEvents(1, 0, function() {
...
});
SpecialPowers.pushPrefEnv(...)
to ensure that we're already waiting for the load event when the preference change occurs.

@@ +176,5 @@
>            expectEvents(1, 0, function() {
>              is(img.currentSrc, testPNG200, "Should now have testPNG200 as current request");
> +            img.src = img.src;
> +            is(img.currentSrc, testPNG200, "Should still have testPNG200 as current request");
> +            expectEvents(1, 0, function() {

I'm... surprised that we get a load event here, for the same reason that I'm surprised we got one on rebinding to the document.
Attachment #8655462 - Flags: review?(josh) → feedback+
(In reply to Josh Matthews [:jdm] from comment #9)
> I'm... surprised that we get a load event here, for the same reason that I'm
> surprised we got one on rebinding to the document.

| img.src = img.src | Is a magic edge case in the spec, it always triggers a reload even when it would be a no-op. We handle this with special logic in ::SetAttr :-/
(In reply to Josh Matthews [:jdm] from comment #9)
> @@ +158,5 @@
> >        info("test 8");
> >        document.body.appendChild(img);
> >        is(img.currentSrc, testPNG100, "Should still have testPNG100 as current request");
> >  
> > +      expectEvents(1, 0, nextTest);
> 
> It is not clear to me why we get a load event here, given step 5 of
> https://html.spec.whatwg.org/multipage/embedded-content.html#img-environment-
> changes and the block at
> http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLImageElement.
> cpp?force=1#1087

QueueImageLoadTask always calls LoadImage with the force flag, so it fires an event even if the image did not change. IIRC, the 'relevant mutations' steps always trigger a re-load, but opportunistic image re-selection shouldn't be firing no-op events, so we may need to propagate a flag to QueueImageLoadTask to mark it as non-force.
Updated in response to review.
Attachment #8655462 - Attachment is obsolete: true
Attachment #8655681 - Flags: review?(josh)
Comment on attachment 8655681 [details] [diff] [review]
Make img srcset react to resize/viewport changes

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

Some more drive-by feedback. This looks good, assuming we're not culling too many load events with the LoadSelectedImage(force) change. (that "force" flag is misnamed, but that's not your doing)

::: dom/html/HTMLImageElement.cpp
@@ +932,5 @@
>      // In responsive mode we generally want to re-run the full
>      // selection algorithm whenever starting a new load, per
>      // spec. This also causes us to re-resolve the URI as appropriate.
> +    if (!UpdateResponsiveSource()) {
> +      return NS_OK;

I'm not sure about this - does this cause us to not fire some load events where we would before? Not sure how close we are (or were) to the spec here. aForce is being used below to force a reload for unchanged URIs as well by passing it on to LoadImage.

@@ +1059,5 @@
>  HTMLImageElement::UpdateResponsiveSource()
>  {
>    if (!IsSrcsetEnabled()) {
> +    RemoveResponsiveSelector();
> +    return false;

Shouldn't this return true if we had mResponsiveSelector set?

@@ +1123,3 @@
>    }
> +
> +  return true;

Similarly, this is false if we started with a selector but now have null.
Attachment #8655681 - Flags: feedback+
This should hopefully break less things now. I haven't had a chance to push it to try yet because tree closage, but I think I've also responded to your comments.
Attachment #8655681 - Attachment is obsolete: true
Attachment #8655681 - Flags: review?(josh)
Attachment #8656132 - Flags: review?(josh)
Comment on attachment 8656132 [details] [diff] [review]
Make img srcset react to resize/viewport changes

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

I think we're almost there :)

::: dom/html/HTMLImageElement.cpp
@@ +601,5 @@
> +    if (HTMLPictureElement::IsPictureEnabled() &&
> +        aParent && aParent->IsHTMLElement(nsGkAtoms::picture)) {
> +      QueueImageLoadTask(true);
> +    } else {
> +      QueueImageLoadTask(false);

I would be inclined to write this as
bool forceLoadEvent = HTMLPictureElement::IsPictureEnabled() &&
    aParent && aParent->IsHTMLElement(nsGkAtoms::picture);
QueueImageLoadTask(forceLoadEvent);

@@ +1107,5 @@
>      candidateSource = this;
>    }
>  
>    while (candidateSource) {
> +    printf("candidateSource = %lx - currentSource = %lx\n", (uintptr_t) candidateSource, (uintptr_t) currentSource);

Remove this?

@@ +1153,5 @@
>      // Ran out of siblings without finding ourself, e.g. XBL magic.
>      mResponsiveSelector = nullptr;
>    }
> +
> +  return !hadSelector || mResponsiveSelector;

If we started out with no selector and ended up with no selector, we're not fulfilling the documented return contract. Should we instead compare mResponsiveSelector against the initial value of mResponsiveSelector?

@@ +1326,5 @@
>  
>  void
>  HTMLImageElement::MediaFeatureValuesChanged()
>  {
> +  QueueImageLoadTask(true);

According to https://html.spec.whatwg.org/multipage/embedded-content.html#img-environment-changes we should probably pass `false` here instead.

::: dom/html/HTMLImageElement.h
@@ +291,5 @@
>    bool InResponsiveMode();
>  
>    // Resolve and load the current mResponsiveSelector (responsive mode) or src
>    // attr image.
> +  nsresult LoadSelectedImage(bool aForce, bool aNotify, bool aAlwaysLoad = false);

Why the default value?

::: dom/html/test/test_bug1166138.html
@@ +101,5 @@
> +
> +      SpecialPowers.pushPrefEnv({'set': [['layout.css.devPixelsPerPx', 1]]});
> +      yield spin(true);
> +      is(newImage.currentSrc, img1x, "new image after switching to 1x");
> +      is(image.currentSrc, img1x, "new image after switching to 2x");

"old image after switching to 1x" would be more correct.
Attachment #8656132 - Flags: review?(josh) → feedback+
(In reply to Josh Matthews [:jdm] from comment #16)
> Comment on attachment 8656132 [details] [diff] [review]
> Make img srcset react to resize/viewport changes
> 
> Review of attachment 8656132 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we're almost there :)
> 
> ::: dom/html/HTMLImageElement.cpp
> @@ +601,5 @@
> > +    if (HTMLPictureElement::IsPictureEnabled() &&
> > +        aParent && aParent->IsHTMLElement(nsGkAtoms::picture)) {
> > +      QueueImageLoadTask(true);
> > +    } else {
> > +      QueueImageLoadTask(false);
> 
> I would be inclined to write this as
> bool forceLoadEvent = HTMLPictureElement::IsPictureEnabled() &&
>     aParent && aParent->IsHTMLElement(nsGkAtoms::picture);
> QueueImageLoadTask(forceLoadEvent);

Much better

> @@ +1107,5 @@
> >      candidateSource = this;
> >    }
> >  
> >    while (candidateSource) {
> > +    printf("candidateSource = %lx - currentSource = %lx\n", (uintptr_t) candidateSource, (uintptr_t) currentSource);
> 
> Remove this?

Oops

> @@ +1153,5 @@
> >      // Ran out of siblings without finding ourself, e.g. XBL magic.
> >      mResponsiveSelector = nullptr;
> >    }
> > +
> > +  return !hadSelector || mResponsiveSelector;
> 
> If we started out with no selector and ended up with no selector, we're not
> fulfilling the documented return contract. Should we instead compare
> mResponsiveSelector against the initial value of mResponsiveSelector?

I think this is correct. But you can correct me if it isn't :S

If we get to this point, either we cleared the mResponsiveSelector, or we
changed the source (that this returns true if the source changes, not
if the selector changes - the source can change if the selector changes). 
In the case where the source is the same, we returned early in the if 
(isUsableCandidate) block.

> @@ +1326,5 @@
> >  
> >  void
> >  HTMLImageElement::MediaFeatureValuesChanged()
> >  {
> > +  QueueImageLoadTask(true);
> 
> According to
> https://html.spec.whatwg.org/multipage/embedded-content.html#img-environment-
> changes we should probably pass `false` here instead.

Ok

> ::: dom/html/HTMLImageElement.h
> @@ +291,5 @@
> >    bool InResponsiveMode();
> >  
> >    // Resolve and load the current mResponsiveSelector (responsive mode) or src
> >    // attr image.
> > +  nsresult LoadSelectedImage(bool aForce, bool aNotify, bool aAlwaysLoad = false);
> 
> Why the default value?

That's a good question. Fixed.

> ::: dom/html/test/test_bug1166138.html
> @@ +101,5 @@
> > +
> > +      SpecialPowers.pushPrefEnv({'set': [['layout.css.devPixelsPerPx', 1]]});
> > +      yield spin(true);
> > +      is(newImage.currentSrc, img1x, "new image after switching to 1x");
> > +      is(image.currentSrc, img1x, "new image after switching to 2x");
> 
> "old image after switching to 1x" would be more correct.

Oops


new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67513d058c09
Attachment #8656132 - Attachment is obsolete: true
Attachment #8670356 - Flags: review?(josh)
Duplicate of this bug: 1222335
Comment on attachment 8670356 [details] [diff] [review]
Make img srcset react to resize/viewport changes

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

Soooooooooorry. Let's land this thing :)
Attachment #8670356 - Flags: review?(josh) → review+
https://hg.mozilla.org/mozilla-central/rev/1c1cde95bfa6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Depends on: 1259482
This appears to have caused a regression in thousands of sites using a couple of common commerce libraries. See bug 1259482
Attached patch Make test not racey. v1 (obsolete) — Splinter Review
Depending on the chunking and timing of the HTML parser, we may end up
firing onload on the image before the script tag is evaluated, leading
to an undefined onLoad (which is the intermittent failure in the test).

MozReview-Commit-ID: 78OAZan1xbC
Attachment #8969485 - Flags: review?(nika)
Comment on attachment 8969485 [details] [diff] [review]
Make test not racey. v1

Er, meant to post this on bug 1240225.
Attachment #8969485 - Attachment is obsolete: true
Attachment #8969485 - Flags: review?(nika)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.