Allow 0 (without unit) as a valid offset for rootMargin

w3c / IntersectionObserver

Intersection Observer

https://www.w3.org/TR/intersection-observer/

Other

3.62k stars 526 forks source link

Allow 0 (without unit) as a valid offset for rootMargin #453

Closed lebnic closed 3 years ago

lebnic commented 3 years ago

Closes #244

The following tasks have been completed:

[ ] Modified Web platform tests (link to pull request)

Implementation commitment:

[ ] WebKit (https://bugs.webkit.org/show_bug.cgi?id=)
[ ] Chromium (https://bugs.chromium.org/p/chromium/issues/detail?id=)
[ ] Gecko (https://bugzilla.mozilla.org/show_bug.cgi?id=)

lebnic commented 3 years ago

FYI @philipwalton

dontcallmedom commented 3 years ago

Marked as non substantive for IPR from ash-nazg.

lebnic commented 3 years ago

This only fixes the polyfill, not the specification, right? What's the point of that?

Documentation states that [rootMargin] Can have values similar to the CSS margin property (ref: https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API)

Thus, one would expect, for exemple, a value of rootMargin: "200px 0 0" to be valid. However, current implementation will throw an error, because only 0px or 0% are valid offsets, but not 0.

Hence this PR.

Moreover, there has been an open issue (https://github.com/w3c/IntersectionObserver/issues/244) since 2017 for this particular use-case, so I thought that maybe my proposed changes would be beneficial to others. If not, I guess we could simply write rootMargin: "200px 0px 0px" and move on with our lives. Anyways, thanks for the review @emilio , and let me know if there is anything else I can clarify

emilio commented 3 years ago

Well, my point is that:

Documentation, right now, is wrong. https://w3c.github.io/IntersectionObserver/#parse-a-root-margin disallows 0. It also disallows calc() / min() / max() / viewport units and other relative units that you can use in the margin shorthand (some of them for very good reason, it's not clear what em would do here for example!).
You're changing a polyfill that doesn't have anything to do with any browser implementation. If you want browsers to change, first you need to change the spec (which is in this repo, and is in the index.bs file), then you need to change browsers (filing bugs in the relevant bug trackers).

So changing the polyfill without changing the specification and at the very least filing browser bugs and adding tests to wpt seems a bit odd / not useful.

lebnic commented 3 years ago

Well, my point is that:

Documentation, right now, is wrong. https://w3c.github.io/IntersectionObserver/#parse-a-root-margin disallows 0. It also disallows calc() / min() / max() / viewport units and other relative units that you can use in the margin shorthand (some of them for very good reason, it's not clear what em would do here for example!).

You're changing a polyfill that doesn't have anything to do with any browser implementation. If you want browsers to change, first you need to change the spec (which is in this repo, and is in the index.bs file), then you need to change browsers (filing bugs in the relevant bug trackers).

So changing the polyfill without changing the specification and at the very least filing browser bugs and adding tests to wpt seems a bit odd / not useful.

You are 100% right, my bad.