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

There's two backdrop panels to cause blurred avatars in the room list #19014

Closed
turt2live opened this issue Sep 13, 2021 · 11 comments
Closed

There's two backdrop panels to cause blurred avatars in the room list #19014

turt2live opened this issue Sep 13, 2021 · 11 comments
Labels
A-Developer-Experience A-Room-List T-Task Tasks for the team like planning

Comments

@turt2live
Copy link
Member

Steps to reproduce

  1. Inspect the room list DOM tree

What happened?

What did you expect?

One backdrop.

What happened?

Two backdrops. Possibly related: #19013

image

Operating system

Windows 10

Browser information

Chrome

URL for webapp

develop.element.io

Homeserver

t2l.io

Have you submitted a rageshake?

No

@SimonBrandner SimonBrandner added A-Room-List O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Severely degrades major functionality or product features, with no satisfactory workaround X-Regression labels Sep 13, 2021
@Palid
Copy link
Contributor

Palid commented Sep 13, 2021

@turt2live Can you give me a comparison screenshot of the blurred avatars and non-blurred ones without those backdrop panels? I can't reproduce it on my machine and I can't see how two DOM elements instead of one are a relevant problem.

@turt2live
Copy link
Member Author

It's just DOM noise that we don't need. It's not anything of major concern here, more a maintenance task.

The performance impact, if any, would be #19013

@turt2live turt2live added S-Tolerable Low/no impact on users and removed S-Major Severely degrades major functionality or product features, with no satisfactory workaround labels Sep 13, 2021
@Palid
Copy link
Contributor

Palid commented Sep 13, 2021

@turt2live it's not a DOM noise that we don't need, it's the most optimized version of the background-blur effect we can get without spending even more time refactoring our DOM structure. Both of the elements are necessary to properly achieve this effect. CC @janogarcia

About the performance problem, I don't really think it's related exactly to this change, as changes I did while working on it have drastically improved the rendering time and general performance on slower machines. I currently don't even have even a slight idea what can be the problem in #19013 - I can only assume that it's somewhere around bugged hardware acceleration in this specific config.

@turt2live
Copy link
Member Author

... there must be a way to achieve the effect without layering two DOM elements on top of each other. The clipping is already quite heavy on the machine because it extends far beyond the room list: if we can reduce the amount of clipping, then I'd be okay with the two DOM elements, but at present the two are using significant area that isn't even rendered.

@Palid
Copy link
Contributor

Palid commented Sep 13, 2021

... there must be a way to achieve the effect without layering two DOM elements on top of each other. The clipping is already quite heavy on the machine because it extends far beyond the room list: if we can reduce the amount of clipping, then I'd be okay with the two DOM elements, but at present the two are using significant area that isn't even rendered.

Please profile the problem and we can try figuring it out later without breaking this design. I was toying a little bit with using <div> with background-image: url instead of <img> tag so that the background would only extend properly, but it was hard to get exactly the same behavior we get with <img> tags. We could probably remove one of those elements and replace it with some shadow/lighten background instead, but for now, unless it really kills performance on some machines, I don't see a very good reason why we should immediately start working on it.

The biggest performance problem there is with this background is on Safari, because it doesn't support css contain property that exactly takes care of the problem you described. We could try figuring out a workaround for this case I suppose.

@Palid
Copy link
Contributor

Palid commented Sep 13, 2021

There's really a lot of different problems related to this tiny little effect which seems relatively simple until you actually start digging into it. I think the path I went through while refactoring this feature was nicely visible in the merge requests:
First canvas implementation that turned out to work horribly: matrix-org/matrix-react-sdk#6262
And then the actual img-css implementation that turned out to work much better: matrix-org/matrix-react-sdk#6659

There was also a lot of minor fixes that were regression-related or design related (not in a specific order):
Design: matrix-org/matrix-react-sdk#6688
Resizer, cause it's behavior was quite hardcoded and didn't work with new layout anymore: matrix-org/matrix-react-sdk#6670
Layers optimization: matrix-org/matrix-react-sdk#6672
DOM changes regressions: matrix-org/matrix-react-sdk#6669

I think that currently there is a way to make it more performant without having to go through the same regressions pathway I did, but I can't really justify prioritising it right now.

@turt2live
Copy link
Member Author

I'm not sure I can make it more clear: this is not anywhere near a request to immediately fix the problem. It's a minor note that there's DOM redundancy, which means it can get fixed or looked at in weeks or months.

This is not a performance issue. It is not a high priority bug report. It is just a note of something noticed while investigating a different issue.

@novocaine novocaine added T-Task Tasks for the team like planning A-Developer-Experience and removed X-Regression T-Defect S-Tolerable Low/no impact on users O-Occasional Affects or can be seen by some users regularly or most users rarely labels Sep 17, 2021
@novocaine
Copy link
Contributor

I don't think this is a defect, so categorising it as DX.

@Palid
Copy link
Contributor

Palid commented Sep 17, 2021

@novocaine it is some kind of a defect, as for slower machines, or weird browser configs (see that Firefox Fedora issue we had) it is creating a lot of headache. We could improve it so there's only one of those.

@novocaine
Copy link
Contributor

novocaine commented Sep 17, 2021

Then we should change the title and acceptance criteria to reflect the user impact (probably a new issue at this point)

@turt2live
Copy link
Member Author

I'm confused as to what's happening on this issue, so just going to close it for evaluation later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Developer-Experience A-Room-List T-Task Tasks for the team like planning
Projects
None yet
Development

No branches or pull requests

4 participants