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

Lazyload images #3752

Merged
merged 8 commits into from
Feb 12, 2020
Merged

Lazyload images #3752

merged 8 commits into from
Feb 12, 2020

Conversation

bengreenstein
Copy link
Contributor

@bengreenstein bengreenstein commented Jun 8, 2018

@domenic

This is a draft of spec changes to support a lazyload attribute in iframe and img elements.


Issue: #2806
Tests: https://chromium-review.googlesource.com/c/chromium/src/+/1417117 (wpt export)


/embedded-content.html ( diff )
/images.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/media.html ( diff )
/rendering.html ( diff )
/urls-and-fetching.html ( diff )

@domenic domenic added addition/proposal New features or enhancements topic: img labels Jun 12, 2018
@domenic
Copy link
Member

domenic commented Jun 12, 2018

I did another minor pass; PTAL.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some issues with the metadata fetcher

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metadata fetching looks good to me editorially now. Eager to hear others thoughts on whether it provides the right foundation for interoperability.

@domenic domenic dismissed their stale review June 28, 2018 21:17

Things were fixed

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to update the element indexes to include this attribute for img and iframe.

Should this influence https://fetch.spec.whatwg.org/#concept-request-priority somehow?

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@jakearchibald jakearchibald self-requested a review July 25, 2018 14:42
Copy link
Contributor

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: img.decode() waits for the image fetch to complete. With lazyload, this may take a long time, or might never happen. Is that the intent? Feels like decode() is a strong signal that the developer wants the image to fetch.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
<tr>
<td><dfn><code data-x="attr-lazyload-auto">auto</code></dfn>
<td><dfn data-x="attr-lazyload-auto-state">Auto</dfn>
<td>Indicates that the user agent may determine the fetching strategy (the default).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are cases where we need to define 'auto'. Expectations are pretty strong when it comes to new Image(). Perhaps "off" is the default unless the image was created by the regular document parser (not innerHTML and createContextualFragment).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of leaving auto free to experiment for now...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new Audio() sets preload=auto, maybe new Image() could set load=eager to match expectations.

source Outdated

<p>A <dfn>lazy loading attribute</dfn> is an <span>enumerated attribute</span>. The following
table lists the keywords and states for the attribute &mdash; the keywords in the left column map
to the states in the cell in the second column on the same row as the keyword.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this value is changed after element creation? Eg, if I switch from "on" to "off". Feels like the image should immediately load. Worth spelling out?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is explicit in the spec's

user agents must obtain images immediately when the CSS boxes those images intersect the viewport, or when the corresponding img element's lazyload attribute is in the Off state

But maybe we can add a clarifying note right after that paragraph, saying "This means that if the author changes the lazyload attribute's value to Off, and the image has not already been obtained, it will immediately be obtained."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a possibly common use case. Is it clear that changing the state is the way to load the image, or should there be a load() method (like for media elements)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that load() would conflict with load IDL attribute...

aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 16, 2018
This CL implements support for the "lazyload" attribute on frames,
according to whatwg/html#3752, and as part of
the LazyLoad feature. The accepted values are:

"on", which causes the browser to lazily load a frame even if it's
same-origin or nested inside another lazyloaded frame,

"off", which causes the browser to avoid lazily loading this frame or
any of it's children (unless those children are marked with
lazyload="on"),

"auto", which activates the default behavior, and is therefore not
explicitly handled in code.

Bug: 873358
Change-Id: I2fde65adb15216260291b08e39888a2363f44d4a
Reviewed-on: https://chromium-review.googlesource.com/1176293
Commit-Queue: Scott Little <sclittle@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583841}
aarongable pushed a commit to chromium/chromium that referenced this pull request Sep 19, 2018
This CL implements support for the "lazyload" attribute on images,
according to whatwg/html#3752, and as part of
the LazyLoad feature. The accepted values are:

"off", which causes the browser to avoid lazily loading the <img> element

"on" and "auto", activate the default behavior of lazily load the <img> element

When the attribute is changed to "off", the deferred image loads immediately.

Bug: 875080
Change-Id: I839926a9827d019f23aafc40f8315476fe1b3048
Reviewed-on: https://chromium-review.googlesource.com/1197782
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: rajendrant <rajendrant@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592599}
Copy link
Contributor

@sideshowbarker sideshowbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d suggest that since the change to the CONTRIBUTING.md and README.md files arenn’t directly related to the substance of the rest of this patch (lazyload feature), it’d be preferable for the CONTRIBUTING.md and README.md changes to be in separate patches from this patch. It’d be fine if they are just a separate commits as part of PR #3752 but not sure if that would happen, given that the policy here for PRs is that they result in a single (squashed) commit that gets merged to master

Copy link
Contributor

@sideshowbarker sideshowbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to dev/style.css for adding the syntax-highlighter styles have already been merged into master, so they should be dropped from this patch.

Copy link
Contributor

@sideshowbarker sideshowbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I’m now realizing that this reason some unrelated changes are showing up in the diff here are that this patch includes some merges? So it seems like it needs to be rebased against master to clear those away?

@annevk
Copy link
Member

annevk commented Dec 11, 2018

@bengreenstein could you rebase this against master and then force push the resulting changeset to get rid of the merge commits? As it stands it looks like this is proposing a number of changes against dev/styles.css for instance, which doesn't seem correct.

@sideshowbarker
Copy link
Contributor

This branch is now in a state that it can’t be rebased against master without multiple merge conflicts.

@bengreenstein
Copy link
Contributor Author

@sideshowbarker and @annevk Sorry about the state of the branch. Hopefully now it is all cleaned up.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some stuff to work out around images; I'm most concerned about srcset/picture.

Will look through other reviewers' comments and flag them also.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small things and one big thing: what to do for srcset and picture.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domenic
Copy link
Member

domenic commented Dec 21, 2018

OK, sweet! I pushed up a few minor tweaks of my own, almost all editorial. This now looks good to me.

@jakearchibald, I think we addressed your comments, so I will dismiss your review, but if you have the time, a re-review would be much appreciated.

From what I recall, this had pretty active and helpful support from WebKit at least in #2806. @smfr and/or @othermaciej, would you like to take a look at the results, probably using the diff links in the OP to see the rendered additions? From what I can see we haven't yet addressed the WebKit request to have a feature for a placeholder image while it is loading ("lowsrc"), but I think we went through a lot of the other questions in that thread and resolved them; please correct me if I'm wrong.

@bengreenstein, are there web platform tests for this yet?

@domenic domenic added the needs tests Moving the issue forward requires someone to write tests label Dec 21, 2018
@domenic domenic dismissed jakearchibald’s stale review December 21, 2018 16:23

Comments addressed, I think!

@BitPopCoin
Copy link

I'd like a call back I can insert into an anchor or something to let me know when a location has been reached.

aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 11, 2019
In previous CLs, support for the lazyLoad attribute was added, but the
corresponding idl attribute entries were mistakely forgotten. This CL
adds those attribute entries, making it possible to detect if the
browser supports native lazy loading from JavaScript.

Spec: whatwg/html#3752
ChromeStatus entry: https://www.chromestatus.com/feature/5645767347798016
Intent to implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/czmmZUd4Vww/1-H6j-zdAwAJ

Bug: 913162
Change-Id: Ib801c376ac70bf14eb2779e1642409f9cf03a95d
Reviewed-on: https://chromium-review.googlesource.com/c/1372534
Commit-Queue: Scott Little <sclittle@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622203}
rwlbuis added a commit to web-platform-tests/wpt that referenced this pull request Feb 13, 2020
whatwg/html#3752 landed, so
remove comments and tentative extension.
rwlbuis added a commit to web-platform-tests/wpt that referenced this pull request Feb 13, 2020
whatwg/html#3752 landed, so
remove comments and tentative extension.
rwlbuis added a commit to web-platform-tests/wpt that referenced this pull request Feb 13, 2020
whatwg/html#3752 landed, so
remove comments and tentative extension.
rwlbuis added a commit to web-platform-tests/wpt that referenced this pull request Feb 13, 2020
whatwg/html#3752 landed, so
remove comments and tentative extension.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 17, 2020
…=testonly

Automatic update from web-platform-tests
Remove tentative from lazy load tests (#21773)

whatwg/html#3752 landed, so
remove comments and tentative extension.
--

wpt-commits: 283eaa796815403a3e959861f6f1e61b0d35848f
wpt-pr: 21773


--HG--
rename : testing/web-platform/tests/loading/lazyload/common.js => testing/web-platform/tests/html/semantics/embedded-content/the-img-element/common.js
rename : testing/web-platform/tests/loading/lazyload/disconnected-image-loading-lazy.tentative.html => testing/web-platform/tests/html/semantics/embedded-content/the-img-element/disconnected-image-loading-lazy.html
rename : testing/web-platform/tests/loading/lazyload/resources/image-loading-lazy-below-viewport-iframe.html => testing/web-platform/tests/html/semantics/embedded-content/the-img-element/resources/image-loading-lazy-below-viewport-iframe.html
rename : testing/web-platform/tests/loading/lazyload/resources/image-loading-lazy-in-viewport-iframe.html => testing/web-platform/tests/html/semantics/embedded-content/the-img-element/resources/image-loading-lazy-in-viewport-iframe.html
rename : testing/web-platform/tests/loading/lazyload/resources/image.png => testing/web-platform/tests/html/semantics/embedded-content/the-img-element/resources/image.png
rename : testing/web-platform/tests/loading/lazyload/resources/newwindow.html => testing/web-platform/tests/html/semantics/embedded-content/the-img-element/resources/newwindow.html
rename : testing/web-platform/tests/loading/lazyload/resources/referrer-checker-img.py => testing/web-platform/tests/html/semantics/embedded-content/the-img-element/resources/referrer-checker-img.py
rename : testing/web-platform/tests/loading/lazyload/resources/subframe.html => testing/web-platform/tests/html/semantics/embedded-content/the-img-element/resources/subframe.html
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Feb 18, 2020
…=testonly

Automatic update from web-platform-tests
Remove tentative from lazy load tests (#21773)

whatwg/html#3752 landed, so
remove comments and tentative extension.
--

wpt-commits: 283eaa796815403a3e959861f6f1e61b0d35848f
wpt-pr: 21773
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Feb 18, 2020
…=testonly

Automatic update from web-platform-tests
Remove tentative from lazy load tests (#21773)

whatwg/html#3752 landed, so
remove comments and tentative extension.
--

wpt-commits: 283eaa796815403a3e959861f6f1e61b0d35848f
wpt-pr: 21773

UltraBlame original commit: e201cb3f54180dcb5ea5203f472035bca7a1da14
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Feb 18, 2020
…=testonly

Automatic update from web-platform-tests
Remove tentative from lazy load tests (#21773)

whatwg/html#3752 landed, so
remove comments and tentative extension.
--

wpt-commits: 283eaa796815403a3e959861f6f1e61b0d35848f
wpt-pr: 21773

UltraBlame original commit: e201cb3f54180dcb5ea5203f472035bca7a1da14
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Feb 18, 2020
…=testonly

Automatic update from web-platform-tests
Remove tentative from lazy load tests (#21773)

whatwg/html#3752 landed, so
remove comments and tentative extension.
--

wpt-commits: 283eaa796815403a3e959861f6f1e61b0d35848f
wpt-pr: 21773

UltraBlame original commit: e201cb3f54180dcb5ea5203f472035bca7a1da14
Copy link

@vertzjdean vertzjdean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont know what is going on but im not going to pay for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation needs tests Moving the issue forward requires someone to write tests topic: img
Development

Successfully merging this pull request may close these issues.