[iOS][actor-login] Update `CheckViewAreaVisible` to match Blink [chromium/src : main]
Ginny Huang (Gerrit)
Ginny Huang voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vincent Boisselle (Gerrit)
Vincent Boisselle added 19 comments![]( "Open in Gerrit")
The commit message refers to this as `getContainingBlock`, but the function is named `getContainingBlockForElement`. Consider updating the commit message or function name for consistency.
// Handles cases when the nearest containing block might be further up in thePotential TODO
idk how we want to deal with shadow roots, they are actually ignored by the model afaik so might not be worth considering them at this point but maybe later on once we incorporate shadow dom into the annotated page content
Note: If `element` is inside a Shadow Root (Web Component), `element.parentElement` is `null` (since its parent is a `ShadowRoot`, which is not an `Element`). This will prevent `getContainingBlockForElement` from traversing past shadow boundaries to check clipping on the host element or host ancestors. Consider using `(element.assignedSlot || element.parentElement || (element.getRootNode() as ShadowRoot)?.host) as Element | null` if form elements inside Shadow DOM need to be supported.
const containerStyle = window.getComputedStyle(container);Put the logic to determine whether the container is eligible in a function that returns a boolean which will control the loop, makes the flow easier to read
if (position === 'absolute' && containerStyle.position !== 'static') {this condition is true in 99% of the cases with a small exception
"For normal flow elements ( position other than fixed and absolute ), the containing block is the parent element (unless the parent does not generate a box, e.g. display: contents )."
if (position === 'absolute' && containerStyle.position !== 'static') {Comment out the reason why these conditions make the container eligible
if (containerStyle.transform !== 'none' ||Comment out the reason why these conditions make the container eligible
const contain = containerStyle.contain;Avoid using `(containerStyle as any).webkitBackdropFilter`. You can use `containerStyle.getPropertyValue(-webkit-backdrop-filter)` to access `-webkit-backdrop-filter` in a type-safe manner without casting to `any`.
if (contain &&Comment out the reason why these conditions make the container eligible
const willChange = containerStyle.willChange;Comment out the reason why these conditions make the container eligible
willChange.includes('perspective') || willChange.includes('filter') ||Per CSS Containment 3, `container-type: inline-size` or `size` (CSS Container Queries) also establishes layout containment and creates a containing block for `position: absolute` descendants. Consider adding `containerStyle.containerType !== none`.
Note: Ancestors with CSS `clip-path` or `mask` clip all descendants (including `position: absolute` or `fixed` elements) regardless of whether the ancestor is a containing block. Consider checking if `containerStyle.clipPath !== none` or `containerStyle.mask !== none` / `webkitMask !== none` in the clipping check.
if (bottom <= top || right <= left) {comment out what this check does
`clippingElement.getBoundingClientRect()` returns the border-box (including border and scrollbar width). However, CSS `overflow` clips content at the padding-box boundary. If an overflow container has a border (e.g., `border: 10px solid black`), content in the border area will not be properly clipped.
Consider using `clientTop`, `clientLeft`, `clientWidth`, and `clientHeight` to compute the padding-box:
```typescript
const clipLeft = clipBox.left + clippingElement.clientLeft;
const clipTop = clipBox.top + clippingElement.clientTop;
const clipRight = clipLeft + clippingElement.clientWidth;
const clipBottom = clipTop + clippingElement.clientHeight;
```
function scrollAndCheckViewAreaVisible(fieldIdentifier: number): boolean {Consider adding a visual occlusion check (e.g., for fixed headers, cookie banners, or modal overlays covering the field) using `document.elementFromPoint`:
Once the clipped `visibleRect` is calculated, you can sample its center point `(centerX, centerY)` to verify that the top-most element at that point is the target element (or its descendant / associated `<label>`):
```typescript
const centerX = rect.left + rect.width / 2;
const centerY = rect.top + rect.height / 2;
const hitElement = document.elementFromPoint(centerX, centerY);
const isNotOccluded = hitElement && (element.contains(hitElement) || (hitElement instanceof HTMLLabelElement && hitElement.control === element));
```
This provides a lightweight occlusion check for single-element visibility without needing complex z-order tree computations.
let block = getContainingBlockForElement(element);could rename "container"
let rect: DOMRect|null = element.getBoundingClientRect();elementRect
while (block && rect) {Explain what this while loop does, it basically clips the element with all the eligible containers
it can clip as long as there is rect and a block (container) to clip on
}Need a TODO somehwere about testing as we really want to test this :)
Testing / Coverage: Consider adding unit test or integration test coverage for `scrollAndCheckViewAreaVisible` to prevent regressions and verify containing-block clipping (e.g., testing `position: absolute` elements, elements inside `overflow: hidden` containers, and elements outside the viewport).
Consider extracting a generic `clipRect(rect: DOMRect, clipBox: {left: number, top: number, right: number, bottom: number}): DOMRect | null` helper.
This would allow reusing the exact same rectangle intersection math for both `clipRectWithElement` and the final viewport check:
```typescript
function clipRect(
rect: DOMRect,
clipBox: {left: number, top: number, right: number, bottom: number}): DOMRect|null {
const left = Math.max(rect.left, clipBox.left);
const right = Math.min(rect.right, clipBox.right);
const top = Math.max(rect.top, clipBox.top);
const bottom = Math.min(rect.bottom, clipBox.bottom);
if (bottom <= top || right <= left) {
return null;
}
return new DOMRect(left, top, right - left, bottom - top);
}
```Then in `scrollAndCheckViewAreaVisible`:
```typescript
const viewportBox = {
left: 0,
top: 0,
right: window.innerWidth || document.documentElement.clientWidth,
bottom: window.innerHeight || document.documentElement.clientHeight,
};
return clipRect(rect, viewportBox) !== null;
```
Related details
- Ginny Huang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ginny Huang (Gerrit)
Ginny Huang voted and added 19 comments![]( "Open in Gerrit")
Votes added by Ginny Huang
| Auto-Submit | +1 |
| Commit-Queue | +1 |
19 comments
The commit message refers to this as `getContainingBlock`, but the function is named `getContainingBlockForElement`. Consider updating the commit message or function name for consistency.
Rewritten the function
// Handles cases when the nearest containing block might be further up in thePotential TODO
idk how we want to deal with shadow roots, they are actually ignored by the model afaik so might not be worth considering them at this point but maybe later on once we incorporate shadow dom into the annotated page content
Note: If `element` is inside a Shadow Root (Web Component), `element.parentElement` is `null` (since its parent is a `ShadowRoot`, which is not an `Element`). This will prevent `getContainingBlockForElement` from traversing past shadow boundaries to check clipping on the host element or host ancestors. Consider using `(element.assignedSlot || element.parentElement || (element.getRootNode() as ShadowRoot)?.host) as Element | null` if form elements inside Shadow DOM need to be supported.
Bug created.
const containerStyle = window.getComputedStyle(container);Put the logic to determine whether the container is eligible in a function that returns a boolean which will control the loop, makes the flow easier to read
Done
if (position === 'absolute' && containerStyle.position !== 'static') {this condition is true in 99% of the cases with a small exception
"For normal flow elements ( position other than fixed and absolute ), the containing block is the parent element (unless the parent does not generate a box, e.g. display: contents )."
Done
if (position === 'absolute' && containerStyle.position !== 'static') {Comment out the reason why these conditions make the container eligible
Done
Comment out the reason why these conditions make the container eligible
Done
Avoid using `(containerStyle as any).webkitBackdropFilter`. You can use `containerStyle.getPropertyValue(-webkit-backdrop-filter)` to access `-webkit-backdrop-filter` in a type-safe manner without casting to `any`.
Done
Comment out the reason why these conditions make the container eligible
Done
Comment out the reason why these conditions make the container eligible
Done
willChange.includes('perspective') || willChange.includes('filter') ||Per CSS Containment 3, `container-type: inline-size` or `size` (CSS Container Queries) also establishes layout containment and creates a containing block for `position: absolute` descendants. Consider adding `containerStyle.containerType !== none`.
It's not mentioned in https://developer.mozilla.org/en-US/docs/Web/CSS/Guides/Display/Containing_block and I personally tried and see that they work similarly as "none". Do you have a source?
Note: Ancestors with CSS `clip-path` or `mask` clip all descendants (including `position: absolute` or `fixed` elements) regardless of whether the ancestor is a containing block. Consider checking if `containerStyle.clipPath !== none` or `containerStyle.mask !== none` / `webkitMask !== none` in the clipping check.
Done
comment out what this check does
Done
`clippingElement.getBoundingClientRect()` returns the border-box (including border and scrollbar width). However, CSS `overflow` clips content at the padding-box boundary. If an overflow container has a border (e.g., `border: 10px solid black`), content in the border area will not be properly clipped.
Consider using `clientTop`, `clientLeft`, `clientWidth`, and `clientHeight` to compute the padding-box:
```typescript
const clipLeft = clipBox.left + clippingElement.clientLeft;
const clipTop = clipBox.top + clippingElement.clientTop;
const clipRight = clipLeft + clippingElement.clientWidth;
const clipBottom = clipTop + clippingElement.clientHeight;
```
Done
function scrollAndCheckViewAreaVisible(fieldIdentifier: number): boolean {Consider adding a visual occlusion check (e.g., for fixed headers, cookie banners, or modal overlays covering the field) using `document.elementFromPoint`:
Once the clipped `visibleRect` is calculated, you can sample its center point `(centerX, centerY)` to verify that the top-most element at that point is the target element (or its descendant / associated `<label>`):
```typescript
const centerX = rect.left + rect.width / 2;
const centerY = rect.top + rect.height / 2;
const hitElement = document.elementFromPoint(centerX, centerY);
const isNotOccluded = hitElement && (element.contains(hitElement) || (hitElement instanceof HTMLLabelElement && hitElement.control === element));
```
This provides a lightweight occlusion check for single-element visibility without needing complex z-order tree computations.
Done
let block = getContainingBlockForElement(element);Ginny Huangcould rename "container"
Refactored so this does not apply anymore
let rect: DOMRect|null = element.getBoundingClientRect();elementRect
I don't think `elementRect` is accurate, as it will be keep clipped to the intersection of element and its container ancestors.
Explain what this while loop does, it basically clips the element with all the eligible containers
it can clip as long as there is rect and a block (container) to clip on
Done
Need a TODO somehwere about testing as we really want to test this :)
Testing / Coverage: Consider adding unit test or integration test coverage for `scrollAndCheckViewAreaVisible` to prevent regressions and verify containing-block clipping (e.g., testing `position: absolute` elements, elements inside `overflow: hidden` containers, and elements outside the viewport).
Do we have tests for other JS methods in this file?
Consider extracting a generic `clipRect(rect: DOMRect, clipBox: {left: number, top: number, right: number, bottom: number}): DOMRect | null` helper.
This would allow reusing the exact same rectangle intersection math for both `clipRectWithElement` and the final viewport check:
```typescript
function clipRect(
rect: DOMRect,
clipBox: {left: number, top: number, right: number, bottom: number}): DOMRect|null {
const left = Math.max(rect.left, clipBox.left);
const right = Math.min(rect.right, clipBox.right);
const top = Math.max(rect.top, clipBox.top);
const bottom = Math.min(rect.bottom, clipBox.bottom);if (bottom <= top || right <= left) {
return null;
}
return new DOMRect(left, top, right - left, bottom - top);
}
```Then in `scrollAndCheckViewAreaVisible`:
```typescript
const viewportBox = {
left: 0,
top: 0,
right: window.innerWidth || document.documentElement.clientWidth,
bottom: window.innerHeight || document.documentElement.clientHeight,
};
return clipRect(rect, viewportBox) !== null;
```
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |