1076583 - Update <img> load model (non-responsive case) to match new spec

Status: NEW

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

Description

[John Schoenick [:johns]]

In https://www.w3.org/Bugs/Public/show_bug.cgi?id=24711

The image loading model was greatly updated. Bug 1037643 implements a lot of this for the responsive (<img srcset>/<picture>) case -- The simple img.src case should have its special cases removed and ride along with that algorithm, and the remaining differences fixed.

Comment 1

[John Schoenick [:johns]]

I'm not currently actively working on this

Comment 2

[Franziskus Kiefer [:franziskus]]

Hi Seth,
we have a problem with the referrer policy here. Similar to the crossorigin attribute it affects the image request but the request is sent out before it is actually set (we kick of the load as soon as src is set). Do you know if there's any progress here? We don't really want to do something like [1].
thanks

[1] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/dom/html/HTMLImageElement.cpp#462

Comment 3

[Edgar Chen [:edgar]]

Attachment: WIP, Patch, v1

Comment 4

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #3)
> Created attachment 8819821 [details] [diff] [review]
> WIP, Patch, v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8139d43901ca71293534dac97d21853ff89a9ee2&filter-tier=1&group_state=expanded

Comment 5

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #4)
> (In reply to Edgar Chen [:edgar][:echen] from comment #3)
> > Created attachment 8819821 [details] [diff] [review]
> > WIP, Patch, v1
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8139d43901ca71293534dac97d21853ff89a9ee2&filter-
> tier=1&group_state=expanded

Some tests got failed because there is still a case that image will be loaded synchronously, see step 5.3 of https://html.spec.whatwg.org/multipage/embedded-content.html#updating-the-image-data. We need to handle only-src-and-image-is-in-cache case.

Comment 6

[Edgar Chen [:edgar]]

Attachment: WIP, Patch, v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91453e19e905446d1c4be6d4bc3b2d8d3c8e28c0&filter-tier=1&group_state=expanded

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 1: Rename QueueImageLoadTask to UpdateImage, v1

Comment 8

[Edgar Chen [:edgar]]

Attachment: Part 2: Load image synchronously if it is in the image cache, v1

Comment 9

[Edgar Chen [:edgar]]

Attachment: Part 3: Reset animations when load image, v1

Comment 10

[Edgar Chen [:edgar]]

Attachment: Part 4: If an adoption of <img> is happening with the different source and destination document, we need to force it to reload, v1

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 5: Update non-responsive mode to match new spec, v1

Comment 12

[Edgar Chen [:edgar]]

Attachment: Part 6: Don't draw image if image has pending load task, v1

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 5: Update .complete logic to match spec more, v2

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 6: Update non-responsive mode to match new spec, v2

Comment 15

[Edgar Chen [:edgar]]

Attachment: Part 7: Don't draw image if image has pending load task, v1

Comment 16

[Edgar Chen [:edgar]]

Attachment: Part 8: Update test_bug1118689.html since now we only load image in active document per spec, v1

Comment 17

[Edgar Chen [:edgar]]

Attachment: Part 9: Add img.comlete test for cached image, v1

Comment 18

[Edgar Chen [:edgar]]

Attachment: Part 10: Add tests for img adoption, v1

Comment 19

[Edgar Chen [:edgar]]

TODO: maybe add some tests for resetting animations.

Comment 20

[Edgar Chen [:edgar]]

Attachment: Part 6: Update non-responsive mode to match new spec, v3

Comment 21

[Edgar Chen [:edgar]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=993fa0f214f121601dbfbd73312e24de349d4abf&filter-tier=1&group_state=expanded

Comment 22

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #19)
> TODO: maybe add some tests for resetting animations.

I tried to add a test which uses SpecialPowers.snapshotWindow() to get a snapshot and do some checks, but it is hard to guarantee that we can get "correct" snapshot for reset case. It can easily result in intermittent failures. Filed bug 1332582 to figure out how to add a test in a better way.

Comment 23

[Josh Matthews [:jdm]]

Comment on attachment 8827392 [details] [diff] [review]
Part 2: Load image synchronously if it is in the image cache, v1

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

Clearing review until we investigate the expected events in other browsers.

::: dom/html/HTMLImageElement.cpp
@@ +920,5 @@
> +      // see https://bugzilla.mozilla.org/show_bug.cgi?id=1135313.
> +      if (nsContentUtils::IsImageInCache(uri, doc)) {
> +        // A hack to get animations to reset. See bug 594771.
> +        mNewRequestsWillNeedAnimationReset = true;
> +        LoadImage(uri, true, true, eImageLoadType_Normal);

One way this doesn't match the spec is that it dispatches loadstart/loadend messages. Step 5 only wants a load event that's dispatched asynchronously. We should see what events other browsers fire in this case.

Comment 24

[Josh Matthews [:jdm]]

Comment on attachment 8827393 [details] [diff] [review]
Part 3: Reset animations when load image, v1

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

::: dom/html/HTMLImageElement.cpp
@@ +1009,2 @@
>        rv = LoadImage(url, aForce, aNotify, eImageLoadType_Imageset);
> +      mNewRequestsWillNeedAnimationReset = false;

Rather than repeating this code in various places, we should have a LoadImageAndResetAnimation method which encapsulates this behaviour.

Additionally, according to the spec the only time we should be restarting the animation is when the image's src attribute is set to the same value as previously. All other relevant mutations do not set the "restart animations" flag.

Comment 25

[Josh Matthews [:jdm]]

Comment on attachment 8828270 [details] [diff] [review]
Part 5: Update .complete logic to match spec more, v2

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

::: dom/html/HTMLImageElement.cpp
@@ +204,5 @@
>      return true;
>    }
>  
> +  nsAutoString src;
> +  if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src) && src.IsEmpty()) {

https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-complete says we need to check for srcset here too.

@@ +215,5 @@
> +  }
> +
> +  // Note: It is still not clear what is the definition of "completely available".
> +  // See https://www.w3.org/Bugs/Public/show_bug.cgi?id=27484.
> +  return (!mPendingImageLoadTask && !mPendingRequest &&

It's not clear to me that we should check mPendingImageLoadTask unless there are tests showing that this is compatible with other browsers' behaviour. Do such tests exist?

Comment 26

[Josh Matthews [:jdm]]

Comment on attachment 8828277 [details] [diff] [review]
Part 7: Don't draw image if image has pending load task, v1

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

We should file an issue in web-platform-tests and ask zcorpan about 2d.drawImage.incomplete.reload.html. My reading of the spec doesn't agree with the comments inside the test - if we run "update the image data" synchronously, I don't see text that instructs us to set the image to the unavailable state in the steps that occur before "await a stable state", which is when we are supposed to return control to JS.

::: dom/html/HTMLImageElement.h
@@ +274,5 @@
>  
> +  // When an img element has experienced relevant mutations, if the user agent
> +  // only obtains images on demand, the img element must return to the
> +  // unavailable state.
> +  // https://html.spec.whatwg.org/multipage/embedded-content.html#when-to-obtain-images

The second sentence of this text says "In a browsing context where scripting is enabled, user agents must obtain images immediately." The applicable text in this section says nothing about the unavailable state, in this case.

Comment 27

[Simon Pieters [:zcorpan]]

I filed https://github.com/w3c/web-platform-tests/issues/4648

Comment 28

[Josh Matthews [:jdm]]

Comment on attachment 8828278 [details] [diff] [review]
Part 8: Update test_bug1118689.html since now we only load image in active document per spec, v1

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

::: dom/base/test/test_bug1118689.html
@@ -12,5 @@
>  
>    /** Test for Bug 1118689 **/
>    SimpleTest.requestFlakyTimeout("Just need some random timeout.");
>  
> -  function test1() {

I'm having trouble figuring out what change from the earlier patches made is necessary to remove this test. Could you point it out?

Comment 29

[Josh Matthews [:jdm]]

Comment on attachment 8828286 [details] [diff] [review]
Part 10: Add tests for img adoption, v1

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

::: testing/web-platform/tests/html/semantics/embedded-content/the-img-element/adoption.html
@@ +89,5 @@
> +      p.appendChild(i);
> +    });
> +  }, 'adoption is from appendChild');
> +
> +  done();

I don't think this or explicit_done should be necessary in this test. What happens if you remove it?

Comment 30

[Josh Matthews [:jdm]]

Comment on attachment 8828679 [details] [diff] [review]
Part 6: Update non-responsive mode to match new spec, v3

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

My biggest concern about this change is that we'll hit web-compatibility problems and need to back it out quickly. I think we should make it possible to revert to the synchronous behaviour by using a preference, and retain tests that verify that we do not break the old behaviour while the preference exists. Given this request, I did not spend much time looking through the changes in HTMLImageElement.cpp since I expect the necessary changes will be nontrivial.

Comment 31

[Edgar Chen [:edgar]]

(In reply to Josh Matthews [:jdm] from comment #23)
> Comment on attachment 8827392 [details] [diff] [review]
> Part 2: Load image synchronously if it is in the image cache, v1
> 
> Review of attachment 8827392 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/html/HTMLImageElement.cpp
> @@ +920,5 @@
> > +      // see https://bugzilla.mozilla.org/show_bug.cgi?id=1135313.
> > +      if (nsContentUtils::IsImageInCache(uri, doc)) {
> > +        // A hack to get animations to reset. See bug 594771.
> > +        mNewRequestsWillNeedAnimationReset = true;
> > +        LoadImage(uri, true, true, eImageLoadType_Normal);
> 
> One way this doesn't match the spec is that it dispatches loadstart/loadend
> messages. Step 5 only wants a load event that's dispatched asynchronously.

Ah, good catch! We don't match step 5. :(
Since we have this issue even without these changes, do you think we could file a separated bug and fix there?

> We should see what events other browsers fire in this case.

It looks like both Chrome and Edge doesn't support loadstart/loadend event yet (test page: https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4898).

Comment 32

[Edgar Chen [:edgar]]

(In reply to Josh Matthews [:jdm] from comment #25)
> Comment on attachment 8828270 [details] [diff] [review]
> Part 5: Update .complete logic to match spec more, v2
> 
> Review of attachment 8828270 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/html/HTMLImageElement.cpp
> @@ +215,5 @@
> > +  }
> > +
> > +  // Note: It is still not clear what is the definition of "completely available".
> > +  // See https://www.w3.org/Bugs/Public/show_bug.cgi?id=27484.
> > +  return (!mPendingImageLoadTask && !mPendingRequest &&
> 
> It's not clear to me that we should check mPendingImageLoadTask unless there
> are tests showing that this is compatible with other browsers' behaviour. Do
> such tests exist?

If we move non-responsive mode to async load and don't check mPendingImageLoadTask, the img.complete will remain true when we check img.complete right after changing src. And the "async src complete test" in http://w3c-test.org/html/semantics/embedded-content/the-img-element/img.complete.html will fail. Chrome and Edge both pass the test.

But img.complete for responsive mode is a bit difference on each browser.
Take https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4899 as example,
- Chrome: false false
- Edge: false true
- Firefox: true true

And we will have same result as Chrome if we check mPendingImageLoadTask.

Comment 33

[Josh Matthews [:jdm]]

https://bugs.webkit.org/show_bug.cgi?id=76102 and https://bugs.chromium.org/p/chromium/issues/detail?id=458851 are kind of depressing.

I think it would be worthwhile to fix the incompatibility in our implementation in this bug, rather than putting it off and potentially forgetting about it.

Comment 34

[Edgar Chen [:edgar]]

(In reply to Josh Matthews [:jdm] from comment #28)
> Comment on attachment 8828278 [details] [diff] [review]
> Part 8: Update test_bug1118689.html since now we only load image in active
> document per spec, v1
> 
> Review of attachment 8828278 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/test/test_bug1118689.html
> @@ -12,5 @@
> >  
> >    /** Test for Bug 1118689 **/
> >    SimpleTest.requestFlakyTimeout("Just need some random timeout.");
> >  
> > -  function test1() {
> 
> I'm having trouble figuring out what change from the earlier patches made is
> necessary to remove this test. Could you point it out?

In part6, the non-responsive mode will also call UpdateImage(), and UpdateImage() won't do anything if IsCurrentActiveDocument() returns false.

Comment 35

[Edgar Chen [:edgar]]

I rewrite some parts that make it possible to revert to old behaviour by using a preference.

Comment 36

[Edgar Chen [:edgar]]

Attachment: Part 2: Add pref for switching image loading algorithm, v1

Comment 37

[Edgar Chen [:edgar]]

Attachment: Part 3: Load image synchronously if it is in the image cache, v2

Comment 38

[Edgar Chen [:edgar]]

Attachment: Part 4: Reset animations when the image's src attribute is set to the same value as previously, v2

Comment 39

[Edgar Chen [:edgar]]

Attachment: Part 5: If an adoption of <img> is happening with the different source and destination document, we need to force it to reload, v2

Comment 40

[Edgar Chen [:edgar]]

Attachment: Part 6: Update non-responsive mode to match new spec in newer loading algorithm, v4

Comment 41

[Edgar Chen [:edgar]]

Attachment: Part 7: Update .complete logic to match spec more, v3

Comment 42

[Edgar Chen [:edgar]]

Attachment: Part 8: Run image tests on both loading algorithm, v2

Comment 43

[Edgar Chen [:edgar]]

Attachment: Part 10: Add tests for img adoption, v2, r=jdm

(In reply to Josh Matthews [:jdm] from comment #29)
> I don't think this or explicit_done should be necessary in this test. What
> happens if you remove it?

We don't need them. Removed.

Comment 44

[Edgar Chen [:edgar]]

Attachment: Part 11: Add tests for progress event, v1

Comment 45

[Edgar Chen [:edgar]]

Comment on attachment 8845830 [details] [diff] [review]
Part 2: Add pref for switching image loading algorithm, v1

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

This part adds "dom.image.newer.loading" pref for switching image loading algorithm and false by default.
I plan enable pref on a separated bug, so if any web-compatibility problem occurs, we could back pref change out and reopen that bug only.

Comment 46

[Edgar Chen [:edgar]]

Comment on attachment 8845833 [details] [diff] [review]
Part 3: Load image synchronously if it is in the image cache, v2

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

1). Keep old behaviour if nsContentUtils::ImageNewerLoading() returns false
2). Address comment #23,
    - Don't fire loadstart and loadend event if image is from "the list of available images".
    - I have added a test case for loadstart and loadend event (also include this case) in part 11.

Comment 47

[Edgar Chen [:edgar]]

Comment on attachment 8845835 [details] [diff] [review]
Part 4: Reset animations when the image's src attribute is set to the same value as previously, v2

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

Address comment #24,
- Reset animations only when the image's src attribute is set to the same value as previously.
- Also guard the changes with nsContentUtils::ImageNewerLoading().

Comment 48

[Edgar Chen [:edgar]]

Comment on attachment 8845837 [details] [diff] [review]
Part 5: If an adoption of <img> is happening with the different source and destination document, we need to force it to reload, v2

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

This part is original part 4 (Attachment 8827395 [details] [diff]) and r+ed.
But in order to switch to old behaviour completely, we need to guard the changes with nsContentUtils::ImageNewerLoading(), so r? again.

Comment 49

[Edgar Chen [:edgar]]

Comment on attachment 8845838 [details] [diff] [review]
Part 6: Update non-responsive mode to match new spec in newer loading algorithm, v4

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

Address comment #30
- All changes guard with nsContentUtils::ImageNewerLoading().

Comment 50

[Edgar Chen [:edgar]]

Comment on attachment 8845839 [details] [diff] [review]
Part 7: Update .complete logic to match spec more, v3

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

Address comment #25
- Also check srcset for src is empty case
- The reason why we need to check mPendingImageLoadTask in new loading algorithm is in comment #32.

Comment 51

[Edgar Chen [:edgar]]

Comment on attachment 8845841 [details] [diff] [review]
Part 8: Run image tests on both loading algorithm, v2

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

In this part, I extend img mochitest tests to run tests in both mode (pref on and off).
- Most cases behave the same in both mode
- expect,
  1). test_bug1118689.html: in newer loading, we won't try to load image if document is not active one (see also comment #34).
  2). test_img_mutations.html: img without <picture> and srcset attribute becomes asynchronous loading.

Comment 52

[Edgar Chen [:edgar]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ad44407a4becacc8d115cd0ba4bbc16d29b5442&filter-tier=1&group_state=expanded

Comment 53

[Edgar Chen [:edgar]]

Attachment: Part 11: Add tests for progress event, v2

Comment 54

[Edgar Chen [:edgar]]

Comment on attachment 8846943 [details] [diff] [review]
Part 11: Add tests for progress event, v2

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

This part adds wpt for loadstart and loadend event.
- The "cached img" test only get passed with newer loading, so set dom.image.newer.loading to true in .ini.
- And the "invalid src" test will timeout due to no "error" event received.
  It is because bug 1264768 was backed out. Once bug 1264768 is getting re-land (in bug 1329603), we will no longer get timeout.

Comment 55

[Edgar Chen [:edgar]]

There is a possible spec issue: https://github.com/whatwg/html/issues/2429

Comment 57

[Timothy Nikkel (:tnikkel)]

Should we pick this back up? I think this would fix bug 1572360 for example.