Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node removal should not trigger "update the image data" #274

Open
yoavweiss opened this issue Dec 7, 2015 · 8 comments
Open

Node removal should not trigger "update the image data" #274

yoavweiss opened this issue Dec 7, 2015 · 8 comments

Comments

@yoavweiss
Copy link
Member

@yoavweiss yoavweiss commented Dec 7, 2015

Following an IRC discussion I don't think that node removal should trigger the "update the image data" algorithm as it may trigger spurious downloads, and there's no real benefit in doing that.

Once the image is removed from the picture parent, nothing will be displayed. Also, once it will get added elsewhere, "update the image data" algorithm should run.

Maybe we should just make sure that once it was removed from picture, the algo should run at next insertion (even if not to a picture parent or a separate document)?

@yoavweiss
Copy link
Member Author

@yoavweiss yoavweiss commented Dec 7, 2015

/cc @zcorpan

@zcorpan
Copy link

@zcorpan zcorpan commented Dec 7, 2015

It might not be displayed but it can be painted on a canvas and its naturalWidth etc can be read. I'm not convinced we should complicate this. Is it a problem in practice? The "relevant mutations" are intentionally pretty dumb to be more likely to be interoperable.

@yoavweiss
Copy link
Member Author

@yoavweiss yoavweiss commented Dec 7, 2015

I'd argue that it's more likely that a subtle "removing <img> from one parent and adding it to another" will cause spurious downloads than it is that handling of a detached img node would result in the wrong image being displayed by accident.

@yoavweiss
Copy link
Member Author

@yoavweiss yoavweiss commented Dec 7, 2015

More irc discussions lead to the conclusion that the above scenario won't trigger spurious downloads if done in the same script, but the "removed an <img> and forgot about it" would.

@yoavweiss
Copy link
Member Author

@yoavweiss yoavweiss commented Dec 7, 2015

A way to resolve this would be to "update the image data" with a null "selected source" in the removal scenario.

@zcorpan
Copy link

@zcorpan zcorpan commented Dec 7, 2015

That would mean an extra flag to "update the image data", and check it in step 3 and 8. Step 9 would then set the image to the broken state and fire an error event. (Maybe we should avoid the error event also?)

@yoavweiss
Copy link
Member Author

@yoavweiss yoavweiss commented Dec 7, 2015

Yeah, avoiding the error is probably better

@zcorpan
Copy link

@zcorpan zcorpan commented Dec 7, 2015

But that would still leave the image broken if you insert it to the document again... I'm still unconvinced we should change things here.

dstockwell pushed a commit to dstockwell/chromium that referenced this issue Dec 10, 2015
This CL (almost) aligns the HTMLImageElement's implementation of
relevant mutations with the spec [1] and ensures that the "update the
image data" algorithm fires when it should, and that it doesn't fire
when it shouldn't.

Specific changes:
* Avoid firing update when srcset and sizes are set to current value
* Fire an update when an image is inserted into a <picture>
* Fire a (null) update when an image is removed from a <picture>
* Avoid firing update when a source that's after the <img> was removed,
  inserted or changed

The part where this CL diverges from the spec is upon removal of an
HTMLImageElement from an HTMLPictureElement parent. Instead of running
a full "update the image data", we run it with a null "selected
source" to avoid spurious downloads. See [2] for details.

Compatibility:
* Firefox agrees with the spec on all these points, so this change
  increases compat there.
* The relevant mutations parts we change here are not yet implemented
  in WebKit.
* Latest MS Edge seems to be failing most tests that this CL fixes.

Test:
 Imported from https://github.com/w3c/web-platform-tests/blob/master/html/semantics/embedded-content/the-img-element/relevant-mutations.html

[1] https://html.spec.whatwg.org/multipage/embedded-content.html#relevant-mutations
[2] ResponsiveImagesCG/picture-element#274

BUG=418903

Review URL: https://codereview.chromium.org/1511523002

Cr-Commit-Position: refs/heads/master@{#364307}
dstockwell pushed a commit to dstockwell/chromium that referenced this issue Dec 14, 2015
… #6 id:100001 of https://codereview.chromium.org/1511523002/ )

Reason for revert:
Reverting due to esprehn's request

Original issue's description:
> Align HTMLImageElement relevant mutations to spec
>
> This CL (almost) aligns the HTMLImageElement's implementation of
> relevant mutations with the spec [1] and ensures that the "update the
> image data" algorithm fires when it should, and that it doesn't fire
> when it shouldn't.
>
> Specific changes:
> * Avoid firing update when srcset and sizes are set to current value
> * Fire an update when an image is inserted into a <picture>
> * Fire a (null) update when an image is removed from a <picture>
> * Avoid firing update when a source that's after the <img> was removed,
>   inserted or changed
>
> The part where this CL diverges from the spec is upon removal of an
> HTMLImageElement from an HTMLPictureElement parent. Instead of running
> a full "update the image data", we run it with a null "selected
> source" to avoid spurious downloads. See [2] for details.
>
> Compatibility:
> * Firefox agrees with the spec on all these points, so this change
>   increases compat there.
> * The relevant mutations parts we change here are not yet implemented
>   in WebKit.
> * Latest MS Edge seems to be failing most tests that this CL fixes.
>
> Test:
>  Imported from https://github.com/w3c/web-platform-tests/blob/master/html/semantics/embedded-content/the-img-element/relevant-mutations.html
>
> [1] https://html.spec.whatwg.org/multipage/embedded-content.html#relevant-mutations
> [2] ResponsiveImagesCG/picture-element#274
>
> BUG=418903
>
> Committed: https://crrev.com/cb0e14ed4b021eff0699175f0fe3ac222ae3978f
> Cr-Commit-Position: refs/heads/master@{#364307}

TBR=esprehn@chromium.org,mkwst@chromium.org,noel@chromium.org,tkent@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=418903

Review URL: https://codereview.chromium.org/1522253002

Cr-Commit-Position: refs/heads/master@{#365120}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.