Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make sure unhidden combo-box items get re-rendered #6638

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

tomivirkki
Copy link
Member

@tomivirkki tomivirkki commented Oct 12, 2023

Description

Fixes vaadin/flow-components#1304

When the Virtualizer's size gets reduced to a value smaller than the number of elements it currently manages, it simply hides the excess elements instead of removing them from the DOM. When this happens, it's possible that an element that represents some item gets hidden, while another element continues representing the same item.

This can happen, for example, with a combo box when you filter the list of options. Consider the following scenario:

  1. Initial combo-box dropdown content with three elements representing three items:
ComboBox
AA
BB
CC
  1. Filter the combo box with "B". Now we are left with just 1 visible element that represents item BB. The other elements are hidden and not updated.
ComboBox
BB
BB [hidden]
CC [hidden]
  1. Remove the filter. The first element is updated and the hidden elements become visible again.
ComboBox
AA
BB
CC

In phase 3, the second element, which was hidden in phase 2 still represents the same item (BB) in the same index (1) it did before it was hidden. In <vaadin-combo-box-item>'s case, this would mean that none of the dependencies of the __rendererOrItemChanged observer function got changed between hiding and unhiding the element, so it would not trigger a re-render while becoming visible again either.

In most cases, this wouldn't be a problem. However, the ComponentRenderer works in the way that it obtains a reference to a node by a node ID it receives and then appends that node inside the content root. In the above scenario, if a ComponentRenderer is used and the items are actual elements instead of text:

  • 1: There are 3 ComponentRenderers at play, each receiving different node IDs (1, 2, and 3), and they append the corresponding nodes to the DOM.
  • 2 (filter): The node ID of the first element's ComponentRenderer is changed to 2. Based on the updated ID, it gets the node "BB", which is still attached to the (hidden) second element, and replaces its previous content "AA" with it. As a result, "BB" gets removed from the second (hidden) element, because a DOM node can't exist in two places simultaneously.
  • 3 (clear filter): The node ID of the first element's ComponentRenderer is changed back to 1 so its content is changed back to "AA". The second element becomes visible again, but since it still has the same index and item it had before getting hidden, no re-renders are triggered and the content remains empty. The third element also becomes visible again, but its content was never taken by another element so it's fine.
Kapture.2023-10-12.at.15.19.24.mp4

This PR fixes the issue by making <vaadin-combo-box-item> observe the hidden attribute and based on that, when the Virtualizer hides it, it clears one of the __rendererOrItemChanged dependencies. This guarantees that a re-render gets triggered when the element becomes visible again and its properties are updated.

Type of change

Bugfix

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@DiegoCardoso DiegoCardoso left a comment

Choose a reason for hiding this comment

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

Tested in a Flow app running this code and confirmed that the issue goes away.

@web-padawan web-padawan merged commit a75aa3d into main Oct 13, 2023
9 checks passed
@web-padawan web-padawan deleted the fix/combo-box/unhidden-items-renderer branch October 13, 2023 06:32
@vaadin-bot
Copy link
Collaborator

Hi @tomivirkki and @web-padawan, when i performed cherry-pick to this commit to 23.3, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick a75aa3d
error: could not apply a75aa3d... fix: make sure unhidden combo-box items get re-rendered (#6638)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

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.

ComponentRenderer fails when filtering moves the item in the overlay
4 participants