Bug 165746 - Allow cross-document intersection observing
Summary: Allow cross-document intersection observing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ali Juma
URL:
Keywords: InRadar
Depends on: 190493
Blocks: 159475
  Show dependency treegraph
 
Reported: 2016-12-11 20:35 PST by Simon Fraser (smfr)
Modified: 2018-11-02 09:55 PDT (History)
15 users (show)

See Also:


Attachments
Patch (16.03 KB, patch)
2018-10-29 12:11 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.34 MB, application/zip)
2018-10-29 14:15 PDT, Build Bot
no flags Details
Patch (18.68 KB, patch)
2018-10-29 14:37 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (26.92 KB, patch)
2018-10-31 13:58 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch for landing (32.92 KB, patch)
2018-11-02 07:55 PDT, Ali Juma
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2016-12-11 20:35:57 PST
My first version of Intersection Observer will disallow cross-document observing, for simplicity. This bug tracks supporting cross-document observation.
Comment 1 Jeffrey Gilbert 2017-07-09 10:48:23 PDT
It's a tragedy this wont land in iOS 11 or Safari 11, those being non-evergreen browsers. My first request for this functionality was in 2014 and Safari is the only major browser to not implement a mechanism for cross document in view measurement. There are mechanisms for measurement all the way back to IE9 and tons of destructive/invasive hacks like frame busting and geometric guessing that are being used today, including in some cases measurement using plugins. 

It seems this is somehow a lower priority item than other enhancement requests, but Safari is most mobile web traffic and still relevant in desktop browser markets as well. So long as those browsers don't report in view dims, fraudsters will target iOS applications and safari web browsers, and safe measurement techniques will not be available, making energy/computationally expensive and potentially unsafe operations for measurement the only way to combat bad actors on the web.
Comment 2 Jeffrey Gilbert 2017-07-09 11:17:09 PDT
For more information about "Viewability" measurement guidelines, see the following pages provided by standards leaders in advertising, IAB and MRC found here:

https://www.iab.com/guidelines/iab-measurement-guidelines/

Desktop: http://mediaratingcouncil.org/Desktop-Display-Impression-Measurement-Guidelines-US%20(MMTF%20Public%20Comment%20Final).pdf

Mobile App: http://mediaratingcouncil.org/Mobile%20In-App%20Measurement%20Guidelines%20(MMTF%20Public%20Comment%20Draft%20Final).pdf

Mobile Web: http://mediaratingcouncil.org/Mobile%20Web%20Measurement%20Guidelines%20(MMTF%20Public%20Comment%20Draft%20Final).pdf

Mobile (Guidance): http://mediaratingcouncil.org/062816%20Mobile%20Viewable%20Guidelines%20Final.pdf

WICG developed standard and examples: https://github.com/WICG/IntersectionObserver

And here's where we stand today regarding universal availability on browsers: http://caniuse.com/#feat=intersectionobserver

A deep dive (video) into ad fraud can be found here: https://www.youtube.com/watch?v=tgtH1L57iLc

All this is to say, we very much are ready for this feature out in the wild. It's sorely needed on Safari browsers, and it is a high value feature for the mobile web and mobile app ecosystems and this helps reduce payouts for bad actors.
Comment 3 Radar WebKit Bug Importer 2017-07-18 11:37:45 PDT
<rdar://problem/33381360>
Comment 4 Jack Wellborn 2018-10-15 10:45:21 PDT
Hi Simon, I am stoked to see IntersectionObserver land as an experimental feature in (In reply to Simon Fraser (smfr) from comment #0)
> My first version of Intersection Observer will disallow cross-document
> observing, for simplicity. This bug tracks supporting cross-document
> observation.

Hi Simon, I am stoked to see IntersectionObserver land as an experimental feature in STP. I didn't notice this post until after I made this page https://jackwellborn.com/playground/Webkit/IntersectionObserver/top.html and saw the NaN for the iFrame. Do you have any sense of whether this feature will be released without cross-document support? In either case, is there any ballpark of when cross-document will be supported.

Thanks,

~Jack
Comment 5 Ali Juma 2018-10-29 12:11:52 PDT
Created attachment 353308 [details]
Patch
Comment 6 Build Bot 2018-10-29 14:15:37 PDT
Comment on attachment 353308 [details]
Patch

Attachment 353308 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9770994

New failing tests:
imported/w3c/web-platform-tests/intersection-observer/iframe-no-root.html
imported/w3c/web-platform-tests/intersection-observer/cross-origin-iframe.html
Comment 7 Build Bot 2018-10-29 14:15:39 PDT
Created attachment 353322 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 8 Ali Juma 2018-10-29 14:37:51 PDT
Created attachment 353323 [details]
Patch
Comment 9 Simon Fraser (smfr) 2018-10-30 13:11:09 PDT
Comment on attachment 353308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353308&action=review

> Source/WebCore/dom/Document.cpp:7615
> +static FloatRect convertFromRootContentsToFrameContents(const FloatRect& rootRect, const FrameView& frameView)
> +{
> +    if (frameView.frame().isMainFrame())
> +        return rootRect;
> +
> +    auto* ownerRenderer = frameView.frame().ownerRenderer();
> +    auto& parentFrameView = ownerRenderer->view().frameView();
> +    FloatRect parentAbsoluteRect = convertFromRootContentsToFrameContents(rootRect, parentFrameView);
> +    LayoutRect rect = LayoutRect(ownerRenderer->absoluteToLocalQuad(parentAbsoluteRect).boundingBox());
> +    rect.moveBy(-ownerRenderer->contentBoxLocation());
> +    return frameView.viewToContents(snappedIntRect(rect));
> +}

This seems to belong in the family of rootViewToContents/contentsToRootView etc. in ScrollView.h. Maybe put this there? We should probably have FloatRect versions of all those.
Comment 10 Ali Juma 2018-10-31 13:58:34 PDT
Created attachment 353529 [details]
Patch
Comment 11 Ali Juma 2018-10-31 13:59:34 PDT
Comment on attachment 353308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353308&action=review

>> Source/WebCore/dom/Document.cpp:7615
>> +}
> 
> This seems to belong in the family of rootViewToContents/contentsToRootView etc. in ScrollView.h. Maybe put this there? We should probably have FloatRect versions of all those.

Removed this, added a FloatRect version of rootViewToContents, and used that instead.
Comment 12 Simon Fraser (smfr) 2018-10-31 23:35:19 PDT
Comment on attachment 353529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353529&action=review

> Source/WebCore/dom/Document.cpp:7691
> +            FloatRect rootViewIntersectionRect = frameView.delegatesScrolling() ? rootAbsoluteIntersectionRect : frameView.contentsToView(rootAbsoluteIntersectionRect);

Is this correct on iOS WK2?

> Source/WebCore/page/FrameView.cpp:2020
> +        if (auto* page = frame().page()) {
> +            if (document->numberOfIntersectionObservers())
>                  page->addDocumentNeedingIntersectionObservationUpdate(*document);
> +            if (!frame().isMainFrame()) {
> +                if (auto* mainDocument = frame().mainFrame().document()) {
> +                    if (mainDocument->numberOfIntersectionObservers())
> +                        page->addDocumentNeedingIntersectionObservationUpdate(*mainDocument);
> +                }
> +            }

Does this handle the case of a intermediate frame (with no observers) changing? Also who triggers observers when an intermediate frame scrolls? Is there a test for that?
Comment 13 Ali Juma 2018-11-01 13:33:22 PDT
Comment on attachment 353529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353529&action=review

>> Source/WebCore/dom/Document.cpp:7691
>> +            FloatRect rootViewIntersectionRect = frameView.delegatesScrolling() ? rootAbsoluteIntersectionRect : frameView.contentsToView(rootAbsoluteIntersectionRect);
> 
> Is this correct on iOS WK2?

Yes, the delegatesScrolling() condition makes this work on iOS WK2.

>> Source/WebCore/page/FrameView.cpp:2020
>> +            }
> 
> Does this handle the case of a intermediate frame (with no observers) changing? Also who triggers observers when an intermediate frame scrolls? Is there a test for that?

Yes, this handles that case. When an intermediate frame scrolls or has its layout change for other reasons, it gets a FrameView::viewportContentsChanged call, and the logic here will ensure the main frame updates its observers (all cross-document observations are tracked by the main frame).

I'll add a test for this case before landing.
Comment 14 Ali Juma 2018-11-02 07:55:15 PDT
Created attachment 353701 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2018-11-02 09:55:24 PDT
Comment on attachment 353701 [details]
Patch for landing

Clearing flags on attachment: 353701

Committed r237737: <https://trac.webkit.org/changeset/237737>
Comment 16 WebKit Commit Bot 2018-11-02 09:55:26 PDT
All reviewed patches have been landed.  Closing bug.