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

Define content clip #386

Merged
merged 3 commits into from
Apr 1, 2022
Merged

Define content clip #386

merged 3 commits into from
Apr 1, 2022

Conversation

szager-chromium
Copy link
Collaborator

@szager-chromium szager-chromium commented Sep 18, 2019

Issue#351


Preview | Diff


Preview | Diff

Copy link
Collaborator

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should probably use https://drafts.csswg.org/css-overflow/#scroll-container instead.

There are elements for which the overflow-x/y styles don't necessarily create a scroll container (e.g., an <img> element, or an element with display: none).

index.bs Outdated
@@ -304,6 +308,8 @@ or else the <a>top-level browsing context</a>'s <a>document</a> node
(referred to as the <dfn for="IntersectionObserver">implicit root</dfn>) if
the {{IntersectionObserver/root}} attribute is <code>null</code>.

An {{Element}} is defined as having an <dfn for="IntersectionObserver">overflow clip</dfn> if its computed style has a value other than "visible" for its <a>overflow</a>, <a>overflow-x</a>, or <a>overflow-y</a> property.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"overflow clip" seems quite an odd term because clip is a valid value of the overflow property, fwiw.

There's no need to mention the overflow property explicitly, as it's just a shorthand of overflow-x and overflow-y.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to "content clip".

index.bs Outdated
@@ -581,7 +587,7 @@ run these steps:
2. Let |container| be the <a>containing block</a> of the <a>target</a>.
3. While |container| is not the <a>intersection root</a>:
1. Map |intersectionRect| to the coordinate space of |container|.
2. If |container| has overflow clipping or a css <a>clip-path</a> property,
2. If |container| has an <a>overflow clip</a> or a CSS <A>clip-path</a> property,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The casing change at the left of clip-path seems unintentional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@szager-chromium
Copy link
Collaborator Author

I think this should probably use https://drafts.csswg.org/css-overflow/#scroll-container instead.

There are elements for which the overflow-x/y styles don't necessarily create a scroll container (e.g., an <img> element, or an element with display: none).

"Scroll container" is not quite right, because it is defined as an element that can be scrolled by the user, whereas IntersectionObserver should be applying the clip regardless of whether the element is user scrollable. The first part of that spec section refers to CSS properties that "specify whether a box’s content (including any ink overflow) is clipped to its padding edge". I've updated the language to specify that IntersectionObserver will apply any padding edge clip mandated by that spec section.

@szager-chromium szager-chromium changed the title Define overflow clip Define content clip Sep 2, 2020
@emilio
Copy link
Collaborator

emilio commented Sep 2, 2020

it is defined as an element that can be scrolled by the user

Hmm, how so? <div style="overflow: hidden"></div> is not scrollable by the user but is a scroll container, right? I think the definition there is a bit poor but https://drafts.csswg.org/css-overflow/#valdef-overflow-hidden makes it clear...

@szager-chromium
Copy link
Collaborator Author

it is defined as an element that can be scrolled by the user

Hmm, how so? <div style="overflow: hidden"></div> is not scrollable by the user but is a scroll container, right? I think the definition there is a bit poor but https://drafts.csswg.org/css-overflow/#valdef-overflow-hidden makes it clear...

Here's the first line in that section of the spec:

"These properties specify whether a box’s content (including any ink overflow) is clipped to its padding edge, and if so, whether it is a scroll container that allows the user to scroll clipped parts of its scrollable overflow area into view."

So, "scroll container" is a subset of elements that have their content clipped to the padding edge; specifically, it refers to those elements that are user-scrollable. For IntersectionObserver, we want to apply the padding edge clip to the superset, including overflow:hidden and overflow:clip.

@emilio
Copy link
Collaborator

emilio commented Oct 19, 2020

I think that's a bug in the css spec fwiw. <div style="overflow: hidden"> totally is a scroll container. But I overflow: clip isn't so... I guess this is fine to account for both.

index.bs Show resolved Hide resolved
Base automatically changed from master to main February 3, 2021 04:25
@szager-chromium szager-chromium merged commit a07cd7d into main Apr 1, 2022
github-actions bot added a commit that referenced this pull request Apr 1, 2022
SHA: a07cd7d
Reason: push, by @szager-chromium

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants