Created attachment 393934 [details] Test Working with intersection Observer I have found an issue when zoom is changed. I have created an html file to demonstrate attached here. Steps To Reproduce: 1. Open HTML file with included JS code 2. Scroll down the page and notice how square change color when the top of the square touches the top of the viewport. 3. Change the Zoom level 4. Scroll again to see how the squares change color at a different timing (not top of square with top of viewport). Results: In Chrome regardless of the zoom level the squares change color when the top of the square touches the top of the viewport.
rdar://problem/60613094
Created attachment 394028 [details] Patch
Comment on attachment 394028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394028&action=review However, I think the bug report is about pinch-zoom (Page::setPageScaleFactor()). So this patch is OK but there's another one to do for pinch-zooming. > Source/WebCore/dom/Document.cpp:7563 > + expandRootBoundsWithRootMargin(localRootBounds, observer.rootMarginBox(), rootRenderer->style().effectiveZoom()); This is the zoom level that's affected the css zoom property, and (I think) Command-+ zoom. > LayoutTests/intersection-observer/root-margin-with-zoom.html:18 > + window.internals.setPageZoomFactor(2); This is also about Command-+ zooming
Comment on attachment 394028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394028&action=review > LayoutTests/intersection-observer/root-margin-with-zoom.html:1 > +<!DOCTYPE html> Would be nice to have an official WPT test too. Would https://drafts.csswg.org/css-device-adapt/#zoom-desc be usable ? > LayoutTests/intersection-observer/root-margin-with-zoom.html:5 > +#target { Can you please add meta tag pointing to spec reference? Can you also add a comment briefly explaining how the test is working? It seems there are subtle / non-obvious aspects... > LayoutTests/intersection-observer/root-margin-with-zoom.html:7 > + height: 40px; Should this be < 20px? otherwise it seems it would still intersect with a rootMargin: '20px 0px 0px 0px' (half the actual rootMargin).
Created attachment 394105 [details] Patch
(In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 394028 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394028&action=review > > However, I think the bug report is about pinch-zoom > (Page::setPageScaleFactor()). I can't reproduce any difference between Blink and WebKit when using pinch-zoom on this test case. IntersectionObserver generally ignores pinch-zoom, and computes intersections with the layout viewport (when there's no explicit root). So it's expected that the rect we're intersecting with won't match the visible viewport when zoomed in. > So this patch is OK but there's another one to do for pinch-zooming. > > > Source/WebCore/dom/Document.cpp:7563 > > + expandRootBoundsWithRootMargin(localRootBounds, observer.rootMarginBox(), rootRenderer->style().effectiveZoom()); > > This is the zoom level that's affected the css zoom property, and (I think) > Command-+ zoom. > > > LayoutTests/intersection-observer/root-margin-with-zoom.html:18 > > + window.internals.setPageZoomFactor(2); > > This is also about Command-+ zooming (In reply to Frédéric Wang (:fredw) from comment #4) > Comment on attachment 394028 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394028&action=review > > > LayoutTests/intersection-observer/root-margin-with-zoom.html:1 > > +<!DOCTYPE html> > > Would be nice to have an official WPT test too. Would > https://drafts.csswg.org/css-device-adapt/#zoom-desc be usable ? @viewport isn't supported in WebKit or Gecko, and is slated to be removed from the spec: https://github.com/w3c/csswg-drafts/issues/4766 The CSS zoom property would be another option, but this is also non-standard and not supported by Gecko. So there doesn't seem to be a good cross-platform way to set page zoom from web content. > > LayoutTests/intersection-observer/root-margin-with-zoom.html:5 > > +#target { > > Can you please add meta tag pointing to spec reference? Done. > Can you also add a comment briefly explaining how the test is working? It > seems there are subtle / non-obvious aspects... Done. > > LayoutTests/intersection-observer/root-margin-with-zoom.html:7 > > + height: 40px; > > Should this be < 20px? otherwise it seems it would still intersect with a > rootMargin: '20px 0px 0px 0px' (half the actual rootMargin). No. When we have a zoom factor of 2, the target's borderBoundingBox (this is in local coordinates) is also scaled by 2, so has height 80 px instead of 40 px. Once we convert this rect into root space (which, in this case, is layout viewport space), its origin is at y=-80px rather than y=-40px. In order for this to be contained within the expanded root rect, we need the root margin to be '40px 0px 0px 0px', so that when it's scaled by 2, it expands by 80px. A simpler way of saying this: the target is 40 CSS pixels above the viewport, so should intersect when using a root margin of '40px 0px 0px 0px', regardless of page zoom, since page zoom should equally affect the position/size of the target and the root margin.
Comment on attachment 394105 [details] Patch Oh right, I forgot that Int.Obs. was about layout viewport.
Committed r258787: <https://trac.webkit.org/changeset/258787> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394105 [details].
Committed r258791: <https://trac.webkit.org/changeset/258791>