Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Move backdrop filter to a canvas based solution #6262

Merged
merged 41 commits into from
Aug 19, 2021

Conversation

germain-gg
Copy link
Contributor

@germain-gg germain-gg commented Jun 24, 2021

After many issue with the backdrop filter having glitches on hover and sparse browser compatibility. This is an attempt to move this implementation to a canvas

This introduces a polyfill for CanvasRenderingContext2D.filter so that Safari can display the image with the correct blur value

Reference values: https://web-ui-d998d9.webflow.io/

Rendering examples:

Dark theme:

https://cln.sh/gx3s3X51UeT8HALlD38b Amandine
https://cln.sh/FT0bYIZoxD12I7gDY8HR Matthew
https://cln.sh/rCaxgIGC1igFjwdHD6B0 Ben
https://cln.sh/ZB23KitwV5RZIwxCHrZU Nad
https://cln.sh/NRn4KQEfX62FdUHS5Ti5 Banana
https://cln.sh/RAG9n0yFPSUAEfUupF5L Vertical rainbow
https://cln.sh/1pzKtaTIlBpQjpL2aGb5 Horizontal rainbow

Light theme:

https://cln.sh/5tUWPe0M3crsnLKxB9VM Amandine
https://cln.sh/LT4weX5qPFuQjtQHkJpf Matthew
https://cln.sh/VZkGthQkbYLv2wKsIGmV Ben
https://cln.sh/AJyQ2ptfxhHfvOWJCt0f Nad
https://cln.sh/9z0Y8cdcWrpeElTUrkRN Banana
https://cln.sh/ovDie7SxIrrRvsJszWtD Vertical rainbow
https://cln.sh/B7tgvFrxDtLE99JK87ai Horizontal rainbow


This PR currently has no changelog labels, so will not be included in changelogs.

Add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is plus X-Breaking-Change if it's a breaking change.

Preview: https://611df9c6fbab22b2d967978d--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

@germain-gg germain-gg requested a review from a team June 25, 2021 08:42
@germain-gg germain-gg marked this pull request as ready for review June 25, 2021 08:43
src/components/structures/BackdropPanel.tsx Show resolved Hide resolved
src/components/structures/BackdropPanel.tsx Outdated Show resolved Hide resolved
src/components/structures/LeftPanel.tsx Outdated Show resolved Hide resolved
src/utils/drawable.ts Show resolved Hide resolved
src/utils/drawable.ts Outdated Show resolved Hide resolved
@germain-gg germain-gg requested a review from t3chguy June 25, 2021 10:49
@germain-gg germain-gg requested a review from t3chguy July 12, 2021 11:53
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Hmm, all tonality seems to be lost

Before:
image

After:
image

Once the blurred avatar appears it looks very different to how it used to

Before:
image

After:
image

If you enable the spaces beta at least some form of split appears but it still looks very different to the intended (as designed look)

image

Removing my avatar does not update the blur at all, so it remains as the old avatar.

With a light avatar it makes it obvious the tonality is missing, looks almost identical to not having an avatar at all.

image

@SimonBrandner
Copy link
Contributor

This also makes resizing the room-list a bit less performant on FF :(

@Palid Palid self-assigned this Aug 9, 2021
…rop-filter

* origin/develop: (1278 commits)
  Add a little padding
  Keep number field in focus when pressing dialpad buttons (#6520)
  Remove old version
  Fix video call persisting when widget removed
  Update link to matrix-js-sdk CONTRIBUTING file (#6557)
  $toast-bg-color -> $system
  $system-... -> $system
  Iterate PR based on feedback
  Remove unnecessary code
  Use AccessibleTooltipButton
  Just upload the PR object itself
  Edit PR Description instead of commenting
  publish the right directory
  Fix Netflify builds from fork PRs
  This doesn't need to be here as it was moved into CallViewButtons
  Make scrollbar dot transparent
  Iterate PR based on feedback
  Don't set hidden RRs labs setting at account level
  Add a comment for weirdly placed div
  Add full class names to animations.scss
  ...
@Palid
Copy link
Contributor

Palid commented Aug 17, 2021

Two things:

* When I load the Netlify preview for the first time the backdrop isn't shown

* When I expand the room-list to its full width I can see a grey area

Screenshot_20210817_181858

fixed!

@SimonBrandner
Copy link
Contributor

The first issue seems to still happen :(

@Palid
Copy link
Contributor

Palid commented Aug 17, 2021

The first issue seems to still happen :(

still hasn't deployed, give it a few minutes. 😄

Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

Seems to work now!

(not a code review! :D)

@thany
Copy link

thany commented Aug 17, 2021

How do you know if it works? The preview opens in the browser, but the issue is with the desktop app.

Also if a bloody backdrop causes blurry text, remove the backdrop! Who gives a toss about some backdrop if it makes text illegable? Seriously, what manner of importance did this backdrop have to prioritize it over legible text?!

I say just remove the backdrop. It'll make this part of the app a hundred times simpler, text won't become blurry, and there will be no performance problems.

@Palid
Copy link
Contributor

Palid commented Aug 18, 2021

How do you know if it works? The preview opens in the browser, but the issue is with the desktop app.

Also if a bloody backdrop causes blurry text, remove the backdrop! Who gives a toss about some backdrop if it makes text illegable? Seriously, what manner of importance did this backdrop have to prioritize it over legible text?!

I say just remove the backdrop. It'll make this part of the app a hundred times simpler, text won't become blurry, and there will be no performance problems.

@thany the problem isn't with desktop app only, it's with chrome fonts rendering when layers are changed and too much of the application gets rerasterized. The desktop application is just a chrome wrapped in a nice package with some better, native APIs.

Could you try verifying yourself that fonts are more readable now on netlify deployment? Please be wary that there's one more bug that breaks the rendering, but you can "fix" it by clicking the + button in the top left corner. We're currently figuring out how to make this all work together without getting rid of this simple Call To Action there.

In terms of removing the backdrop: that was the first thing I suggested, as the previous backdrop solution (css-only based) was notoriously buggy, but turns out it's a quite relevant feature for application identity, even though it seems like a very minor thing. It's best that we have found a solution that works, is performant and gives you even better experience than before. 😄

If you have any more questions about the decisions made here we can either continue in the linked issue so we don't add not related to the PR comments here, or you can catch me on matrix in the Element Web/Desktop room, or @Palid:matrix.org .

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

otherwise I think this looks good

src/components/structures/BackdropPanel.tsx Show resolved Hide resolved
Dariusz Niemczyk added 4 commits August 19, 2021 07:11
…rop-filter

* origin/develop: (43 commits)
  Update copy to indicate debug logs contain which UI elements you last interacted with
  Fix name of Netlify workflow
  Add type declarations
  Fix pagination and improve typing
  Fix import
  Reset matrix-js-sdk back to develop branch
  v3.28.1
  Prepare changelog for v3.28.1
  Upgrade matrix-js-sdk to 12.3.1
  Explicitly handle first state change
  Properly listen for call_state
  Proper init in constructors
  Resetting package fields for development
  v3.28.0
  Prepare changelog for v3.28.0
  Fix error on accessing encrypted media without keys
  Fix call tile buttons
  Upgrade matrix-js-sdk to 12.3.0
  Remove test code; good job we have tests
  Fix dates
  ...
@Palid Palid requested a review from dbkr August 19, 2021 05:14
@Palid Palid enabled auto-merge August 19, 2021 05:14
@Palid Palid dismissed t3chguy’s stale review August 19, 2021 08:59

Problem has been solved already, and re-review isn't necessary in this case.

@Palid Palid merged commit 9398741 into develop Aug 19, 2021
@Palid Palid deleted the gsouquet/fix-backdrop-filter branch August 19, 2021 08:59
@dbkr
Copy link
Member

dbkr commented Aug 20, 2021

Comparative screenshots between app/develop now this has been merged:

Before:
Screenshot 2021-08-20 at 16 56 31

After:
Screenshot 2021-08-20 at 16 56 46

@Palid
Copy link
Contributor

Palid commented Aug 20, 2021

@dbkr the calls are now aligned left for a reasons fortunately not related to this one (the new .mx_CallEvent_wrapper is missing justify-content: center if it was supposed to be center-aligned), but I can't figure out how to reproduce those enormous tiles.
Newest develop as of now, could've been fixed already (probably?)

@SimonBrandner
Copy link
Contributor

@dbkr the calls are now aligned left for a reasons fortunately not related to this one

That is an intentional change

but I can't figure out how to reproduce those enormous tiles. Newest develop as of now, could've been fixed already (probably?)

That is probably unrelated, I've played with a few numbers there, so I think it's unrelated

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants