Bug 209264

Summary: Intersection Observer intersections are wrong with zooming
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: DOMAssignee: Ali Juma <ajuma>
Status: RESOLVED FIXED    
Severity: Normal CC: ajuma, cdumez, esprehn+autocc, ews-watchlist, fred.wang, kangil.han, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=209947
Attachments:
Description Flags
Test
none
Patch
none
Patch none

Simon Fraser (smfr)
Reported 2020-03-18 18:47:31 PDT
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.
Attachments
Test (2.03 KB, text/html)
2020-03-18 18:47 PDT, Simon Fraser (smfr) 		no flags
Details
Patch (4.49 KB, patch)
2020-03-19 15:01 PDT, Ali Juma 		no flags
DetailsFormatted DiffDiff
Patch (4.89 KB, patch)
2020-03-20 11:42 PDT, Ali Juma 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1 2020-03-18 18:47:51 PDT
rdar://problem/60613094
Ali Juma
Comment 2 2020-03-19 15:01:19 PDT
Created attachment 394028 [details] Patch
Simon Fraser (smfr)
Comment 3 2020-03-19 19:58:52 PDT
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
Frédéric Wang (:fredw)
Comment 4 2020-03-19 20:07:29 PDT
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).
Ali Juma
Comment 5 2020-03-20 11:42:08 PDT
Created attachment 394105 [details] Patch
Ali Juma
Comment 6 2020-03-20 11:42:24 PDT
(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.
Simon Fraser (smfr)
Comment 7 2020-03-20 13:14:51 PDT
Comment on attachment 394105 [details] Patch Oh right, I forgot that Int.Obs. was about layout viewport.
EWS
Comment 8 2020-03-20 13:58:59 PDT
Committed r258787: <https://trac.webkit.org/changeset/258787> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394105 [details].
Ali Juma
Comment 9 2020-03-20 14:54:54 PDT
Committed r258791: <https://trac.webkit.org/changeset/258791>
Note You need to log in before you can comment on or make changes to this bug.