From 365512d7d4b353381c311b1bbc396f0d2cae0e2c Mon Sep 17 00:00:00 2001 From: Vladislav Bulyukhin Date: Thu, 22 Oct 2020 10:04:27 +0200 Subject: [PATCH] KCL-5612 New tests, rendering algorithm improvements --- CHANGELOG.md | 5 + src/lib/HighlightRenderer.ts | 64 +++++----- src/utils/node.ts | 45 +++++-- test-browser/utils/node.spec.ts | 204 ++++++++++++++++++++++++++++++-- 4 files changed, 272 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f1b007..950fac2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Fixed the positioning of the highlights inside body on pages with scrollbars by improving the rendering algorithm. +- Fixed the positioning of the highlights inside table elements by improving the rendering algorithm. + ## [1.2.0] - 2020-10-07 ### Added diff --git a/src/lib/HighlightRenderer.ts b/src/lib/HighlightRenderer.ts index 0dfa47b..dfeb526 100644 --- a/src/lib/HighlightRenderer.ts +++ b/src/lib/HighlightRenderer.ts @@ -1,4 +1,4 @@ -import { getParentForHighlight, getRelativeScrollOffset } from '../utils/node'; +import { getParentForHighlight, getTotalScrollOffset } from '../utils/node'; export const HighlighterContainerTag = 'KONTENT-SMART-LINK-OVERLAY'; export const HighlighterElementTag = 'KONTENT-SMART-LINK-ELEMENT'; @@ -57,6 +57,7 @@ export class HighlightRenderer implements IRenderer { const newHighlightByNode = new Map(); for (const node of nodes) { + // Get size of the node and its position relative to viewport. const nodeRect = node.getBoundingClientRect(); // This check is needed to prevent highlight rendering for the "flat" elements (height or/and width === 0), @@ -67,41 +68,41 @@ export class HighlightRenderer implements IRenderer { const [parentElement, parentMetadata] = getParentForHighlight(node); const highlight = this.highlightByNode.get(node) ?? this.createHighlight(parentElement); - if (parentElement) { + if (parentElement && parentMetadata) { const parentRect = parentElement.getBoundingClientRect(); const container = this.containerByParent.get(parentElement); - if (container) { - if (!parentMetadata?.hasRelativePosition) { - // When parent element is not relatively positioned it means that highlight - // will be positioned relatively to some other element. That is why we need - // to keep in mind all of the scroll offsets on the way to this relative element. - const [scrollOffsetTop, scrollOffsetLeft] = getRelativeScrollOffset(parentElement); - - container.style.height = `${parentRect.height}px`; - container.style.width = `${parentRect.width}px`; - container.style.top = `${parentElement.offsetTop - scrollOffsetTop}px`; - container.style.left = `${parentElement.offsetLeft - scrollOffsetLeft}px`; - - if (parentMetadata?.hasRestrictedOverflow) { - // When parent element has not relative position but its overflow is restricted - // we need to hide overflow of the container as well to prevent - // highlights from appearing for hidden content. - container.classList.add('kontent-smart-link-overlay--restricted'); - } + if (container && !parentMetadata.isPositioned) { + // When parent element is not positioned it means that highlight + // will be positioned relative to some other element. That is why we need + // to keep in mind all of the scroll offsets on the way to this relative element. + const [scrollOffsetTop, scrollOffsetLeft] = getTotalScrollOffset(parentElement); + + container.style.height = `${parentElement.clientHeight}px`; + container.style.width = `${parentElement.clientWidth}px`; + container.style.top = `${parentElement.offsetTop - scrollOffsetTop}px`; + container.style.left = `${parentElement.offsetLeft - scrollOffsetLeft}px`; + + if (parentMetadata.isContentClipped) { + // When parent element is not positioned and its content is clipped + // we need to hide overflow of the container as well to prevent + // highlights from appearing for overflown content. + container.classList.add('kontent-smart-link-overlay--restricted'); } } - if (parentMetadata?.hasRelativePosition && parentMetadata?.hasRestrictedOverflow) { - highlight.style.top = `${nodeRect.top - parentRect.top + parentElement.scrollTop}px`; - highlight.style.left = `${nodeRect.left - parentRect.left + parentElement.scrollLeft}px`; - } else { - highlight.style.top = `${nodeRect.top - parentRect.top}px`; - highlight.style.left = `${nodeRect.left - parentRect.left}px`; - } + // If the parent element is positioned and its content is clipped (hidden, scroll, auto, clipped), + // the parent element is an offset parent for the highlight and its scroll position can effect the position + // of the highlight, so we need to reckon with that. + const isPositionedAndClipped = parentMetadata.isPositioned && parentMetadata.isContentClipped; + const scrollOffsetTop = isPositionedAndClipped ? parentElement.scrollTop : 0; + const scrollOffsetLeft = isPositionedAndClipped ? parentElement.scrollLeft : 0; + + highlight.style.top = `${nodeRect.top - parentRect.top + scrollOffsetTop}px`; + highlight.style.left = `${nodeRect.left - parentRect.left + scrollOffsetLeft}px`; } else { - // When there is no ancestor with relative position or restricted overflow (hidden, scroll, etc.) - // highlight is placed into the body and page offset is used. + // No parent element means that there is no positioned or clipped ancestor and that highlight will be + // placed into the body and page offset (page scroll) should be used. highlight.style.top = `${nodeRect.top + window.pageYOffset}px`; highlight.style.left = `${nodeRect.left + window.pageXOffset}px`; } @@ -109,16 +110,21 @@ export class HighlightRenderer implements IRenderer { highlight.style.width = `${nodeRect.width}px`; highlight.style.height = `${nodeRect.height}px`; + // We are creating a new highlight by node map to be able to compare it with an old one to find out + // which nodes have been removed before renders and remove their highlights from the DOM. newHighlightByNode.set(node, highlight); this.highlightByNode.delete(node); } } + // All highlights that are left in the old highlightByNode map are the remnants of the old render + // and should be removed because their node has already been removed/or moved out of the viewport. for (const [node, highlight] of this.highlightByNode.entries()) { highlight.remove(); this.highlightByNode.delete(node); } + // All highlight containers that have no children can be removed because they are not used by any highlight. for (const [parent, container] of this.containerByParent.entries()) { if (container.children.length === 0) { container.remove(); diff --git a/src/utils/node.ts b/src/utils/node.ts index 77bb8e6..bd73742 100644 --- a/src/utils/node.ts +++ b/src/utils/node.ts @@ -1,8 +1,17 @@ interface IParentMetadata { - readonly hasRelativePosition: boolean; - readonly hasRestrictedOverflow: boolean; + readonly isPositioned: boolean; + readonly isContentClipped: boolean; } +/** + * Iterate through node ancestors and find an element which will be used as a parent for + * the highlights container (highlights -> container -> parent). This element should either be + * positioned (position is anything except static) or should have its content clipped. In case of + * table element (td, th, table) it should be positioned or it will be ignored (even if its content is clipped). + * + * @param {HTMLElement} node + * @returns {[HTMLElement, IParentMetadata] | [null, null]} + */ export function getParentForHighlight(node: HTMLElement): [HTMLElement, IParentMetadata] | [null, null] { const parent = node.parentNode; @@ -13,11 +22,21 @@ export function getParentForHighlight(node: HTMLElement): [HTMLElement, IParentM const overflow = computedStyle.getPropertyValue('overflow'); const metadata: IParentMetadata = { - hasRelativePosition: position === 'relative', - hasRestrictedOverflow: overflow.split(' ').some((value) => ['auto', 'scroll', 'clip', 'hidden'].includes(value)), + // The positioned element is an element whose computed position is anything except static. + // Offset top and offset left values of child element are relative to the first positioned ancestor, so we can use + // it to correctly position the highlight. + isPositioned: position !== 'static', + // Content is clipped when overflow of the element is hidden (auto, scroll, clip, hidden). Highlights should be placed + // inside such elements so that they do not overflow their parent. + isContentClipped: overflow.split(' ').some((value) => ['auto', 'scroll', 'clip', 'hidden'].includes(value)), }; - return metadata.hasRelativePosition || metadata.hasRestrictedOverflow + // Table HTML element (td, th, table) can be an offset parent of some node, but unless it is positioned it will not be + // used as a offset parent for the absolute positioned child (highlight). That is why we should ignore those elements and + // do not use them as parents unless they are positioned. Otherwise, the highlighting might broke for the tables. + const isNotTable = !['TD', 'TH', 'TABLE'].includes(parent.tagName); + + return metadata.isPositioned || (metadata.isContentClipped && isNotTable) ? [parent, metadata] : getParentForHighlight(parent); } @@ -25,13 +44,25 @@ export function getParentForHighlight(node: HTMLElement): [HTMLElement, IParentM return [null, null]; } -export function getRelativeScrollOffset(node: Element | null): [number, number] { - if (!node || !(node instanceof HTMLElement)) { +/** + * Iterate through node ancestors until HTMLElement.offsetParent is reached and sum scroll offsets. + * + * @param {HTMLElement | null} node + * @returns {[number, number]} + * where the first number is a totalScrollTop, and the second number is a totalScrollLeft. + */ +export function getTotalScrollOffset(node: HTMLElement | null): [number, number] { + if (!node) { return [0, 0]; } const offsetParent = node.offsetParent; + // HTMLElement.offsetParent can be null when the node is or + if (!offsetParent) { + return [0, 0]; + } + let scrollTop = 0; let scrollLeft = 0; let currentNode = node; diff --git a/test-browser/utils/node.spec.ts b/test-browser/utils/node.spec.ts index f54a0fc..8814baf 100644 --- a/test-browser/utils/node.spec.ts +++ b/test-browser/utils/node.spec.ts @@ -1,16 +1,44 @@ import { createHtmlFixture } from '../test-helpers/createHtmlFixture'; -import { getParentForHighlight } from '../../src/utils/node'; +import { getParentForHighlight, getTotalScrollOffset } from '../../src/utils/node'; describe('node.ts', () => { describe('getParentForHighlight', () => { const fixture = createHtmlFixture(); - it('should return the closest parent with relative position', () => { + it('should return [null, null] when there is no suitable parent', () => { + // + fixture.setHtml(` +
+
+
+
+
+
+
+
+
+
+ +
+ `); + // + + const target = fixture.querySelector('#target') as HTMLElement; + const result = getParentForHighlight(target); + expect(result).toEqual([null, null]); + }); + + it('should return the positioned parent', () => { // fixture.setHtml(`
-
-
+
+
@@ -24,14 +52,14 @@ describe('node.ts', () => { const target = fixture.querySelector('#target') as HTMLElement; const [element, metadata] = getParentForHighlight(target); - expect(element?.id).toEqual('c'); + expect(element?.id).toEqual('b'); expect(metadata).toEqual({ - hasRelativePosition: true, - hasRestrictedOverflow: false, + isPositioned: true, + isContentClipped: false, }); }); - it('should return the closest parent with restricted overflow', () => { + it('should return the parent with clipped content', () => { // fixture.setHtml(`
@@ -52,9 +80,165 @@ describe('node.ts', () => { const [element, metadata] = getParentForHighlight(target); expect(element?.id).toEqual('c'); expect(metadata).toEqual({ - hasRelativePosition: false, - hasRestrictedOverflow: true, + isPositioned: false, + isContentClipped: true, }); }); + + it('should return the closest parent which is positioned or has clipped content', () => { + // + fixture.setHtml(` +
+
+
+
+
+
+
+
+
+
+
+
+ `); + // + + const targetA = fixture.querySelector('#targetA') as HTMLElement; + const targetB = fixture.querySelector('#targetB') as HTMLElement; + + const resultA = getParentForHighlight(targetA); + const resultB = getParentForHighlight(targetB); + + expect(resultA?.[0]?.id).toEqual('c'); + expect(resultB?.[0]?.id).toEqual('a'); + }); + + it('should ignore table elements when they are not positioned', () => { + // + fixture.setHtml(` +
+ + + + + + +
+
+
+
+ `); + // + + const target = fixture.querySelector('#target') as HTMLElement; + const [element] = getParentForHighlight(target); + expect(element?.id).toEqual('a'); + }); + + it('should acknowledge table elements when they are positioned', () => { + // + fixture.setHtml(` +
+ + + + + + +
+
+
+
+ `); + // + + const target = fixture.querySelector('#target') as HTMLElement; + const [element] = getParentForHighlight(target); + expect(element?.id).toEqual('b'); + }); + }); + + describe('getTotalScrollOffset', () => { + const fixture = createHtmlFixture(); + + it('should return [0, 0] when no node provided', () => { + expect(getTotalScrollOffset(null)).toEqual([0, 0]); + }); + + it('should return [0, 0] when element has no offsetParent', () => { + expect(getTotalScrollOffset(document.body)).toEqual([0, 0]); + }); + + it('should return total scroll offset', () => { + // + fixture.setHtml(` +
+
+
+
+
+
+
+
+
+
+
+ `); + // + + const divA = fixture.querySelector('#a') as HTMLElement; + const divC = fixture.querySelector('#c') as HTMLElement; + const target = fixture.querySelector('#target') as HTMLElement; + + divA.scrollTop = 10; + divA.scrollLeft = 20; + + divC.scrollTop = 30; + divC.scrollLeft = 40; + + const [scrollTop, scrollLeft] = getTotalScrollOffset(target); + + expect(Math.ceil(scrollTop)).toEqual(40); + expect(Math.ceil(scrollLeft)).toEqual(60); + }); + + it('should not include scroll offset of the offsetParent', () => { + // + fixture.setHtml(` +
+
+
+
+
+
+
+
+
+
+
+
+
+ `); + // + + const offsetParent = fixture.querySelector('#container') as HTMLElement; + const divB = fixture.querySelector('#b') as HTMLElement; + const divD = fixture.querySelector('#d') as HTMLElement; + const target = fixture.querySelector('#target') as HTMLElement; + + offsetParent.scrollTop = 10; + offsetParent.scrollLeft = 20; + + divB.scrollTop = 30; + divB.scrollLeft = 40; + + divD.scrollTop = 50; + divD.scrollLeft = 60; + + const [scrollTop, scrollLeft] = getTotalScrollOffset(target); + + expect(Math.ceil(scrollTop)).toEqual(80); + expect(Math.ceil(scrollLeft)).toEqual(100); + }); }); });