LGTM3 On Thu, Nov 30, 2023 at 10:54 AM Mike Taylor <miketa...@chromium.org> wrote:
> LGTM2 > On 11/30/23 9:33 AM, Traian Captan wrote: > > You're welcome Rick! > > > On Thu, Nov 30, 2023 at 5:33 AM Rick Byers <rby...@chromium.org> wrote: > >> Thank you Traian! >> >> On Thu, Nov 30, 2023 at 7:27 PM Traian Captan <tcap...@chromium.org> >> wrote: >> >>> Hi Rick, >>> >>> Yes. I uploaded a CL that increases the spacer size by 30px: >>> https://chromium-review.googlesource.com/c/chromium/src/+/5075126 >>> >>> The tests are now failing as expected on Safari: >>> >>> https://wpt.fyi/results/html/semantics/embedded-content/the-img-element?label=pr_head&max-count=1&pr=43446 >>> >>> >>> On Thu, Nov 30, 2023 at 12:14 AM Rick Byers <rby...@chromium.org> wrote: >>> >>>> Interesting. Could you try to improve the tests to capture the interop >>>> difference and ensure passing reflects full conformance to the spec? We >>>> rely on WPTs as our quantifiable measure of interoperability... >>>> >>>> Rick >>>> >>>> On Thu, Nov 30, 2023 at 5:07 PM Traian Captan <tcap...@chromium.org> >>>> wrote: >>>> >>>>> Thank you Rick! >>>>> >>>>> I did some investigating into why WebKit is passing some of the new >>>>> WPTs for lazy loaded images. >>>>> I think it might be because WebKit is considering the edge as >>>>> inclusive, while Blink and Gecko do not. >>>>> In the following example if the spacer height is 100px Safari will >>>>> report intersecting as true while Chrome and FireFox would report it as >>>>> false. >>>>> If the height is increased to 101px, all 3 browsers will report the >>>>> intersection as false. >>>>> <!DOCTYPE html> >>>>> <style> >>>>> #scroller { width: 100px; height: 100px; overflow: scroll; >>>>> background-color: gray; } >>>>> #spacer { width: 50px; height: 100px; } >>>>> #target { width: 50px; height: 50px; background-color: green; } >>>>> </style> >>>>> <div id=scroller> >>>>> <div id=spacer></div> >>>>> <div id=target></div> >>>>> </div> >>>>> <script> >>>>> let entries = []; >>>>> window.onload = function() { >>>>> const observer = new IntersectionObserver( >>>>> callback, >>>>> { >>>>> rootMargin: "0px" >>>>> } >>>>> ); >>>>> observer.observe(target); >>>>> }; >>>>> function callback(entries) { >>>>> console.log(`isIntersecting = ${entries[0].isIntersecting}`); >>>>> } >>>>> </script> >>>>> >>>>> >>>>> >>>>> On Mon, Nov 27, 2023 at 11:40 PM Rick Byers <rby...@chromium.org> >>>>> wrote: >>>>> >>>>>> On Wed, Nov 22, 2023 at 2:37 PM Yoav Weiss <yoavwe...@chromium.org> >>>>>> wrote: >>>>>> >>>>>>> Thanks, that sounds like a strict improvement. >>>>>>> >>>>>>> On Wed, Nov 22, 2023 at 6:25 AM Traian Captan <tcap...@chromium.org> >>>>>>> wrote: >>>>>>> >>>>>>>> Yes, that's correct. >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Nov 21, 2023 at 9:18 PM Yoav Weiss <yoavwe...@chromium.org> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Do I understand correctly that currently lazy-loaded images in CSS >>>>>>>>> scrollers have suboptimal behavior and this would improve that without >>>>>>>>> potentially harming other cases? >>>>>>>>> >>>>>>>>> On Wed, Nov 22, 2023 at 1:55 AM Traian Captan < >>>>>>>>> tcap...@chromium.org> wrote: >>>>>>>>> >>>>>>>>>> Contact emails tcap...@chromium.org >>>>>>>>>> >>>>>>>>>> Explainer None >>>>>>>>>> >>>>>>>>> >>>>>>> A short (inline) explainer would help reviewers to understand e.g. >>>>>>> if this involves changes to the API surface that developers need to care >>>>>>> about. >>>>>>> Can you write a few words on the impact on developers? >>>>>>> >>>>>>> >>>>>>>>>> >>>>>>>>>> Specification https://html.spec.whatwg.org/#lazy-load-root-margin >>>>>>>>>> >>>>>>>>> >>>>>>> The spec doesn't mention anything specific around root margins or >>>>>>> scroll margins (other than the algorithm name). >>>>>>> Are these concepts interoperable? >>>>>>> >>>>>> >>>>>> I dug around a little to try to better understand this. The HTML spec >>>>>> does mention >>>>>> <https://html.spec.whatwg.org/#start-intersection-observing-a-lazy-loading-element> >>>>>> setting >>>>>> the "scrollMargin" on the IntersectionObserver, a new property we >>>>>> recently >>>>>> shipped (I2S >>>>>> <https://groups.google.com/a/chromium.org/g/blink-dev/c/Ax8rTBusZ4s/m/nogj-s-eGQAJ?utm_medium=email&utm_source=footer> >>>>>> ). >>>>>> While WebKit and Gecko aren't yet passing the WPT tests >>>>>> <https://wpt.fyi/results/intersection-observer?label=master&label=experimental&aligned&q=scroll-margin> >>>>>> for this yet, interestingly WebKit is already passing >>>>>> <https://wpt.fyi/results/html/semantics/embedded-content/the-img-element?label=master&label=experimental&aligned&q=image-loading-lazy-in-> >>>>>> most of the newly added >>>>>> <https://chromium-review.googlesource.com/c/chromium/src/+/5023396> >>>>>> WPTs for lazy loaded images in particular. So perhaps their >>>>>> implementation >>>>>> already handled this? >>>>>> >>>>>> Seems reasonable to me - LGTM1 >>>>>> >>>>>> >>>>>>>>>> >>>>>>>>>> Summary >>>>>>>>>> >>>>>>>>>> Changes the lazy load intersection observer's init dictionary to >>>>>>>>>> use a scrollMargin instead of a rootMargin. This allows lazy loading >>>>>>>>>> images >>>>>>>>>> contained inside CSS scrollers, like carousels, to load as expected >>>>>>>>>> when >>>>>>>>>> near the viewport instead of the current behavior where these images >>>>>>>>>> load >>>>>>>>>> when at least one pixel is intersecting the viewport. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Blink component Blink>Image >>>>>>>>>> <https://bugs.chromium.org/p/chromium/issues/list?q=component:Blink%3EImage> >>>>>>>>>> >>>>>>>>>> Search tags lazyload >>>>>>>>>> <https://chromestatus.com/features#tags:lazyload>, scrollmargin >>>>>>>>>> <https://chromestatus.com/features#tags:scrollmargin> >>>>>>>>>> >>>>>>>>>> TAG review None >>>>>>>>>> >>>>>>>>>> TAG review status Not applicable >>>>>>>>>> >>>>>>>>>> Risks >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Interoperability and Compatibility >>>>>>>>>> >>>>>>>>>> Overall low as scroll margin also applies to the root element >>>>>>>>>> thus not affecting lazy loading images that are currently loading >>>>>>>>>> with just >>>>>>>>>> a root margin. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> *Gecko*: Positive ( >>>>>>>>>> https://github.com/w3c/IntersectionObserver/issues/431) >>>>>>>>>> https://bugzilla.mozilla.org/show_bug.cgi?id=1864794 >>>>>>>>>> >>>>>>>>>> *WebKit*: Positive ( >>>>>>>>>> https://github.com/w3c/IntersectionObserver/issues/431#issuecomment-930602435 >>>>>>>>>> ) https://bugs.webkit.org/show_bug.cgi?id=264864 >>>>>>>>>> >>>>>>>>>> *Web developers*: Positive ( >>>>>>>>>> https://bugs.chromium.org/p/chromium/issues/detail?id=1391989) >>>>>>>>>> >>>>>>>>>> *Other signals*: >>>>>>>>>> >>>>>>>>>> WebView application risks >>>>>>>>>> >>>>>>>>>> Does this intent deprecate or change behavior of existing APIs, >>>>>>>>>> such that it has potentially high risk for Android WebView-based >>>>>>>>>> applications? >>>>>>>>>> >>>>>>>>>> None >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Debuggability >>>>>>>>>> >>>>>>>>>> None >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Will this feature be supported on all six Blink platforms >>>>>>>>>> (Windows, Mac, Linux, Chrome OS, Android, and Android WebView)? >>>>>>>>>> Yes >>>>>>>>>> >>>>>>>>>> Is this feature fully tested by web-platform-tests >>>>>>>>>> <https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md> >>>>>>>>>> ? Yes >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> https://wpt.fyi/results/html/semantics/embedded-content/the-img-element?label=master&label=experimental&aligned&q=image-loading-lazy-in- >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Flag name on chrome://flags LazyLoadScrollMargin >>>>>>>>>> >>>>>>>>>> Finch feature name None >>>>>>>>>> >>>>>>>>>> Non-finch justification >>>>>>>>>> >>>>>>>>>> This feature is behind an enabled-by-default flag that can be >>>>>>>>>> disabled if needed. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Requires code in //chrome? False >>>>>>>>>> >>>>>>>>>> Tracking bug >>>>>>>>>> https://bugs.chromium.org/p/chromium/issues/detail?id=1391989 >>>>>>>>>> >>>>>>>>>> Estimated milestones >>>>>>>>>> Shipping on desktop 121 >>>>>>>>>> DevTrial on desktop 121 >>>>>>>>>> Shipping on Android 121 >>>>>>>>>> DevTrial on Android 121 >>>>>>>>>> Shipping on WebView 121 >>>>>>>>>> >>>>>>>>>> Anticipated spec changes >>>>>>>>>> >>>>>>>>>> Open questions about a feature may be a source of future web >>>>>>>>>> compat or interop issues. Please list open issues (e.g. links to >>>>>>>>>> known >>>>>>>>>> github issues in the project for the feature specification) whose >>>>>>>>>> resolution may introduce web compat/interop risk (e.g., changing to >>>>>>>>>> naming >>>>>>>>>> or structure of the API in a non-backward-compatible way). >>>>>>>>>> None >>>>>>>>>> >>>>>>>>>> Link to entry on the Chrome Platform Status >>>>>>>>>> https://chromestatus.com/feature/5106926245642240 >>>>>>>>>> >>>>>>>>>> Links to previous Intent discussions Intent to prototype: >>>>>>>>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAFxahvtrmHkoOpTuD2eZYVOyFuAhP8ZFVoTuNBS8zYTVY%3DTaaQ%40mail.gmail.com >>>>>>>>>> >>>>>>>>>> This intent message was generated by Chrome Platform Status >>>>>>>>>> <https://chromestatus.com/>. >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> You received this message because you are subscribed to the >>>>>>>>>> Google Groups "blink-dev" group. >>>>>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>>>>> send an email to blink-dev+unsubscr...@chromium.org. >>>>>>>>>> To view this discussion on the web visit >>>>>>>>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAFxahvsUb0GEG9WNWRN7Akkowjm03gLj%2Biiq5rG8%2BzdAWMBTNA%40mail.gmail.com >>>>>>>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAFxahvsUb0GEG9WNWRN7Akkowjm03gLj%2Biiq5rG8%2BzdAWMBTNA%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>>>>>> . >>>>>>>>>> >>>>>>>>> -- >>>>>>> You received this message because you are subscribed to the Google >>>>>>> Groups "blink-dev" group. >>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>> send an email to blink-dev+unsubscr...@chromium.org. >>>>>>> To view this discussion on the web visit >>>>>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAL5BFfVhH_QxckxRLbR05jrN0CY48aQ-Ag3BypusnsyKoDTc0A%40mail.gmail.com >>>>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAL5BFfVhH_QxckxRLbR05jrN0CY48aQ-Ag3BypusnsyKoDTc0A%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>>> . >>>>>>> >>>>>> -- > You received this message because you are subscribed to the Google Groups > "blink-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to blink-dev+unsubscr...@chromium.org. > To view this discussion on the web visit > https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAFxahvuw19j2DwRAV4kGecr_V%2BZ2D89nW5PdSUJ1z43HG7JW8g%40mail.gmail.com > <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAFxahvuw19j2DwRAV4kGecr_V%2BZ2D89nW5PdSUJ1z43HG7JW8g%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > > -- > You received this message because you are subscribed to the Google Groups > "blink-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to blink-dev+unsubscr...@chromium.org. > To view this discussion on the web visit > https://groups.google.com/a/chromium.org/d/msgid/blink-dev/e34334bd-1d26-4f38-8e77-4540f2658fc5%40chromium.org > <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/e34334bd-1d26-4f38-8e77-4540f2658fc5%40chromium.org?utm_medium=email&utm_source=footer> > . > -- You received this message because you are subscribed to the Google Groups "blink-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscr...@chromium.org. To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAOMQ%2Bw_1p%2Bd2_QYVyhjXwGsrAop_ASY6xh9797eoQnxj2oaPkQ%40mail.gmail.com.
