From 22baab78c4bdf34caeb99c750079cd415aca046b Mon Sep 17 00:00:00 2001 From: danielwiehl Date: Mon, 27 Mar 2023 16:19:24 +0200 Subject: [PATCH] fix(components/viewport): 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 calculations, the viewport assumes that its elements are scrolled into view when there is no viewport overflow. --- .../viewport/src/viewport.component.spec.ts | 227 +++++++++++++++++- .../viewport/src/viewport.component.ts | 29 ++- 2 files changed, 238 insertions(+), 18 deletions(-) diff --git a/projects/scion/components/viewport/src/viewport.component.spec.ts b/projects/scion/components/viewport/src/viewport.component.spec.ts index d81a6528..ae88f717 100644 --- a/projects/scion/components/viewport/src/viewport.component.spec.ts +++ b/projects/scion/components/viewport/src/viewport.component.spec.ts @@ -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'; @@ -27,6 +27,7 @@ describe('Viewport', () => { Testee1Component, Testee2Component, Testee3Component, + ElementDecimalSizeTestComponent, ], imports: [ SciViewportModule, @@ -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 @@ -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 @@ -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', () => { @@ -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: ` + +
+
+ + `, + 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; + + @ViewChild('element2', {read: ElementRef, static: true}) + public element2!: ElementRef; + + 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, style: Dictionary): void { + Object.keys(style).forEach(key => this._renderer.setStyle(element.nativeElement, key, style[key])); + } +} diff --git a/projects/scion/components/viewport/src/viewport.component.ts b/projects/scion/components/viewport/src/viewport.component.ts index 4a4f4618..9531b6bc 100644 --- a/projects/scion/components/viewport/src/viewport.component.ts +++ b/projects/scion/components/viewport/src/viewport.component.ts @@ -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, 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); } }