From dce927891badb76256d714cfe9bbd0617d734215 Mon Sep 17 00:00:00 2001 From: Duc Vo Ngoc Date: Fri, 21 Jun 2024 09:38:16 +0200 Subject: [PATCH 01/15] feat(ui5-table): improve horizontal scroll mode - fixed selection cell to the left - table has its own scrollbar when in Scroll mode - documented restrictions --- packages/main/src/Table.ts | 13 +++++++++++++ packages/main/src/TableHeaderRow.hbs | 1 + packages/main/src/TableHeaderRow.ts | 2 ++ packages/main/src/TableRow.hbs | 2 +- packages/main/src/themes/TableCellBase.css | 6 ++++++ packages/main/src/themes/TableHeaderRow.css | 2 +- packages/main/src/themes/TableRowBase.css | 12 +++++++++++- packages/main/test/pages/Table.html | 10 +++------- 8 files changed, 38 insertions(+), 10 deletions(-) diff --git a/packages/main/src/Table.ts b/packages/main/src/Table.ts index b968a35155ce..341004aa257e 100644 --- a/packages/main/src/Table.ts +++ b/packages/main/src/Table.ts @@ -273,6 +273,17 @@ class Table extends UI5Element { @property({ type: String, defaultValue: "0" }) stickyTop!: string; + /** + * Defines the height of the component. + * + * Note: When using sticky headers, make sure that the height of the table is defined. + * + * @default "auto" + * @public + */ + @property({ type: String, defaultValue: "auto" }) + height!: string; + @property({ type: Integer, defaultValue: 0, noAttribute: true }) _invalidate!: number; @@ -471,6 +482,8 @@ class Table extends UI5Element { return { table: { "grid-template-columns": this._gridTemplateColumns, + "height": this.height, + "overflow-x": this._tableOverflowX, }, }; } diff --git a/packages/main/src/TableHeaderRow.hbs b/packages/main/src/TableHeaderRow.hbs index 4f155d401e40..3ad6687e8ae3 100644 --- a/packages/main/src/TableHeaderRow.hbs +++ b/packages/main/src/TableHeaderRow.hbs @@ -2,6 +2,7 @@ {{#if _isMultiSelect}} + {{#if _isMultiSelect}} - - - + - + Product - Supplier + Supplier Dimensions Weight Price @@ -86,8 +84,6 @@ - - + + + + + + + + + Product + Supplier + Dimensions + Weight + Price + + + Notebook Basic 15
HT-1000 + Very Best Screens + 30 x 18 x 3 cm + 4.2 KG + 956 EUR + + + Notebook Basic 16
HT-1001 + Smartcards + + 4.5 KG + 1249 EUR + + + Notebook Basic 17
HT-1002 + Technocom + 32 x 21 x 4 cm + 3.7 KG + 29 EUR + + + Notebook Basic 18
HT-1003 + Technocom + 32 x 21 x 4 cm + 3.7 KG + 29 EUR + + + Notebook Basic 19
HT-1004 + Technocom + 32 x 21 x 4 cm + 3.7 KG + 29 EUR + + + Notebook Basic 20
HT-1005 + Technocom + 32 x 21 x 4 cm + 3.7 KG + 29 EUR + + + Notebook Basic 21
HT-1006 + Technocom + 32 x 21 x 4 cm + 3.7 KG + 29 EUR + + + + + + + + \ No newline at end of file diff --git a/packages/main/test/specs/Table.spec.js b/packages/main/test/specs/Table.spec.js index edb09264ae7f..3a85689dfb9f 100644 --- a/packages/main/test/specs/Table.spec.js +++ b/packages/main/test/specs/Table.spec.js @@ -196,4 +196,48 @@ describe("Table - Fixed Header", async () => { assert.isAbove(headerYPosition, 50, "Header is above the viewport and above the toolbar"); assert.ok(headerRow.isDisplayedInViewport(), "Header is displayed in the viewport"); }); +}); + +describe("Table - Horizontal Scrolling", async () => { + before(async () => { + await browser.url(`test/pages/TableHorizontal.html`); + }); + + it("selection column should be fixed to the left", async () => { + const table = await browser.$("#table"); + const innerTable = await table.shadow$("#table"); + const lastColumn = await browser.$("#lastCell"); + const firstRow = await browser.$("#firstRow"); + + assert.ok(await table.isExisting(), "Table exists"); + assert.ok(await innerTable.isExisting(), "Inner table exists"); + assert.ok(await lastColumn.isExisting(), "Last column cell exists"); + assert.ok(await firstRow.isExisting(), "First row exists"); + + const { leftOffset, fixedX } = await browser.execute(() => { + const table = document.getElementById("table"); + const row = document.getElementById("firstRow"); + return { + fixedX: row.shadowRoot.querySelector("#selection-cell").getBoundingClientRect().x, + leftOffset: table.shadowRoot.querySelector("#table")?.scrollLeft || 0 + }; + }); + + assert.equal(leftOffset, 0, "Table is not scrolled horizontally"); + assert.equal(fixedX, 0, "Selection column is fixed to the left"); + + await lastColumn.scrollIntoView(); + + const { leftOffset2, fixedX2 } = await browser.execute(() => { + const table = document.getElementById("table"); + const row = document.getElementById("firstRow"); + return { + fixedX2: row.shadowRoot.querySelector("#selection-cell").getBoundingClientRect().x, + leftOffset2: table.shadowRoot.querySelector("#table")?.scrollLeft || 0 + }; + }); + + assert.ok(leftOffset2 > 0, "Table is scrolled horizontally"); + assert.equal(fixedX2, 0, "Selection column is still fixed to the left"); + }); }); \ No newline at end of file From 263ee876dd1e518061b084b3b34f95888cfa1ada Mon Sep 17 00:00:00 2001 From: Duc Vo Ngoc Date: Tue, 25 Jun 2024 10:29:13 +0200 Subject: [PATCH 04/15] refactor: move focus logic into TableUtil --- packages/main/src/Table.ts | 74 ++++----------------------------- packages/main/src/TableUtils.ts | 52 +++++++++++++++++++++++ 2 files changed, 61 insertions(+), 65 deletions(-) diff --git a/packages/main/src/Table.ts b/packages/main/src/Table.ts index cfff4e0da6a4..8f9f3dd9fbff 100644 --- a/packages/main/src/Table.ts +++ b/packages/main/src/Table.ts @@ -24,6 +24,7 @@ import { } from "./generated/i18n/i18n-defaults.js"; import BusyIndicator from "./BusyIndicator.js"; import TableCell from "./TableCell.js"; +import { findScrollContainer, scrollElementIntoView } from "./TableUtils.js"; /** * Interface for components that can be slotted inside the features slot of the ui5-table. @@ -390,54 +391,8 @@ class Table extends UI5Element { } _onfocusin(e: FocusEvent) { - // Handles focus that is below sticky element - const stickyElements = this._stickyElements; - const fixedElements = this._fixedElements; - - if (stickyElements.length === 0 && fixedElements.length === 0) { - return; - } - - // Find the sticky element that is closest to the focused element - const target = e.target as HTMLElement; - const element = target.closest("ui5-table-cell, ui5-table-row") as HTMLElement ?? target; - const elementRect = element.getBoundingClientRect(); - - let stickyBottom = stickyElements.reduce((min, stickyElement) => { - const stickyRect = stickyElement.getBoundingClientRect(); - - if (stickyRect.bottom > elementRect.top) { - return Math.max(min, stickyRect.bottom); - } - return min; - }, 0); - let stickyLeft = fixedElements.reduce((min, fixedElement) => { - if (!fixedElement) { - return min; - } - - const fixedRect = fixedElement.getBoundingClientRect(); - - if (fixedRect.right > elementRect.left) { - return Math.max(min, fixedRect.right); - } - return min; - }, 0); - - // If the focused element is not behind any sticky element, do nothing - if (stickyBottom === 0) { - stickyBottom = elementRect.top; - } - if (stickyLeft === 0) { - stickyLeft = elementRect.left; - } - - // Scroll the focused element into view - const scrollContainer = this._scrollContainer; - scrollContainer.scrollBy({ - top: elementRect.top - stickyBottom, - left: elementRect.left - stickyLeft, - }); + // Handles focus in the table, when the focus is below a sticky element + scrollElementIntoView(this._scrollContainer, e.target as HTMLElement, this._stickyElements); } /** @@ -570,26 +525,15 @@ class Table extends UI5Element { return this.features.find(feature => this._isGrowingFeature(feature)) as ITableGrowing; } - // TODO: Could be moved to UI5Element. TBD - get _scrollContainer() { - let element: HTMLElement = this._tableElement; - while (element) { - const { overflowY } = window.getComputedStyle(element); - if (overflowY === "auto" || overflowY === "scroll") { - return element; - } - element = element.parentElement as HTMLElement; - } - - return document.scrollingElement as HTMLElement || document.documentElement; - } - get _stickyElements() { - return [this.headerRow[0]].filter(row => row.sticky); + const stickyRows = [this.headerRow[0]].filter(row => row.sticky); + const stickyColumns = this.headerRow[0]._cells.filter(cell => cell && cell.hasAttribute("fixed")) as TableHeaderCell[]; + + return [...stickyRows, ...stickyColumns]; } - get _fixedElements() { - return [...this.headerRow[0]._cells.filter(cell => cell?.hasAttribute("fixed"))]; + get _scrollContainer() { + return findScrollContainer(this._tableElement); } get isTable() { diff --git a/packages/main/src/TableUtils.ts b/packages/main/src/TableUtils.ts index c9e19c0e16e6..1436caaeb19a 100644 --- a/packages/main/src/TableUtils.ts +++ b/packages/main/src/TableUtils.ts @@ -17,9 +17,61 @@ const findRowInPath = (composedPath: Array) => { return composedPath.find((el: EventTarget) => el instanceof HTMLElement && el.hasAttribute("ui5-table-row")) as TableRow; }; +const findScrollContainer = (element: HTMLElement): HTMLElement => { + while (element) { + const { overflowY } = window.getComputedStyle(element); + if (overflowY === "auto" || overflowY === "scroll") { + return element; + } + + if (element.parentNode instanceof ShadowRoot) { + element = element.parentNode.host as HTMLElement; + } else { + element = element.parentElement as HTMLElement; + } + } + + return document.scrollingElement as HTMLElement || document.documentElement; +}; + +const scrollElementIntoView = (scrollContainer: HTMLElement, element: HTMLElement, stickyElements: HTMLElement[]) => { + if (stickyElements.length === 0) { + return; + } + + const elementRect = element.getBoundingClientRect(); + + const { x: stickyX, y: stickyY } = stickyElements.reduce(({ x, y }, stickyElement) => { + const { top, left } = getComputedStyle(stickyElement); + const stickyElementRect = stickyElement.getBoundingClientRect(); + if (top !== "auto" && stickyElementRect.bottom > elementRect.top) { + y = Math.max(y, stickyElementRect.bottom); + } + if (left !== "auto" && stickyElementRect.right > elementRect.left) { + x = Math.max(x, stickyElementRect.right); + } + return { x, y }; + }, { x: elementRect.left, y: elementRect.top }); + + const scrollX = elementRect.left - stickyX; + const scrollY = elementRect.top - stickyY; + + if (scrollX === 0 && scrollY === 0) { + // avoid unnecessary scroll call, when nothing changes + return; + } + + scrollContainer.scrollBy({ + top: scrollY, + left: scrollX, + }); +}; + export { isInstanceOfTable, isSelectionCheckbox, isHeaderSelector, findRowInPath, + findScrollContainer, + scrollElementIntoView, }; From d53608bac86f538741a3e77f3f253a65c6309b03 Mon Sep 17 00:00:00 2001 From: Duc Vo Ngoc Date: Tue, 25 Jun 2024 11:34:10 +0200 Subject: [PATCH 05/15] feat(ui5-table): navigated indicator is sticky --- packages/main/src/TableSelection.ts | 6 ++++-- packages/main/src/themes/TableRow.css | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/main/src/TableSelection.ts b/packages/main/src/TableSelection.ts index 25c86c78299b..13d881ba70b3 100644 --- a/packages/main/src/TableSelection.ts +++ b/packages/main/src/TableSelection.ts @@ -250,9 +250,11 @@ class TableSelection extends UI5Element implements ITableFeature { return; } - if (!eventOrigin.hasAttribute("ui5-table-row") || !this._rangeSelection || isShift(e) || !isSelectionCheckbox(e)) { + if (!eventOrigin.hasAttribute("ui5-table-row") || !this._rangeSelection || !isShift(e)) { // Stop range selection if a) Shift is relased or b) the event target is not a row or c) the event is not from the selection checkbox - this._stopRangeSelection(); + if (isSelectionCheckbox(e)) { + this._stopRangeSelection(); + } } if (this._rangeSelection) { diff --git a/packages/main/src/themes/TableRow.css b/packages/main/src/themes/TableRow.css index 27e968660340..14822300d5ee 100644 --- a/packages/main/src/themes/TableRow.css +++ b/packages/main/src/themes/TableRow.css @@ -37,6 +37,8 @@ grid-row: span 2; min-width: 0; padding: 0; + position: sticky; + right: 0; } :host([navigated]) #navigated-cell { From ae0715aef692c4dad0ae5b4816fb211647bae1a4 Mon Sep 17 00:00:00 2001 From: Duc Vo Ngoc Date: Tue, 25 Jun 2024 13:15:02 +0200 Subject: [PATCH 06/15] test: add test for sticky indicator --- packages/main/test/pages/TableHorizontal.html | 2 +- packages/main/test/specs/Table.spec.js | 20 ++++++++++++++----- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/packages/main/test/pages/TableHorizontal.html b/packages/main/test/pages/TableHorizontal.html index 611ff2a158f8..1c3e51b9c292 100644 --- a/packages/main/test/pages/TableHorizontal.html +++ b/packages/main/test/pages/TableHorizontal.html @@ -25,7 +25,7 @@ Weight Price - + Notebook Basic 15
HT-1000 Very Best Screens 30 x 18 x 3 cm diff --git a/packages/main/test/specs/Table.spec.js b/packages/main/test/specs/Table.spec.js index 08283aea0e09..90c979a60524 100644 --- a/packages/main/test/specs/Table.spec.js +++ b/packages/main/test/specs/Table.spec.js @@ -203,16 +203,26 @@ describe("Table - Horizontal Scrolling", async () => { await browser.url(`test/pages/TableHorizontal.html`); }); + it("navigated indicator is fixed to the right", async () => { + const table = await browser.$("#table"); + + assert.ok(await table.isExisting(), "Table exists"); + + const rightOffset = await browser.execute(() => { + const table = document.getElementById("table"); + const row = document.getElementById("firstRow"); + + return row.shadowRoot.querySelector("#navigated-cell").getBoundingClientRect().right - table.getBoundingClientRect().right; + }); + + assert.equal(rightOffset, 0, "Navigated indicator is fixed to the right"); + }); + it("selection column should be fixed to the left", async () => { const table = await browser.$("#table"); - const innerTable = await table.shadow$("#table"); const lastColumn = await browser.$("#lastCell"); - const firstRow = await browser.$("#firstRow"); assert.ok(await table.isExisting(), "Table exists"); - assert.ok(await innerTable.isExisting(), "Inner table exists"); - assert.ok(await lastColumn.isExisting(), "Last column cell exists"); - assert.ok(await firstRow.isExisting(), "First row exists"); const { leftOffset, fixedX } = await browser.execute(() => { const table = document.getElementById("table"); From ec6677a39ba67e9808c9fe76151bcc5c254fe2d4 Mon Sep 17 00:00:00 2001 From: Duc Vo Ngoc Date: Tue, 25 Jun 2024 16:14:10 +0200 Subject: [PATCH 07/15] test: fix offset --- packages/main/test/specs/Table.spec.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/main/test/specs/Table.spec.js b/packages/main/test/specs/Table.spec.js index 90c979a60524..54cfe1e07117 100644 --- a/packages/main/test/specs/Table.spec.js +++ b/packages/main/test/specs/Table.spec.js @@ -209,10 +209,9 @@ describe("Table - Horizontal Scrolling", async () => { assert.ok(await table.isExisting(), "Table exists"); const rightOffset = await browser.execute(() => { - const table = document.getElementById("table"); const row = document.getElementById("firstRow"); - return row.shadowRoot.querySelector("#navigated-cell").getBoundingClientRect().right - table.getBoundingClientRect().right; + return row.shadowRoot.querySelector("#navigated-cell").getBoundingClientRect().right; }); assert.equal(rightOffset, 0, "Navigated indicator is fixed to the right"); From e1f697fdbdd99a5f89462e93273510a54b511ad1 Mon Sep 17 00:00:00 2001 From: Duc Vo Ngoc Date: Wed, 26 Jun 2024 09:11:47 +0200 Subject: [PATCH 08/15] test: adjust test assertion --- packages/main/test/specs/Table.spec.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/main/test/specs/Table.spec.js b/packages/main/test/specs/Table.spec.js index 54cfe1e07117..ced95633ce3d 100644 --- a/packages/main/test/specs/Table.spec.js +++ b/packages/main/test/specs/Table.spec.js @@ -208,13 +208,16 @@ describe("Table - Horizontal Scrolling", async () => { assert.ok(await table.isExisting(), "Table exists"); - const rightOffset = await browser.execute(() => { + const { navigatedOffset, tableOffset } = await browser.execute(() => { const row = document.getElementById("firstRow"); - return row.shadowRoot.querySelector("#navigated-cell").getBoundingClientRect().right; + return { + navigatedOffset: row.shadowRoot.querySelector("#navigated-cell").getBoundingClientRect().right, + tableOffset: document.getElementById("table").shadowRoot.querySelector("#table").getBoundingClientRect().right, + }; }); - assert.equal(rightOffset, 0, "Navigated indicator is fixed to the right"); + assert.equal(navigatedOffset, tableOffset, `Navigated indicator is fixed to the right (${navigatedOffset}, ${tableOffset})`); }); it("selection column should be fixed to the left", async () => { From 355af47dd26b8001b2e4d8f4b383659909c16cc7 Mon Sep 17 00:00:00 2001 From: Duc Vo Ngoc Date: Wed, 26 Jun 2024 09:31:00 +0200 Subject: [PATCH 09/15] test: adjust test assertion --- packages/main/test/specs/Table.spec.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/main/test/specs/Table.spec.js b/packages/main/test/specs/Table.spec.js index ced95633ce3d..0199088c5256 100644 --- a/packages/main/test/specs/Table.spec.js +++ b/packages/main/test/specs/Table.spec.js @@ -208,16 +208,16 @@ describe("Table - Horizontal Scrolling", async () => { assert.ok(await table.isExisting(), "Table exists"); - const { navigatedOffset, tableOffset } = await browser.execute(() => { + const { navigatedOffset, rowOffset } = await browser.execute(() => { const row = document.getElementById("firstRow"); return { navigatedOffset: row.shadowRoot.querySelector("#navigated-cell").getBoundingClientRect().right, - tableOffset: document.getElementById("table").shadowRoot.querySelector("#table").getBoundingClientRect().right, + rowOffset: row.getBoundingClientRect().right, }; }); - assert.equal(navigatedOffset, tableOffset, `Navigated indicator is fixed to the right (${navigatedOffset}, ${tableOffset})`); + assert.equal(navigatedOffset, rowOffset, `Navigated indicator is fixed to the right (${navigatedOffset}, ${rowOffset})`); }); it("selection column should be fixed to the left", async () => { From 28f25be051613f986a44590f726162ef9eec7e77 Mon Sep 17 00:00:00 2001 From: Duc Vo Ngoc Date: Wed, 26 Jun 2024 10:10:59 +0200 Subject: [PATCH 10/15] chore: adjust test and documentation --- packages/main/src/Table.ts | 4 ++-- packages/main/test/specs/Table.spec.js | 11 ++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/main/src/Table.ts b/packages/main/src/Table.ts index b10b30cdf99c..56df91bcb1e7 100644 --- a/packages/main/src/Table.ts +++ b/packages/main/src/Table.ts @@ -283,12 +283,12 @@ class Table extends UI5Element { /** * Defines the height of the component. * - * Note: When using sticky headers, make sure that the height of the table is defined. + * **Note:** When using sticky headers, make sure that the height of the table is defined. * * @default "auto" * @public */ - @property({}) + @property() height = "auto"; @property({ type: Number, noAttribute: true }) diff --git a/packages/main/test/specs/Table.spec.js b/packages/main/test/specs/Table.spec.js index 0199088c5256..f377f9ce2b07 100644 --- a/packages/main/test/specs/Table.spec.js +++ b/packages/main/test/specs/Table.spec.js @@ -208,16 +208,17 @@ describe("Table - Horizontal Scrolling", async () => { assert.ok(await table.isExisting(), "Table exists"); - const { navigatedOffset, rowOffset } = await browser.execute(() => { - const row = document.getElementById("firstRow"); + const { position, right } = await browser.execute(() => { + const navigatedCell = row.shadowRoot.querySelector("#navigated-cell"); return { - navigatedOffset: row.shadowRoot.querySelector("#navigated-cell").getBoundingClientRect().right, - rowOffset: row.getBoundingClientRect().right, + position: getComputedStyle(navigatedCell).position, + right: getComputedStyle(navigatedCell).right }; }); - assert.equal(navigatedOffset, rowOffset, `Navigated indicator is fixed to the right (${navigatedOffset}, ${rowOffset})`); + assert.equal(position, "sticky", `Navigated indicator is sticky`); + assert.equal(right, "0px", `Navigated indicator is at the right edge`); }); it("selection column should be fixed to the left", async () => { From 52ccb791cd45a1cede398b17a42bdaf6f4b4ad97 Mon Sep 17 00:00:00 2001 From: Duc Vo Ngoc Date: Wed, 26 Jun 2024 10:28:00 +0200 Subject: [PATCH 11/15] test: fix test --- packages/main/test/specs/Table.spec.js | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/packages/main/test/specs/Table.spec.js b/packages/main/test/specs/Table.spec.js index f377f9ce2b07..5860f9611a1b 100644 --- a/packages/main/test/specs/Table.spec.js +++ b/packages/main/test/specs/Table.spec.js @@ -208,17 +208,16 @@ describe("Table - Horizontal Scrolling", async () => { assert.ok(await table.isExisting(), "Table exists"); - const { position, right } = await browser.execute(() => { - const navigatedCell = row.shadowRoot.querySelector("#navigated-cell"); + const row = await browser.$("#firstRow"); + const navigatedCell = await row.shadow$("#navigated-cell"); - return { - position: getComputedStyle(navigatedCell).position, - right: getComputedStyle(navigatedCell).right - }; - }); + assert.ok(await navigatedCell.isExisting(), "Navigated cell exists"); + + const stickyProperty = await navigatedCell.getCSSProperty("position"); + const rightProperty = await navigatedCell.getCSSProperty("right"); - assert.equal(position, "sticky", `Navigated indicator is sticky`); - assert.equal(right, "0px", `Navigated indicator is at the right edge`); + assert.strictEqual(stickyProperty.value, "sticky", "Navigated cell is sticky"); + assert.strictEqual(rightProperty.value, "0px", "Navigated cell is at the right edge"); }); it("selection column should be fixed to the left", async () => { From c05ef29468c1d55fa68de991430b4396e7183143 Mon Sep 17 00:00:00 2001 From: Duc Vo Ngoc Date: Tue, 2 Jul 2024 09:03:11 +0200 Subject: [PATCH 12/15] fix: rtl support - adjust code according to feedback - adjust focus logic to account for rtl --- packages/main/src/Table.ts | 8 +++---- packages/main/src/TableHeaderRow.ts | 1 + packages/main/src/TableRowBase.ts | 12 ++++++---- packages/main/src/TableUtils.ts | 22 ++++++++++++------- packages/main/src/themes/TableRow.css | 4 ++++ packages/main/src/themes/TableRowBase.css | 10 ++++++--- .../main/src/themes/base/Table-parameters.css | 9 ++++++++ packages/main/test/pages/Table.html | 2 +- 8 files changed, 48 insertions(+), 20 deletions(-) diff --git a/packages/main/src/Table.ts b/packages/main/src/Table.ts index 56df91bcb1e7..606efe9c8ff7 100644 --- a/packages/main/src/Table.ts +++ b/packages/main/src/Table.ts @@ -24,7 +24,7 @@ import { } from "./generated/i18n/i18n-defaults.js"; import BusyIndicator from "./BusyIndicator.js"; import TableCell from "./TableCell.js"; -import { findScrollContainer, scrollElementIntoView } from "./TableUtils.js"; +import { findVerticalScrollContainer, scrollElementIntoView } from "./TableUtils.js"; /** * Interface for components that can be slotted inside the features slot of the ui5-table. @@ -410,7 +410,7 @@ class Table extends UI5Element { _onfocusin(e: FocusEvent) { // Handles focus in the table, when the focus is below a sticky element - scrollElementIntoView(this._scrollContainer, e.target as HTMLElement, this._stickyElements); + scrollElementIntoView(this._scrollContainer, e.target as HTMLElement, this._stickyElements, this.effectiveDir === "rtl"); } /** @@ -548,13 +548,13 @@ class Table extends UI5Element { get _stickyElements() { const stickyRows = [this.headerRow[0]].filter(row => row.sticky); - const stickyColumns = this.headerRow[0]._cells.filter(cell => cell && cell.hasAttribute("fixed")) as TableHeaderCell[]; + const stickyColumns = this.headerRow[0]._stickyCells as TableHeaderCell[]; return [...stickyRows, ...stickyColumns]; } get _scrollContainer() { - return findScrollContainer(this._tableElement); + return findVerticalScrollContainer(this._tableElement); } get isTable() { diff --git a/packages/main/src/TableHeaderRow.ts b/packages/main/src/TableHeaderRow.ts index 8f95d6681f07..e6a51b74f6dd 100644 --- a/packages/main/src/TableHeaderRow.ts +++ b/packages/main/src/TableHeaderRow.ts @@ -101,6 +101,7 @@ class TableHeaderRow extends TableRowBase { } get _cells() { + // only add if not null #selection-cell if it is not null to the array return [this.shadowRoot?.querySelector("#selection-cell"), ...this.cells]; } } diff --git a/packages/main/src/TableRowBase.ts b/packages/main/src/TableRowBase.ts index e736f964b232..09bf0b7b949e 100644 --- a/packages/main/src/TableRowBase.ts +++ b/packages/main/src/TableRowBase.ts @@ -112,12 +112,16 @@ abstract class TableRowBase extends UI5Element { return this.cells.filter(c => c._popin); } - get _i18nRowSelector(): string { - return TableRowBase.i18nBundle.getText(TABLE_ROW_SELECTOR); + get _stickyCells() { + const selectionCell = this.shadowRoot?.querySelector("#selection-cell"), + navigatedCell = this.shadowRoot?.querySelector("#navigated-cell"); + + // filter out null/undefined + return [selectionCell, ...this.cells, navigatedCell].filter(cell => cell?.hasAttribute("fixed")); } - get isTableRowBase() { - return true; + get _i18nRowSelector(): string { + return TableRowBase.i18nBundle.getText(TABLE_ROW_SELECTOR); } } diff --git a/packages/main/src/TableUtils.ts b/packages/main/src/TableUtils.ts index 1436caaeb19a..94f530e3e2ad 100644 --- a/packages/main/src/TableUtils.ts +++ b/packages/main/src/TableUtils.ts @@ -17,7 +17,7 @@ const findRowInPath = (composedPath: Array) => { return composedPath.find((el: EventTarget) => el instanceof HTMLElement && el.hasAttribute("ui5-table-row")) as TableRow; }; -const findScrollContainer = (element: HTMLElement): HTMLElement => { +const findVerticalScrollContainer = (element: HTMLElement): HTMLElement => { while (element) { const { overflowY } = window.getComputedStyle(element); if (overflowY === "auto" || overflowY === "scroll") { @@ -34,26 +34,32 @@ const findScrollContainer = (element: HTMLElement): HTMLElement => { return document.scrollingElement as HTMLElement || document.documentElement; }; -const scrollElementIntoView = (scrollContainer: HTMLElement, element: HTMLElement, stickyElements: HTMLElement[]) => { +const scrollElementIntoView = (scrollContainer: HTMLElement, element: HTMLElement, stickyElements: HTMLElement[], isRtl: boolean) => { if (stickyElements.length === 0) { return; } const elementRect = element.getBoundingClientRect(); + const inline = isRtl ? "right" : "left"; const { x: stickyX, y: stickyY } = stickyElements.reduce(({ x, y }, stickyElement) => { - const { top, left } = getComputedStyle(stickyElement); + const { top, [inline]: inlineStart } = getComputedStyle(stickyElement); const stickyElementRect = stickyElement.getBoundingClientRect(); if (top !== "auto" && stickyElementRect.bottom > elementRect.top) { y = Math.max(y, stickyElementRect.bottom); } - if (left !== "auto" && stickyElementRect.right > elementRect.left) { - x = Math.max(x, stickyElementRect.right); + if (inlineStart !== "auto") { + if (!isRtl && stickyElementRect.right > elementRect.left) { + x = Math.max(x, stickyElementRect.right); + } else if (isRtl && stickyElementRect.left < elementRect.right) { + x = Math.min(x, stickyElementRect.left); + } } + return { x, y }; - }, { x: elementRect.left, y: elementRect.top }); + }, { x: elementRect[inline], y: elementRect.top }); - const scrollX = elementRect.left - stickyX; + const scrollX = elementRect[inline] - stickyX; const scrollY = elementRect.top - stickyY; if (scrollX === 0 && scrollY === 0) { @@ -72,6 +78,6 @@ export { isSelectionCheckbox, isHeaderSelector, findRowInPath, - findScrollContainer, + findVerticalScrollContainer, scrollElementIntoView, }; diff --git a/packages/main/src/themes/TableRow.css b/packages/main/src/themes/TableRow.css index 14822300d5ee..ebe2136f0abe 100644 --- a/packages/main/src/themes/TableRow.css +++ b/packages/main/src/themes/TableRow.css @@ -41,6 +41,10 @@ right: 0; } +:dir(rtl)#navigated-cell { + left: 0; +} + :host([navigated]) #navigated-cell { background-color: var(--sapList_SelectionBorderColor); } diff --git a/packages/main/src/themes/TableRowBase.css b/packages/main/src/themes/TableRowBase.css index 085bff25dc9d..6af56bc22633 100644 --- a/packages/main/src/themes/TableRowBase.css +++ b/packages/main/src/themes/TableRowBase.css @@ -17,6 +17,10 @@ left: 0; } +:dir(rtl)#selection-cell { + right: 0; +} + #selection-component { vertical-align: middle; } @@ -24,7 +28,7 @@ /** Focus outline for the selection cell */ :host([tabindex]:focus) #selection-cell { outline: none; - box-shadow: inset 0 var(--sapContent_FocusWidth) var(--sapContent_FocusColor), - inset var(--sapContent_FocusWidth) 0 var(--sapContent_FocusColor), - inset 0 -1px var(--sapContent_FocusColor); /* There is a 1px difference between row and cell, therefore the outline is also 1px difference between their outline offsets */ + box-shadow: var(--_ui5_table_shadow_border_top), + var(--_ui5_table_shadow_border_left), + var(--_ui5_table_shadow_border_bottom); /* There is a 1px difference between row and cell, therefore the outline is also 1px difference between their outline offsets */ } diff --git a/packages/main/src/themes/base/Table-parameters.css b/packages/main/src/themes/base/Table-parameters.css index 970efb577542..7cf2b221d514 100644 --- a/packages/main/src/themes/base/Table-parameters.css +++ b/packages/main/src/themes/base/Table-parameters.css @@ -2,4 +2,13 @@ --_ui5_table_cell_valign: center; --_ui5_table_cell_min_width: 2.75rem; --_ui5_table_navigated_cell_width: 0.25rem; + --_ui5_table_shadow_border_left: inset var(--sapContent_FocusWidth) 0 var(--sapContent_FocusColor); + --_ui5_table_shadow_border_right: inset calc(-1 * var(--sapContent_FocusWidth)) 0 var(--sapContent_FocusColor); + --_ui5_table_shadow_border_top: inset 0 var(--sapContent_FocusWidth) var(--sapContent_FocusColor); + --_ui5_table_shadow_border_bottom: inset 0 -1px var(--sapContent_FocusColor); +} + +[dir="rtl"] { + --_ui5_table_shadow_border_left: inset calc(-1 * var(--sapContent_FocusWidth)) 0 var(--sapContent_FocusColor); + --_ui5_table_shadow_border_right: inset var(--sapContent_FocusWidth) 0 var(--sapContent_FocusColor); } \ No newline at end of file diff --git a/packages/main/test/pages/Table.html b/packages/main/test/pages/Table.html index 4a494a6e5d52..652ed8a53962 100644 --- a/packages/main/test/pages/Table.html +++ b/packages/main/test/pages/Table.html @@ -15,7 +15,7 @@ - + My Selectable Products (3) From d7312d669a43cda08815e5883af0c1da10f59949 Mon Sep 17 00:00:00 2001 From: Duc Vo Ngoc Date: Fri, 5 Jul 2024 15:14:45 +0200 Subject: [PATCH 13/15] chore: implement feedback --- packages/main/src/Table.ts | 16 ++-------------- packages/main/src/TableHeaderRow.ts | 5 ----- packages/main/test/pages/Table.html | 4 ++-- 3 files changed, 4 insertions(+), 21 deletions(-) diff --git a/packages/main/src/Table.ts b/packages/main/src/Table.ts index 50bab89ccb4f..8147f2bcb003 100644 --- a/packages/main/src/Table.ts +++ b/packages/main/src/Table.ts @@ -289,17 +289,6 @@ class Table extends UI5Element { @property() stickyTop = "0"; - /** - * Defines the height of the component. - * - * **Note:** When using sticky headers, make sure that the height of the table is defined. - * - * @default "auto" - * @public - */ - @property() - height = "auto"; - @property({ type: Number, noAttribute: true }) _invalidate = 0; @@ -477,7 +466,6 @@ class Table extends UI5Element { return { table: { "grid-template-columns": this._gridTemplateColumns, - "height": this.height, "overflow-x": this._tableOverflowX, }, }; @@ -503,7 +491,7 @@ class Table extends UI5Element { } get _tableOverflowX() { - return (this.overflowMode === TableOverflowMode.Popin) ? "" : "auto"; + return (this.overflowMode === TableOverflowMode.Popin) ? "clip" : "auto"; } get _tableOverflowY() { @@ -552,7 +540,7 @@ class Table extends UI5Element { } get _stickyElements() { - const stickyRows = [this.headerRow[0]].filter(row => row.sticky); + const stickyRows = this.headerRow.filter(row => row.sticky); const stickyColumns = this.headerRow[0]._stickyCells as TableHeaderCell[]; return [...stickyRows, ...stickyColumns]; diff --git a/packages/main/src/TableHeaderRow.ts b/packages/main/src/TableHeaderRow.ts index ebe731d20363..a4767758c636 100644 --- a/packages/main/src/TableHeaderRow.ts +++ b/packages/main/src/TableHeaderRow.ts @@ -100,11 +100,6 @@ class TableHeaderRow extends TableRowBase { get _i18nRowPopin() { return TableRowBase.i18nBundle.getText(TABLE_ROW_POPIN); } - - get _cells() { - // only add if not null #selection-cell if it is not null to the array - return [this.shadowRoot?.querySelector("#selection-cell"), ...this.cells]; - } } TableHeaderRow.define(); diff --git a/packages/main/test/pages/Table.html b/packages/main/test/pages/Table.html index 36e1211be949..228c9e800649 100644 --- a/packages/main/test/pages/Table.html +++ b/packages/main/test/pages/Table.html @@ -15,7 +15,7 @@ - + My Selectable Products (3) @@ -23,7 +23,7 @@ label-interval="0"> - + From 82b2d4c451eecd911050880542bf9fed2c947a43 Mon Sep 17 00:00:00 2001 From: Duc Vo Ngoc Date: Tue, 23 Jul 2024 10:39:51 +0200 Subject: [PATCH 14/15] docs: adjust documentation to reflect need for height --- packages/main/src/Table.ts | 1 - packages/main/src/TableHeaderRow.ts | 3 ++- packages/main/src/themes/Table.css | 4 ++++ packages/main/test/pages/Table.html | 2 +- 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/main/src/Table.ts b/packages/main/src/Table.ts index be7a36d2bd61..3133b2790368 100644 --- a/packages/main/src/Table.ts +++ b/packages/main/src/Table.ts @@ -467,7 +467,6 @@ class Table extends UI5Element { return { table: { "grid-template-columns": this._gridTemplateColumns, - "overflow-x": this._tableOverflowX, }, }; } diff --git a/packages/main/src/TableHeaderRow.ts b/packages/main/src/TableHeaderRow.ts index a4767758c636..b3ec30366d3b 100644 --- a/packages/main/src/TableHeaderRow.ts +++ b/packages/main/src/TableHeaderRow.ts @@ -66,7 +66,8 @@ class TableHeaderRow extends TableRowBase { /** * Sticks the `ui5-table-header-row` to the top of a table. * - * Note: If used in combination with overflowMode "Scroll", the table needs a defined height for the sticky header to work as expected. + * Note: If used in combination with overflowMode "Scroll", the table needs a defined height + * or needs to be inside of a container with a defined height for the sticky header to work as expected. * * @default false * @public diff --git a/packages/main/src/themes/Table.css b/packages/main/src/themes/Table.css index ffab6b155632..85e7af1752c1 100644 --- a/packages/main/src/themes/Table.css +++ b/packages/main/src/themes/Table.css @@ -9,6 +9,10 @@ display: none; } +:host([overflow-mode="Scroll"]) { + overflow-x: scroll; +} + #table { display: grid; grid-auto-rows: minmax(min-content, auto); diff --git a/packages/main/test/pages/Table.html b/packages/main/test/pages/Table.html index 228c9e800649..389b196e0435 100644 --- a/packages/main/test/pages/Table.html +++ b/packages/main/test/pages/Table.html @@ -23,7 +23,7 @@ label-interval="0"> - + From c2b19937b8cee3378b4a496fadb3f2b9aa50074c Mon Sep 17 00:00:00 2001 From: Duc Vo Ngoc Date: Tue, 23 Jul 2024 10:56:30 +0200 Subject: [PATCH 15/15] fix: adjust failing test --- packages/main/test/specs/Table.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/main/test/specs/Table.spec.js b/packages/main/test/specs/Table.spec.js index bfbf2ebf7804..8cc64483b03c 100644 --- a/packages/main/test/specs/Table.spec.js +++ b/packages/main/test/specs/Table.spec.js @@ -245,7 +245,7 @@ describe("Table - Horizontal Scrolling", async () => { const row = document.getElementById("firstRow"); return { fixedX2: row.shadowRoot.querySelector("#selection-cell").getBoundingClientRect().x, - leftOffset2: table.shadowRoot.querySelector("#table")?.scrollLeft || 0 + leftOffset2: table.scrollLeft || 0 }; });