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

IntersectionObserver V2: Detecting occlusion and visual effects #109

Closed
szager-chromium opened this issue Nov 9, 2018 · 27 comments · Fixed by #1025
Closed

IntersectionObserver V2: Detecting occlusion and visual effects #109

szager-chromium opened this issue Nov 9, 2018 · 27 comments · Fixed by #1025
Labels
position: positive ready to add Appears ready to add to the table of positions. venue: W3C Specifications in W3C Working Groups

Comments

@szager-chromium
Copy link

Request for Mozilla Position on an Emerging Web Specification

This is a proposed iteration on the existing IntersectionObserver spec.

The explainer doc explains the motivation for this change, but in a nutshell: it's intended to be a strong weapon for combatting click-jacking and other UI redress attacks.

Early signals from web developers have been very positive.

The feature is currently implemented in chromium, behind a flag:

--enable-blink-features=IntersectionObserverV2

Sample code here.

@emilio
Copy link
Collaborator

emilio commented Nov 10, 2018

cc @mstange, who reviewed most of the Gecko IntersectionObserver code.

@mikewest
Copy link

Mozillians who can provide input (optional)

@dveditz and @ckerschb have (positive, I think) opinions about this from a security perspective.

@dbaron
Copy link
Contributor

dbaron commented Nov 29, 2018

My initial reaction is that it seems like a reasonable thing to want, but I'm concerned about three things:

  • the use cases for v2 are very different from the use cases for v1 (ad visibility), and I'm a little concerned the different sets of use cases could cause tension in the future. Probably not a big deal, though.
  • I'm a little concerned about what happens in the future as we add features that could count as modifying or occluding content. Are we always going to remember to add them to what matters for this API? (Is it setting up good mechanisms for maintaining that list as, e.g., future CSS specs add such mechanisms.)
  • I'm quite a bit concerned about browser differences (that is, where the spec allows false negatives). For example, if commerce sites start requiring isVisible to be true, and the sites do something that causes it to become false in Firefox but not in Chrome (say, by putting a blur filter on a nearby element the edges of which may occlude the element depending on the blurring algorithm used), then those sites would be broken in Firefox -- and then implicitly or even explicitly encourage their users to switch to Chrome. (This is a pretty realistic example because Chrome has a bunch of real interop bugs with blurs where it's producing results that are nonconformant to the spec as a result of covering a smaller area than other browsers, such as bug 179006 (although maybe bug 624175 fixed some of it?). I haven't looked into the interop situation for a few years, but it used to be quite bad with Chrome as the largest outlier, at least.)

@dbaron dbaron added the venue: W3C Specifications in W3C Working Groups label Nov 30, 2018
@szager-chromium
Copy link
Author

@dbaron I'll address just the third of your three concerns, as it is the most important one, and also the most difficult.

I very much doubt that IOv2 will serve as a one-stop visibility detection mechanism right from the get-go. I think it's very likely that there will be some percentage of well-intentioned web sites that inadvertently get on the wrong side of IOv2. I have no idea what that percentage will be; hopefully it will be on the small side. I expect that some amount of outreach will be required to get site authors to fix their sites and avoid anti-patterns that may trigger IOv2. I would expect that site authors would eventually come to understand that they need to be somewhat conservative and avoid pushing the envelope with crazy CSS in the vicinity of iframes, to avoid doing things that run the risk of being on the wrong side of IOv2 in any browser.

If all of that plays out, and the the great bulk of good-citizen sites do the right thing, then I would also expect that the behavior of different browsers would converge, and really only differ on the fringes -- which is precisely the territory that site authors will be incentivized to avoid.

This is, of course, purely speculative.

@dbaron
Copy link
Contributor

dbaron commented Nov 30, 2018

I think that also depends on other browsers implementing at the same time as Chromium, rather than later.

@bzbarsky
Copy link
Contributor

bzbarsky commented Nov 30, 2018

@szager-chromium Unfortunately, we have observed a history of sites not being good-citizens in the way you describe. Including Google sites, sadly... So I'm personally very wary about whether it would work the way you describe.

@szager-chromium
Copy link
Author

@bzbarsky @dbaron Do you have any thoughts on how to avoid the potential interop trap?

At the time I wrote the language about permitting false negatives, my plan to implement this was to do a walk of the graphics layers above the target and mask them out; and then do a walk of objects that are painted after the target object in the same graphics layer, and mask them out. That approach would probably be faster than doing a hit test, but with a (maybe significantly) higher rate of false negatives.

However, it appears now that doing a hit test will be performant enough; and the performance impact will anyway be mitigated by throttling the frequency of computation (via the 'delay' parameter) to no more than once per 100ms. By using a hit test, we avoid all known possibilities of false negatives.

So... maybe we can just drop the language about permitting false negatives? Would that allay your concerns?

@dbaron
Copy link
Contributor

dbaron commented Nov 30, 2018

Moving in the direction of defining the behavior is definitely an improvement. We may well need to get hit testing better defined, though.

That said, I think there are some cases where things are quite visible (and can cover things up) but don't actually hit test, such as shadows. So then it comes down to the question of which sort of occlusion you care about -- visual occlusion or hit testing occlusion?

@szager-chromium
Copy link
Author

@dbaron In the course of implementing IOv2 in chromium, we added a flag to the hit testing code which allows callers to specify using visual rects rather than layout rects when testing for occlusion.

@dbaron
Copy link
Contributor

dbaron commented Nov 30, 2018

That sounds like it hasn't improved anything, then, since those visual/ink rects are likely to include blurs, shadows, etc., in non-interoperable ways. (Visual/ink overflow rects are totally undefined by specifications, whereas there's at least a start on defining scrollable overflow.)

@szager-chromium
Copy link
Author

@dbaron I looked into the box-shadow bug you mentioned. Although chromium displays a tighter radius of shaded pixels, it actually sets the visual overflow rect to extend 45px in each direction beyond the object's box boundary, which is more or less in line with the "color transition approximately twice the length of the blur radius" in the spec language:

https://www.w3.org/TR/css-backgrounds-3/#shadow-blur

On the chromium side, I think it would make sense to fix the code so that the visual overflow rect is based on a color transition that is exactly twice the blur radius, to make all of this predictable and (hopefully) interoperable. We are willing to fix issues like this to make the behavior of IOv2 interoperable.

Are there other specific issues around visual overflow where you think interoperability could be a problem? I would like to get a sense of whether this is more of a practical or a theoretical concern. As many people have noted, the usefulness of IOv2 (or something like it) for combatting fraud is potentially very high, so we are committed to figuring out how to make it work for all browsers.

@szager-chromium
Copy link
Author

@dbaron -- any thoughts on my previous comment?

@szager-chromium
Copy link
Author

@dbaron -- brief update: the discussion of this issue has spurred us to dig into chromium's issue with blur radius, and we have figured out the root cause of our different behavior. With our fix, blurs in chromium look the same as those in Firefox.

As for how this affects IntersectionObserver V2, I would suggest that the spec language around blur radius should be updated. It defines blur radius as the area of "visibly apparent color transition". That's a squishy definition -- "visibly apparent" is subjective, and in practice it seems to be always smaller than the total area of pixels affected by the blur. The spec should -- instead or in addition -- specify the maximum area which may be affected by the blur. The IntersectionObserverV2 can then refer to the blur radius spec when defining occlusion.

Does that sound reasonable to you? Are there other issues around visual rects that might cause interop issues?

@dbaron
Copy link
Contributor

dbaron commented Dec 19, 2018

I filed bug 1514950 to add a web-platform-test for the details of blurring in <canvas> 2D context, which is where the details are most easily testable.

Regarding updating the spec definition so that there's a limit: I think there are some good reasons why the specs define blur the way they do; see my blog post from 2011 for details. I think standardizing on a clear definition of where the edge of the effect is probably requires standardizing on the algorithm (probably triple box blur), and I'm not sure whether this is something all engines would be OK with.

@szager-chromium
Copy link
Author

@dbaron FWIW, it appears that firefox and chromium use the same math to compute the extent of the blur effect:

https://hg.mozilla.org/mozilla-central/file/795d02901ad0/gfx/2d/Blur.cpp#l879
https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkMaskBlurFilter.cpp?rcl=191e64b6c6c2e791ff4d7dae0209a78eed0376f0&l=27

It's a separate issue whether this is properly spec-able, but it does appear to be de facto interoperable (assuming we fix our bug).

@szager-chromium
Copy link
Author

szager-chromium commented Jan 9, 2019

I went ahead and added language to spec proposal along the lines of my previous comment:

https://szager-chromium.github.io/IntersectionObserver/#calculate-visibility-algo

@dbaron
Copy link
Contributor

dbaron commented Jan 24, 2019

@szager-chromium
Copy link
Author

Oh yes, thank you, that's the right link.

@dbaron
Copy link
Contributor

dbaron commented Jan 24, 2019

Also worth noting here that there's now an Intent to Ship for this on blink-dev.

@szager-chromium
Copy link
Author

FYI, this is scheduled to ship in Chrome 73, which is currently Chrome dev channel.

I hope I've addressed @dbaron's concerns about blur filter. I'm preparing a PR for web-platform-tests that exercises blur filters and other interesting cases. If you still have concerns, or would like changes in the spec, the sooner the better.

@dbaron
Copy link
Contributor

dbaron commented Mar 5, 2019

So I think the issue with blur was an example to help demonstrate the concern, but it wasn't actually the concern. The concern is that:

  1. if this isn't interoperably implemented, it will likely lead to content being blocked in minority-share browsers
  2. in order for it to be interoperably implemented, the things that it relies on need to be clearly specified and interoperably implemented

The issue with blur was an example of point 2, but I think the point is actually much broader than that, since we don't currently have a specification that defines hit testing, a specification that defines visual overflow, etc.

@szager-chromium
Copy link
Author

@dbaron I would like to leave hit testing out of this conversation, if possible. Chromium uses hit testing to implement this feature, but it's not required; it could be implemented differently.

As for specifiying visual overflow, I think it would make sense to flesh out the spec for ink overflow in a couple of ways:

  • The current spec language does not give a comprehensive list of all features that can contribute to ink overflow. We should make it a comprehensive list.

  • For features where the extent of the ink overflow seems pretty straightforward to define (e.g. non-blurred box shadow), we should define it.

That will still likely leave some cases that are difficult or impossible to define interoberably (e.g. overhanging glyphs). I'm not sure what can be done with those cases, but we could at least identify them and try to reason about how they could effect interoperability of IntersectionObserver V2.

What do you think? Am I missing anything?

@dbaron
Copy link
Contributor

dbaron commented Mar 29, 2019

I think that's a reasonable approach, assuming there's willingness from all browsers to converge on a common behavior. It's important to get the concepts underlying the definition defined quite clearly.

@zcorpan
Copy link
Member

zcorpan commented Mar 7, 2024

Occlusion detection is useful for Page Embedded Permission Controls.

See also discussion in the spec issue, in particular w3c/IntersectionObserver#295 (comment)

@emilio
Copy link
Collaborator

emilio commented May 14, 2024

FWIW, I recently reached out to relevant folks internally. I think at this point, thanks to all the work @szager-chromium has done in testing and getting ink overflow details spec'd out, I don't think there are any remaining concerns here, and otherwise the functionality seems positive and a good addition to the platform.

@dveditz left already some feedback in the spec pr (w3c/IntersectionObserver#523).

@zcorpan should we close as positive? do you think this needs a dashboard entry? If so happy to put up a PR or what not.

@zcorpan
Copy link
Member

zcorpan commented May 14, 2024

@emilio yes, and a dashboard entry seems good here. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
position: positive ready to add Appears ready to add to the table of positions. venue: W3C Specifications in W3C Working Groups
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants