Avoid browser freeze when copy paste icon in icon picker #14387

piitaya · 2022-11-15T15:25:14Z

Proposed change

Avoid browser freeze when copy paste icon in icon picker. I just did not understand why. It also depends of the pasted content. Setting filteredItems async fixes the problem.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New feature (thank you!)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example configuration

Additional information

This PR fixes or closes issue: fixes Pasting Icon Text name in Entity Icon Field Causes Freeze of Dashboard. #14147
This PR is related to issue or discussion:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
There is no commented out code in this PR.
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

steverep · 2022-11-15T21:56:58Z

src/components/ha-icon-picker.ts

-        this.comboBox.filteredItems = filteredItems;
+        // Setting it sync cause browser freeze when user paste an icon text
+        setTimeout(() => {
+          this.comboBox.filteredItems = filteredItems;


I think this is a dirty fix to a bigger problem. The problem I think stems from going behind lit's back to update the items instead of tying them to a reactive property. So whenever a litElement update is triggered, you are starting over with iconItems because that's in render.

Instead, @property the filtered items, update them on input, and let lit handle the update.

Yes you right. I think the filtered items assignment is old code to avoid re-rendering the whole input (created before ha-combobox) but it doesn't make sense anymore. I will look to refactor the component 🙂

👍 I think you could probably keep most as is and just replace this.combobox.filteredItems everywhere with a reactive property update. Might also need to call this.requestUpdate() if the property is only mutated.

I changed it to a reactive state but the setTimeout is always needed... 😕 I have the same behavior. I didn't understand why it's just when pasting a text...

Well, at least it's cleaner now 😜

I was so in disbelief that this actually fixes the problem that I pulled the branch down to try it out. When I test, the initial paste into an empty field goes smoothly, but then I can repeatedly crash the browser by:

Tab to the next field

Shift+tab back to the icon picker

Paste over the 1st paste or just delete it and paste again

I'll try to spend a few more minutes on it to see if I can narrow it down.

It also depend of the content paste. If you paste mdi:home, it crash. If you paste mdi:account it doesn't crash. I think the proper solution is to use web worker for the filter to do the search in a background thread.

piitaya · 2022-11-17T08:30:50Z

Doing the computation in background seems to work. I also removed the minimum characters count because it's not needed anymore.

steverep · 2022-11-17T14:19:32Z

My hypothesis last night was that the pastes are probably just firing too many filter-changed events, triggering multiple racing computations. I started to try a fix but didn't get a chance to finish last night. You beat me to it but I finished anyway and it worked great. Here's what I did:

Make the filter-changed listener just set a this._filterString reactive state.
Move the remaining code to perform the filter to render(). This way lit will ensure it only happens once per update (and probably only once per paste)
For the open-changed listener, just call this.requestUpdate() after load instead of setting the filtered items.

This seems to work smoothly no matter what I paste or how many times. I can push it to another branch if you want to take a look, but it was pretty simple.

PS - I also noticed a potential issue when loading custom icon sets but that's unrelated to this.

bramkragten · 2022-11-17T14:22:29Z

I think a combination of both solutions might be best, depending on how long filtering actually takes with the fix from Steve. (if it is fast, there is no need to do it in a background worker)

steverep · 2022-11-17T14:40:38Z

I'll push it out so you guys can take a look and test

steverep · 2022-11-17T15:18:33Z

See #14401

steverep · 2022-11-18T01:08:28Z

I can still repeatedly cause a crash by using the same technique as above even with the web worker. I can also crash it by quickly typing gibberish into the field.

piitaya · 2022-11-18T08:38:08Z

I removed the web worker and I use afterNextRender. I did not reproduce the problem with this change. I can't wait to see if this fixes the problem for you too 😅

Misiu · 2022-11-18T09:38:28Z

My hypothesis last night was that the pastes are probably just firing too many filter-changed events, triggering multiple racing computations. I started to try a fix but didn't get a chance to finish last night. You beat me to it but I finished anyway and it worked great. Here's what I did:

Maybe we can debounce the filter changed event? There is an open issue for vaadin combobox (vaadin/web-components#518), so we must add this for now.

steverep · 2022-11-18T14:35:42Z

Nope, still crashes 😞

I had a similar idea though, but first did the changes I made in #14401 make any difference for you?

piitaya · 2022-11-18T15:17:32Z

Still crashes too 😕
I will do some tests next week with cross devices and browsers tests : Windows 10, Mac OS, iOS and Android (Chrome/Firefox/Safari).

steverep · 2022-11-18T15:21:55Z

Okay, one more trick to try... I'll push in a minute or two

piitaya · 2022-11-21T10:48:04Z

I rebased the PR to have the last vaadin version.

I tested of several devices and saw no issues.

MacOS - Chrome
MacOS - Firefox
MacOS - Safari
iOS - Safari
Windows 11 - Chrome
WIndows 11 - Edge
Windows 11 - Firefox
Android - Chrome

Test protocol :

Open default dashboard
Click on the first entity displayed
Click on settings page in more-info dialog
Paste mdi:home text in the icon picker

@steverep Can you confirm the issue is fixed for you too? If not, can you describe the steps to reproduce it ? 🙂

steverep · 2022-11-21T17:30:26Z

Nope, this branch still has the issue for me on Windows 10 with Chrome. Firefox seems much more resistant because I haven't been able to reproduce it there for a while. Steps are the same as I stated above. Here's more detail:

Load HA with demo content
Open more info then settings tab for CO2 sensor
Paste "mdi:molecule-co2" in icon field (which icon doesn't matter)
Tab to entity field then tab back
Paste "mdi:molecule-co2" again (or another icon)

I think browser is a factor, but if it's a timing issue then any number of factors could affect it.

FYI, I've also tried many other things without success, including completely removing the rendering of icons in the overlay and field. I'm thinking we should treat this as a vaadin bug and solicit their help.

steverep · 2022-12-02T13:58:06Z

Closing since #14401 was merged.

home-assistant bot added the cla-signed label Nov 15, 2022

piitaya force-pushed the fix_browser_freeze_copy_paste branch from 4af2379 to 0ab938d Compare November 15, 2022 15:27

steverep reviewed Nov 15, 2022

View reviewed changes

bramkragten previously approved these changes Nov 16, 2022

View reviewed changes

piitaya dismissed bramkragten’s stale review via ba9f311 November 17, 2022 08:27

bramkragten previously approved these changes Nov 17, 2022

View reviewed changes

steverep mentioned this pull request Nov 17, 2022

Improve filtering and performance for icon picker #14401

Merged

8 tasks

piitaya dismissed bramkragten’s stale review via b3803d8 November 18, 2022 08:34

steverep mentioned this pull request Nov 18, 2022

Bump @vaadin to 23.2.9 #14408

Merged

8 tasks

piitaya added 5 commits November 21, 2022 11:35

Avoid browser freeze when copy paste icon in icon picker

d2d1365

Clean ha-combobox events

ab33e83

use state instead of combobox assignment

26c0fe1

do icon search in a background thread

6c23640

don't use web worker

900b9cd

piitaya force-pushed the fix_browser_freeze_copy_paste branch from b3803d8 to 900b9cd Compare November 21, 2022 10:35

steverep closed this Dec 2, 2022

steverep deleted the fix_browser_freeze_copy_paste branch January 11, 2023 06:11

github-actions bot locked and limited conversation to collaborators Jan 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid browser freeze when copy paste icon in icon picker #14387

Avoid browser freeze when copy paste icon in icon picker #14387

piitaya commented Nov 15, 2022 •

edited

Loading

steverep Nov 15, 2022

piitaya Nov 15, 2022 •

edited

Loading

steverep Nov 15, 2022

piitaya Nov 16, 2022 •

edited

Loading

steverep Nov 16, 2022

piitaya Nov 17, 2022

piitaya commented Nov 17, 2022 •

edited

Loading

steverep commented Nov 17, 2022

bramkragten commented Nov 17, 2022

steverep commented Nov 17, 2022

steverep commented Nov 17, 2022

steverep commented Nov 18, 2022

piitaya commented Nov 18, 2022

Misiu commented Nov 18, 2022

steverep commented Nov 18, 2022

piitaya commented Nov 18, 2022 •

edited

Loading

steverep commented Nov 18, 2022

piitaya commented Nov 21, 2022

steverep commented Nov 21, 2022

steverep commented Dec 2, 2022

Avoid browser freeze when copy paste icon in icon picker #14387

Avoid browser freeze when copy paste icon in icon picker #14387

Conversation

piitaya commented Nov 15, 2022 • edited Loading

Proposed change

Type of change

Example configuration

Additional information

Checklist

steverep Nov 15, 2022

Choose a reason for hiding this comment

piitaya Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

steverep Nov 15, 2022

Choose a reason for hiding this comment

piitaya Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

steverep Nov 16, 2022

Choose a reason for hiding this comment

piitaya Nov 17, 2022

Choose a reason for hiding this comment

piitaya commented Nov 17, 2022 • edited Loading

steverep commented Nov 17, 2022

bramkragten commented Nov 17, 2022

steverep commented Nov 17, 2022

steverep commented Nov 17, 2022

steverep commented Nov 18, 2022

piitaya commented Nov 18, 2022

Misiu commented Nov 18, 2022

steverep commented Nov 18, 2022

piitaya commented Nov 18, 2022 • edited Loading

steverep commented Nov 18, 2022

piitaya commented Nov 21, 2022

steverep commented Nov 21, 2022

steverep commented Dec 2, 2022

piitaya commented Nov 15, 2022 •

edited

Loading

piitaya Nov 15, 2022 •

edited

Loading

piitaya Nov 16, 2022 •

edited

Loading

piitaya commented Nov 17, 2022 •

edited

Loading

piitaya commented Nov 18, 2022 •

edited

Loading