Skip to content

Commit

Permalink
fix(components/viewport): consider elements as scrolled into view whe…
Browse files Browse the repository at this point in the history 
…n there is no viewport overflow

The calculation of whether an element is scrolled into view may be wrong by a few pixels if the viewport contains elements with decimal sizes. This can happen because `offsetLeft` and `offsetTop` operate on an integer (not a decimal), losing precision that can accumulate.

To avoid incorrect calculations, the viewport assumes that its elements are scrolled into view when there is no viewport overflow.
  • Loading branch information
@danielwiehl @Marcarrian
danielwiehl authored and Marcarrian committed Mar 28, 2023
1 parent 900fe45 commit 22baab7
Show file tree
Hide file tree
Showing 2 changed files with 238 additions and 18 deletions.
227 changes: 220 additions & 7 deletions projects/scion/components/viewport/src/viewport.component.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
*/

import {ComponentFixture, TestBed} from '@angular/core/testing';
import {Component, ElementRef, Input, Renderer2, ViewChild} from '@angular/core';
import {Component, ElementRef, HostBinding, Input, Renderer2, ViewChild} from '@angular/core';
import {SciViewportModule} from './viewport.module';
import {By} from '@angular/platform-browser';
import {Dictionary} from '@scion/toolkit/util';
Expand All @@ -27,6 +27,7 @@ describe('Viewport', () => {
Testee1Component,
Testee2Component,
Testee3Component,
ElementDecimalSizeTestComponent,
],
imports: [
SciViewportModule,
Expand Down Expand Up @@ -1046,8 +1047,8 @@ describe('Viewport', () => {

// when element overlaps on top
component.moveElement(component.insideViewportElement, {x: 0, y: -50});
// then expect element to be partially in viewport
expect(component.viewportComponent.isElementInView(component.insideViewportElement, 'full')).toBeFalse();
// then expect element to be in the viewport (native viewport only overflows on the right or bottom)
expect(component.viewportComponent.isElementInView(component.insideViewportElement, 'full')).toBeTrue();
expect(component.viewportComponent.isElementInView(component.insideViewportElement, 'partial')).toBeTrue();

// when element overlaps on top right
Expand Down Expand Up @@ -1082,14 +1083,14 @@ describe('Viewport', () => {

// when element overlaps on left
component.moveElement(component.insideViewportElement, {x: -50, y: 0});
// then expect element to be partially in viewport
expect(component.viewportComponent.isElementInView(component.insideViewportElement, 'full')).toBeFalse();
// then expect element to be in the viewport (native viewport only overflows on the right or bottom)
expect(component.viewportComponent.isElementInView(component.insideViewportElement, 'full')).toBeTrue();
expect(component.viewportComponent.isElementInView(component.insideViewportElement, 'partial')).toBeTrue();

// when element overlaps on top left
component.moveElement(component.insideViewportElement, {x: -50, y: -50});
// then expect element to be partially in viewport
expect(component.viewportComponent.isElementInView(component.insideViewportElement, 'full')).toBeFalse();
// then expect element to be in the viewport (native viewport only overflows on the right or bottom)
expect(component.viewportComponent.isElementInView(component.insideViewportElement, 'full')).toBeTrue();
expect(component.viewportComponent.isElementInView(component.insideViewportElement, 'partial')).toBeTrue();

// when element is outside viewport
Expand All @@ -1098,6 +1099,122 @@ describe('Viewport', () => {
expect(component.viewportComponent.isElementInView(component.insideViewportElement, 'full')).toBeFalse();
expect(component.viewportComponent.isElementInView(component.insideViewportElement, 'partial')).toBeFalse();
});

describe('viewport contains elements with a decimal element width', () => {

it('should report all elements to be in the viewport if the viewport does not overflow (1/2)', async () => {
const fixture = TestBed.createComponent(ElementDecimalSizeTestComponent);
fixture.autoDetectChanges(true);
const component = fixture.componentInstance;
await flushChanges(fixture);

component.setElement2Width(100.5);

// Set width of element-1 to 100.4 pixel
component.setElement1Width(100.4);
expect(component.viewportComponent.isElementInView(component.element1, 'full')).withContext('isElementInView(element-1) (A)').toBeTrue();
expect(component.viewportComponent.isElementInView(component.element2, 'full')).withContext('isElementInView(element-2) (A)').toBeTrue();
expect(isScrollbarVisible(fixture, 'horizontal')).withContext('scrollbar (A)').toBeFalse();

// Set width of element-1 to 100.5 pixel
component.setElement1Width(100.5);
expect(component.viewportComponent.isElementInView(component.element1, 'full')).withContext('isElementInView(element-1) (B)').toBeTrue();
expect(component.viewportComponent.isElementInView(component.element2, 'full')).withContext('isElementInView(element-2) (B)').toBeTrue();
expect(isScrollbarVisible(fixture, 'horizontal')).withContext('scrollbar (B)').toBeFalse();

// Set width of element-1 to 100.6 pixel
component.setElement1Width(100.6);
expect(component.viewportComponent.isElementInView(component.element1, 'full')).withContext('isElementInView(element-1) (C)').toBeTrue();
expect(component.viewportComponent.isElementInView(component.element2, 'full')).withContext('isElementInView(element-2) (C)').toBeTrue();
expect(isScrollbarVisible(fixture, 'horizontal')).withContext('scrollbar (C)').toBeFalse();
});

it('should report all elements to be in the viewport if the viewport does not overflow (2/2)', async () => {
const fixture = TestBed.createComponent(ElementDecimalSizeTestComponent);
fixture.autoDetectChanges(true);
const component = fixture.componentInstance;
await flushChanges(fixture);

component.setElement1Width(100.5);

// Set width of element-2 to 100.4 pixel
component.setElement2Width(100.4);
expect(component.viewportComponent.isElementInView(component.element1, 'full')).withContext('isElementInView(element-1) (A)').toBeTrue();
expect(component.viewportComponent.isElementInView(component.element2, 'full')).withContext('isElementInView(element-2) (A)').toBeTrue();
expect(isScrollbarVisible(fixture, 'horizontal')).withContext('scrollbar (A)').toBeFalse();

// Set width of element-2 to 100.5 pixel
component.setElement2Width(100.5);
expect(component.viewportComponent.isElementInView(component.element1, 'full')).withContext('isElementInView(element-1) (B)').toBeTrue();
expect(component.viewportComponent.isElementInView(component.element2, 'full')).withContext('isElementInView(element-2) (B)').toBeTrue();
expect(isScrollbarVisible(fixture, 'horizontal')).withContext('scrollbar (B)').toBeFalse();

// Set width of element-2 to 100.6 pixel
component.setElement2Width(100.6);
expect(component.viewportComponent.isElementInView(component.element1, 'full')).withContext('isElementInView(element-1) (C)').toBeTrue();
expect(component.viewportComponent.isElementInView(component.element2, 'full')).withContext('isElementInView(element-2) (C)').toBeTrue();
expect(isScrollbarVisible(fixture, 'horizontal')).withContext('scrollbar (C)').toBeFalse();
});
});

describe('viewport contains elements with a decimal element height', () => {

it('should report all elements to be in the viewport if the viewport does not overflow (1/2)', async () => {
const fixture = TestBed.createComponent(ElementDecimalSizeTestComponent);
fixture.autoDetectChanges(true);
fixture.componentRef.setInput('columnLayout', true);
const component = fixture.componentInstance;
await flushChanges(fixture);

component.setElement2Height(100.5);

// Set height of element-1 to 100.4 pixel
component.setElement1Height(100.4);
expect(component.viewportComponent.isElementInView(component.element1, 'full')).withContext('isElementInView(element-1) (A)').toBeTrue();
expect(component.viewportComponent.isElementInView(component.element2, 'full')).withContext('isElementInView(element-2) (A)').toBeTrue();
expect(isScrollbarVisible(fixture, 'horizontal')).withContext('scrollbar (A)').toBeFalse();

// Set height of element-1 to 100.5 pixel
component.setElement1Height(100.5);
expect(component.viewportComponent.isElementInView(component.element1, 'full')).withContext('isElementInView(element-1) (B)').toBeTrue();
expect(component.viewportComponent.isElementInView(component.element2, 'full')).withContext('isElementInView(element-2) (B)').toBeTrue();
expect(isScrollbarVisible(fixture, 'horizontal')).withContext('scrollbar (B)').toBeFalse();

// Set height of element-1 to 100.6 pixel
component.setElement1Height(100.6);
expect(component.viewportComponent.isElementInView(component.element1, 'full')).withContext('isElementInView(element-1) (C)').toBeTrue();
expect(component.viewportComponent.isElementInView(component.element2, 'full')).withContext('isElementInView(element-2) (C)').toBeTrue();
expect(isScrollbarVisible(fixture, 'horizontal')).withContext('scrollbar (C)').toBeFalse();
});

it('should report all elements to be in the viewport if the viewport does not overflow (2/2)', async () => {
const fixture = TestBed.createComponent(ElementDecimalSizeTestComponent);
fixture.autoDetectChanges(true);
fixture.componentRef.setInput('columnLayout', true);
const component = fixture.componentInstance;
await flushChanges(fixture);

component.setElement1Height(100.5);

// Set height of element-2 to 100.4 pixel
component.setElement2Height(100.4);
expect(component.viewportComponent.isElementInView(component.element1, 'full')).withContext('isElementInView(element-1) (A)').toBeTrue();
expect(component.viewportComponent.isElementInView(component.element2, 'full')).withContext('isElementInView(element-2) (A)').toBeTrue();
expect(isScrollbarVisible(fixture, 'horizontal')).withContext('scrollbar (A)').toBeFalse();

// Set height of element-2 to 100.5 pixel
component.setElement2Height(100.5);
expect(component.viewportComponent.isElementInView(component.element1, 'full')).withContext('isElementInView(element-1) (B)').toBeTrue();
expect(component.viewportComponent.isElementInView(component.element2, 'full')).withContext('isElementInView(element-2) (B)').toBeTrue();
expect(isScrollbarVisible(fixture, 'horizontal')).withContext('scrollbar (B)').toBeFalse();

// Set height of element-2 to 100.6 pixel
component.setElement2Height(100.6);
expect(component.viewportComponent.isElementInView(component.element1, 'full')).withContext('isElementInView(element-1) (C)').toBeTrue();
expect(component.viewportComponent.isElementInView(component.element2, 'full')).withContext('isElementInView(element-2) (C)').toBeTrue();
expect(isScrollbarVisible(fixture, 'horizontal')).withContext('scrollbar (C)').toBeFalse();
});
});
});

describe('scrollIntoView', () => {
Expand Down Expand Up @@ -1351,3 +1468,99 @@ class Testee3Component {
Object.keys(style).forEach(key => this._renderer.setStyle(element.nativeElement, key, style[key]));
}
}

/**
* Renders a viewport that contains two elements, either aligned in a row or a column,
* controlled via 'columnLayout' input property. The viewport is sized according to its
* content width and height.
*/
@Component({
selector: 'spec-decimal-element-size',
template: `
<sci-viewport>
<div class="element" #element1></div>
<div class="element" #element2></div>
</sci-viewport>
`,
styles: [`
:host {
display: flex;
height: 100px;
}
:host.column-layout {
flex-direction: column;
height: unset;
width: 100px;
}
sci-viewport {
flex: initial;
background-color: grey;
}
div.element {
width: 100px;
height: 100px;
}
sci-viewport::part(content) {
display: flex;
background-color: cornflowerblue;
}
:host.column-layout sci-viewport::part(content) {
flex-direction: column;
}
`],
})
class ElementDecimalSizeTestComponent {

@Input()
@HostBinding('class.column-layout')
public columnLayout = false;

@ViewChild(SciViewportComponent, {static: true})
public viewportComponent!: SciViewportComponent;

@ViewChild('element1', {read: ElementRef, static: true})
public element1!: ElementRef<HTMLElement>;

@ViewChild('element2', {read: ElementRef, static: true})
public element2!: ElementRef<HTMLElement>;

constructor(private _renderer: Renderer2) {
}

public setElement1Width(px: number): void {
this.setStyle(this.element1, {
'font-size': '1px',
'width': `${px}em`, // set width as `em` to bypass CSS limitation to set decimal pixel values
});
}

public setElement2Width(px: number): void {
this.setStyle(this.element2, {
'font-size': '1px',
'width': `${px}em`, // set width as `em` to bypass CSS limitation to set decimal pixel values
});
}

public setElement1Height(px: number): void {
this.setStyle(this.element1, {
'font-size': '1px',
'height': `${px}em`, // set height as `em` to bypass CSS limitation to set decimal pixel values
});
}

public setElement2Height(px: number): void {
this.setStyle(this.element2, {
'font-size': '1px',
'height': `${px}em`, // set height as `em` to bypass CSS limitation to set decimal pixel values
});
}

public setStyle(element: ElementRef<HTMLElement>, style: Dictionary): void {
Object.keys(style).forEach(key => this._renderer.setStyle(element.nativeElement, key, style[key]));
}
}
29 changes: 18 additions & 11 deletions projects/scion/components/viewport/src/viewport.component.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,23 +212,30 @@ export class SciViewportComponent {
* @throws if the element is not contained in the viewport's slotted content.
*/
public isElementInView(element: ElementRef<HTMLElement> | HTMLElement, fit: 'full' | 'partial'): boolean {
const elLeft = this.computeOffset(element, 'left');
const elRight = elLeft + coerceElement(element).offsetWidth;
const elTop = this.computeOffset(element, 'top');
const elLeft = this.computeOffset(element, 'left');

// Consider elements as scrolled into view when there is no viewport overflow.
// The calculation of whether an element is scrolled into view may be wrong by a few pixels if the viewport contains elements with decimal sizes.
// This can happen because `offsetLeft` and `offsetTop` operate on an integer (not a decimal), losing precision that can accumulate.
// To avoid incorrect calculation when there is no viewport overflow, we consider all contained elements as scrolled into the view.
if (this._viewportElement.scrollWidth <= this._viewportElement.clientWidth && this._viewportElement.scrollHeight <= this._viewportElement.clientHeight) {
return true;
}

const elBottom = elTop + coerceElement(element).offsetHeight;
const elRight = elLeft + coerceElement(element).offsetWidth;

const vpLeft = this._viewportElement.scrollLeft;
const vpRight = vpLeft + this._viewportElement.clientWidth;
const vpTop = this._viewportElement.scrollTop;
const vpLeft = this._viewportElement.scrollLeft;
const vpBottom = vpTop + this._viewportElement.clientHeight;
const vpRight = vpLeft + this._viewportElement.clientWidth;

switch (fit) {
case 'full':
return (elLeft >= vpLeft && elRight <= vpRight) && (elTop >= vpTop && elBottom <= vpBottom);
case 'partial':
return (elRight >= vpLeft && elLeft <= vpRight) && (elBottom >= vpTop && elTop <= vpBottom);
default:
throw Error('Unsupported fit strategy');
if (fit === 'full') {
return (elTop >= vpTop && elBottom <= vpBottom) && (elLeft >= vpLeft && elRight <= vpRight);
}
else {
return (elBottom >= vpTop && elTop <= vpBottom) && (elRight >= vpLeft && elLeft <= vpRight);
}
}

Expand Down

0 comments on commit 22baab7

Please sign in to comment.