Skip to content

Conversation

@ugur-vaadin
Copy link
Contributor

@ugur-vaadin ugur-vaadin commented Mar 21, 2025

Description

In the Cell class, z-index of 1 is automatically applied to cells with text content. However, this should not be the case for sub-cells. The first sub-cell in a merged region still contains the text. Usually, this problem is hidden because initially the sub-cells are added to the DOM before the merged cell. When there are two cells with the same z-index, the last added cell is shown on top. However, when the sheet is scrolled, sometimes the sub-cell is removed and the merged cell still stays in the DOM. Scrolling back to the row, the sub-cell is added back to the DOM with the same z-index of 1. Since the sub-cell is the last added one in this case, it is shown on top.

This PR adds a check in the Cell class to avoid setting z-index for sub-cells with text.

Fixes #7177

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/contributing/overview
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

@ugur-vaadin ugur-vaadin force-pushed the fix-make-sure-subcells-are-hidden branch from 68527b7 to 4f87395 Compare March 31, 2025 07:56
Copy link
Contributor

@yuriy-fix yuriy-fix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, the focus outline (+ handle) of the merged cell is broken.
From the provided Merged_cell_scroll_example.xlsx:

Without the change:

  • Normal initial state:
    initial
  • Broken state after scrolling:
    current-broken

With the change:

  • Merged cell selected:
    latest-broken

Notice that the focus outline (+ handle) is not displayed correctly when the merged cell is selected.

@ugur-vaadin ugur-vaadin force-pushed the fix-make-sure-subcells-are-hidden branch from 58148c5 to d78526c Compare April 4, 2025 17:40
@ugur-vaadin
Copy link
Contributor Author

With this change, the focus outline (+ handle) of the merged cell is broken.

I reverted the initial fix and applied a new one. In the Cell class, z-index of 1 is automatically applied to cells with text content. However, this should not be the case for sub-cells. The first sub-cell in a merged region still contains the text. Usually, this problem is hidden because initially the sub-cells are added to the DOM before the merged cell. When there are two cells with the same z-index, the last added cell is shown on top. However, when the sheet is scrolled, sometimes the sub-cell is removed and the merged cell still stays in the DOM. Scrolling back to the row, the sub-cell is added back to the DOM with the same z-index of 1. Since the sub-cell is the last added one in this case, it is shown on top.

The new solution seems much cleaner and appropriate. There are similar checks in the Cell class already, changing its style based on whether it is a sub-cell. The focus outline problem is also not present in this version.

@ugur-vaadin ugur-vaadin changed the title fix: make sure subcells in merged regions are hidden fix: do not set z-index for sub-cells with text Apr 4, 2025
@ugur-vaadin ugur-vaadin force-pushed the fix-make-sure-subcells-are-hidden branch from f23992e to 16e9d98 Compare April 7, 2025 12:13
@ugur-vaadin ugur-vaadin enabled auto-merge (squash) April 7, 2025 12:13
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 7, 2025

@ugur-vaadin ugur-vaadin merged commit eb29716 into main Apr 7, 2025
4 of 5 checks passed
@ugur-vaadin ugur-vaadin deleted the fix-make-sure-subcells-are-hidden branch April 7, 2025 12:38
ugur-vaadin added a commit that referenced this pull request Apr 7, 2025
* fix: make sure subcells are hidden

* fix: do not set zindex for subcells with text and revert initial fix

* test: replace the test to match the new fix
ugur-vaadin added a commit that referenced this pull request Apr 7, 2025
* fix: make sure subcells are hidden

* fix: do not set zindex for subcells with text and revert initial fix

* test: replace the test to match the new fix
ugur-vaadin added a commit that referenced this pull request Apr 7, 2025
* fix: make sure subcells are hidden

* fix: do not set zindex for subcells with text and revert initial fix

* test: replace the test to match the new fix
ugur-vaadin added a commit that referenced this pull request Apr 8, 2025
* fix: do not set z-index for sub-cells with text (#7239)

* fix: make sure subcells are hidden

* fix: do not set zindex for subcells with text and revert initial fix

* test: replace the test to match the new fix

* test: fix getting styles in the it
ugur-vaadin added a commit that referenced this pull request Apr 8, 2025
* fix: make sure subcells are hidden

* fix: do not set zindex for subcells with text and revert initial fix

* test: replace the test to match the new fix
ugur-vaadin added a commit that referenced this pull request Apr 8, 2025
* fix: make sure subcells are hidden

* fix: do not set zindex for subcells with text and revert initial fix

* test: replace the test to match the new fix
ugur-vaadin added a commit that referenced this pull request Apr 8, 2025
* fix: make sure subcells are hidden

* fix: do not set zindex for subcells with text and revert initial fix

* test: replace the test to match the new fix
ugur-vaadin added a commit that referenced this pull request Apr 8, 2025
* fix: make sure subcells are hidden

* fix: do not set zindex for subcells with text and revert initial fix

* test: replace the test to match the new fix

Co-authored-by: Sascha Ißbrücker <sissbruecker@vaadin.com>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 23.5.16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Merged cells broken after scrolling

4 participants