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

Fix dark and white theme switch in Chrome #9320

Merged
merged 10 commits into from
Oct 7, 2022
Merged

Fix dark and white theme switch in Chrome #9320

merged 10 commits into from
Oct 7, 2022

Conversation

florianduros
Copy link
Contributor

@florianduros florianduros commented Sep 27, 2022 •
edited by github-actions bot
Loading

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Notes: Fixes (element-hq/element-web/issues/22609) where the white theme is not applied when white -> dark -> white sequence is done.

Chrome fires the load event of a style sheet only the first time when the sheet is enabled. If the sheet is disabled and enabled, the sheet will not fire a new load event.

To reproduce, go to element-web with the white theme selected by default:

  • Switch to dark => the dark theme is applied
  • Switch dark to white => the dark theme is still active

Here's what your changelog entry will look like:

🐛 Bug Fixes

@florianduros
Add types for styleElements object
be8e44e
@florianduros
Fix the white/black theme switch in Chrome
f574247 
Chrome doesn't fire twice the load event on a stylesheet when the disabled attribute is toggled (enabled => disabled => enabled)
@florianduros florianduros added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Sep 27, 2022
@florianduros florianduros requested a review from a team as a code owner September 27, 2022 14:01
@t3chguy
Copy link
Member

t3chguy commented Sep 27, 2022

Merging the branch won't make the CI pass, it needs to get the minimum test coverage in order

image

@florianduros
Copy link
Contributor Author

florianduros commented Sep 27, 2022 •
edited
Loading

Merging the branch won't make the CI pass, it needs to get the minimum test coverage in order

image

I have failed ci job about the login (login.spec.ts). I'm a bit confused, I think I didn't touch the login code.

In which job, I can see this error about code coverage ?

Thanks

weeman1337
weeman1337 suggested changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Just one comment. Can you add tests, update the branch and then re-request review?

src/theme.ts Outdated Show resolved Hide resolved
@weeman1337
Copy link
Contributor

In which job, I can see this error about code coverage ?

You can find the detailed report there @florianduros :
image

@t3chguy
Copy link
Member

t3chguy commented Sep 27, 2022

@florianduros https://sonarcloud.io/summary/new_code?id=matrix-react-sdk&pullRequest=9320

image

@florianduros florianduros changed the title Fix black and white theme switch in Chrome Fix dark and white theme switch in Chrome Sep 28, 2022
weeman1337
weeman1337 approved these changes Sep 28, 2022
View reviewed changes
Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests. Tested on the Netlify deployment with Chrome, but I don't have access to an environment as described in the issue. To be 100 % someone with this env should test it.

test/theme-test.ts Outdated Show resolved Hide resolved
@florianduros
Copy link
Contributor Author

florianduros commented Sep 28, 2022 •
edited
Loading

Which environnment ?

@weeman1337
Copy link
Contributor

Which environnment ?

Chrome on Windows 11

@turt2live turt2live self-requested a review September 29, 2022 15:56
@turt2live
Copy link
Member

Right, so I've done some testing on Windows 10 and 11 with not-great results. It seems to get stuck in dark theme for some reason, and on Win 11 Edge specifically it doesn't seem to come out of dark theme:

(regedit because these are not activated versions of Windows in the slightest)

2022-09-29.14-04-30.mp4
2022-09-29.14-07-12.mp4

@turt2live turt2live removed their request for review October 4, 2022 03:35
weeman1337
weeman1337 suggested changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Thanks to Travis for testing!

@florianduros can you have another look at it and re-request review once you have finished?

@florianduros
Copy link
Contributor Author

florianduros commented Oct 6, 2022 •
edited
Loading

I tested in w11 VM and I don't have the issue of @turt2live

Screen.Recording.2022-10-06.at.16.02.06.mov

Chrome on the left and Edge on the right

@weeman1337 what can I do ? :)

@weeman1337
Copy link
Contributor

I've tested this on the Windows 10 dev VM using Chrome. Works fine there. Also tested the current development and can reproduce the issue there. @florianduros can you update your branch?

@florianduros florianduros merged commit a11c1e1 into develop Oct 7, 2022
@florianduros florianduros deleted the florianduros/fix-white-black-theme-switch branch October 7, 2022 07:40
su-ex added a commit to SchildiChat/matrix-react-sdk that referenced this pull request Oct 29, 2022
@su-ex
Merge tag 'v3.59.0' into sc
4ebdd74 
* Include a file-safe room name and ISO date in chat exports ([\matrix-org#9440](matrix-org#9440)). Fixes element-hq/element-web#21812 and element-hq/element-web#19724.
* Room call banner ([\matrix-org#9378](matrix-org#9378)). Fixes element-hq/element-web#23453. Contributed by @toger5.
* Device manager - spinners while devices are signing out ([\matrix-org#9433](matrix-org#9433)). Fixes element-hq/element-web#15865.
* Device manager - silence call ringers when local notifications are silenced ([\matrix-org#9420](matrix-org#9420)).
* Pass the current language to Element Call ([\matrix-org#9427](matrix-org#9427)).
* Hide screen-sharing button in Element Call on desktop ([\matrix-org#9423](matrix-org#9423)).
* Add reply support to WysiwygComposer ([\matrix-org#9422](matrix-org#9422)). Contributed by @florianduros.
* Disconnect other connected devices (of the same user) when joining an Element call ([\matrix-org#9379](matrix-org#9379)).
* Device manager - device tile main click target ([\matrix-org#9409](matrix-org#9409)).
* Add formatting buttons to the rich text editor ([\matrix-org#9410](matrix-org#9410)). Contributed by @florianduros.
* Device manager - current session context menu ([\matrix-org#9386](matrix-org#9386)).
* Remove piwik config fallback for privacy policy URL ([\matrix-org#9390](matrix-org#9390)).
* Add the first step to integrate the matrix wysiwyg composer ([\matrix-org#9374](matrix-org#9374)). Contributed by @florianduros.
* Device manager - UA parsing tweaks ([\matrix-org#9382](matrix-org#9382)).
* Device manager - remove client information events when disabling setting ([\matrix-org#9384](matrix-org#9384)).
* Add Element Call participant limit ([\matrix-org#9358](matrix-org#9358)).
* Add Element Call room settings ([\matrix-org#9347](matrix-org#9347)).
* Device manager - render extended device information ([\matrix-org#9360](matrix-org#9360)).
* New group call experience: Room header and PiP designs ([\matrix-org#9351](matrix-org#9351)).
* Pass language to Jitsi Widget ([\matrix-org#9346](matrix-org#9346)). Contributed by @Fox32.
* Add notifications and toasts for Element Call calls ([\matrix-org#9337](matrix-org#9337)).
* Device manager - device type icon ([\matrix-org#9355](matrix-org#9355)).
* Delete the remainder of groups ([\matrix-org#9357](matrix-org#9357)). Fixes element-hq/element-web#22770.
* Device manager - display client information in device details ([\matrix-org#9315](matrix-org#9315)).
* Send Content-Type: application/json header for integration manager /register API ([\matrix-org#9490](matrix-org#9490)). Fixes element-hq/element-web#23580.
* Device manager - put client/browser device metadata in correct section ([\matrix-org#9447](matrix-org#9447)).
* update the room unread notification counter when the server changes the value without any related read receipt ([\matrix-org#9438](matrix-org#9438)).
* Don't show call banners in video rooms ([\matrix-org#9441](matrix-org#9441)).
* Prevent useContextMenu isOpen from being true if the button ref goes away ([\matrix-org#9418](matrix-org#9418)). Fixes matrix-org/element-web-rageshakes#15637.
* Automatically focus the WYSIWYG composer when you enter a room ([\matrix-org#9412](matrix-org#9412)).
* Improve the tooltips on the call lobby join button ([\matrix-org#9428](matrix-org#9428)).
* Pass the homeserver's base URL to Element Call ([\matrix-org#9429](matrix-org#9429)). Fixes element-hq/element-web#23301.
* Better accommodate long room names in call toasts ([\matrix-org#9426](matrix-org#9426)).
* Hide virtual widgets from the room info panel ([\matrix-org#9424](matrix-org#9424)). Fixes element-hq/element-web#23494.
* Inhibit clicking on sender avatar in threads list ([\matrix-org#9417](matrix-org#9417)). Fixes element-hq/element-web#23482.
* Correct the dir parameter of MSC3715 ([\matrix-org#9391](matrix-org#9391)). Contributed by @dhenneke.
* Use a more correct subset of users in `/remakeolm` developer command ([\matrix-org#9402](matrix-org#9402)).
* use correct default for notification silencing ([\matrix-org#9388](matrix-org#9388)). Fixes element-hq/element-web#23456.
* Device manager - eagerly create `m.local_notification_settings` events ([\matrix-org#9353](matrix-org#9353)).
* Close incoming Element call toast when viewing the call lobby ([\matrix-org#9375](matrix-org#9375)).
* Always allow enabling sending read receipts ([\matrix-org#9367](matrix-org#9367)). Fixes element-hq/element-web#23433.
* Fixes (element-hq/element-web/issues/22609) where the white theme is not applied when `white -> dark -> white` sequence is done. ([\matrix-org#9320](matrix-org#9320)). Contributed by @florianduros.
* Fix applying programmatically set height for "top" room layout ([\matrix-org#9339](matrix-org#9339)). Contributed by @Fox32.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@weeman1337 weeman1337 weeman1337 approved these changes

@andybalaam andybalaam Awaiting requested review from andybalaam andybalaam is a code owner automatically assigned from matrix-org/element-web-reviewers

@justjanne justjanne Awaiting requested review from justjanne justjanne is a code owner automatically assigned from matrix-org/element-web-reviewers

Assignees
No one assigned
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@florianduros @t3chguy @weeman1337 @turt2live