matrix-react-sdk icon indicating copy to clipboard operation
matrix-react-sdk copied to clipboard

Allow user to set timezone

Open Timshel opened this issue 1 year ago • 34 comments

Hey,

This adds the ability for a user to set a Timezone which will replace the default one from the browser.

Worked on it mainly because Firefox with resist fingerprinting (Cf https://github.com/element-hq/element-web/issues/25006) set the timezone to UTC, and I'm not aware of an easy way to whitelist only specific domains.

Edit: probably should have searched more before coding but there is supposed to be a whitelist in Firefox RFP but for now I have issues with it :sweat_smile:. And this might be useful in other cases.

Checklist

  • [ ] Tests written for new code (and old code if feasible).
  • [ ] New or updated public/exported symbols have accurate TSDoc documentation.
  • [x] Linter and other CI checks pass.
  • [x] Sign-off given on the changes (see CONTRIBUTING.md).

Timshel avatar Jul 12 '24 19:07 Timshel

Sorry wrong button

t3chguy avatar Jul 12 '24 19:07 t3chguy

Not sure of the process but the PR is in a relative good state and working well.

Set it as WIP since:

  • I'm aware of a test failing since the properties screen now has more element.
  • Might need more translation ?

But wanted some feedback before spending more time on it :).

Timshel avatar Jul 12 '24 19:07 Timshel

I've requested review from @matrix-org/product but they are quite busy so it may take some time, it is their decision what features land

t3chguy avatar Jul 12 '24 22:07 t3chguy

I'd like @americanrefugee 's input on the design of the button and it's layout. In terms of functionality I would accept this enhancement if/when implemented correctly

Here's how I see it in the Netlify build. Let me know if I'm actually looking at the wrong thing. Screenshot 2024-07-16 at 12 35 19

daniellekirkwood avatar Jul 16 '24 11:07 daniellekirkwood

Made a short recording of the field in action:

Recording 2024-07-16 at 15 19 17

Timshel avatar Jul 16 '24 13:07 Timshel

@americanrefugee I don't have access even with an account to the Figma file.

Timshel avatar Jul 16 '24 13:07 Timshel

Sorry @Timshel I didn't realize at first that you're not an Element employee - which is why I can't give you access to our design files.

However, here are screenshots of how the feature should look like, as well as dropdown menu states: Dropdown Settings_ General - PC

As for the functionality, I agree that it's useful to be able to type to filter dropdown menu results. However, the user shouldn't actually be typing anything into the field.

americanrefugee avatar Jul 16 '24 13:07 americanrefugee

Such a Dropdown component is being worked on in https://github.com/element-hq/compound-web/pull/204 and should be reusable here once it lands

t3chguy avatar Jul 16 '24 13:07 t3chguy

Ok I'll switch to having the dropdown justified on the left with the label on top.

I agree that it's useful to be able to type to filter dropdown menu results. However, the user shouldn't actually be typing anything into the field.

I'll mention first that it's reusing the same dropdown that the one used at the top of the page for Application language.

But I'm not sure what you mean by not "typing anything into the field" do you mean not visible ? The filtering is essentially a search, without the "search" term visible the user won't be able to see if he made a typo, and it will make deleting the search term quite tricky.

If you mean it to work like the classic list with keyboard navigation then I would mention that there is over 400 values so I believe filtering is quite important here.

Timshel avatar Jul 16 '24 14:07 Timshel

I suppose if the user starts typing then it can follow the regular form field component:

Type=Text, State=Typing, Dark=Off

And here is the Clear / X button:

Button_ Clear

  • By default, the selected menu item label should appear
  • If typing, the content are replaced with a cursor and "Clear" (X) button
    • The list should update as the user types
  • If the user clicks the x / clear button, the original label re-appears, the up arrow reappears, and all menu items are shown

Something like that?

americanrefugee avatar Jul 17 '24 09:07 americanrefugee

@americanrefugee adding the Clear/ X would make sense, but I believe there is some "conflict" with the down arrow which indicate that it's a dropdown (probably why it's not already present).

As I mentioned I'm just reusing an existing component, so any change would impact multiple existing fields. To be honest I believe this is a separate issue and should not be addressed here; and since this handling is already used in the same page I don't believe it should block this PR.

Timshel avatar Jul 17 '24 10:07 Timshel

Ok, reuse the existing component. Don't wanna block anything :)

americanrefugee avatar Jul 17 '24 10:07 americanrefugee

@americanrefugee do you have a recommendation for the dropdown width ?

By default, it matches the width of the selected option (The container has the class mx_SettingsSubsection_contentStretch). For the other dropdown (Application dropdown ) it matches the hint width (The app will reload after selecting another language).

Added the current Timezone in the Browser default placeholder/option using the Timezone short format defined as:

Short localized form (e.g.: PST, GMT-8)

Cf https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/DateTimeFormat

For now it look like this element

Timshel avatar Jul 17 '24 14:07 Timshel

do you have a recommendation for the dropdown width ?

360 pixels

americanrefugee avatar Jul 18 '24 08:07 americanrefugee

@americanrefugee page look look like snapshots/settings/preferences-user-settings-tab.spec.ts Set the 360px using min-width to prevent wrapping if the application language description is wider.

Timshel avatar Jul 18 '24 13:07 Timshel

Hey, I suppose someone need to review the code ? :)

Timshel avatar Jul 22 '24 18:07 Timshel

@daniellekirkwood given @americanrefugee is unavailable how can we progress on this?

t3chguy avatar Jul 22 '24 18:07 t3chguy

Any news ?

Timshel avatar Aug 24 '24 13:08 Timshel

Any news ?

LGTM. Is there anything else do you need from me atm?

americanrefugee avatar Aug 26 '24 08:08 americanrefugee

Any news ?

LGTM. Is there anything else do you need from me atm?

Can you approve the PR ? Thanks :)

florianduros avatar Aug 26 '24 08:08 florianduros

@Timshel Can you merge develop in the PR in order to solve the conflict and make it up to date ? I'll review it after. Thanks!

florianduros avatar Aug 26 '24 08:08 florianduros

I rebased but the Playwright tests should fail since I'm unable to run those (and update the snapshot); it fails with :

Test timeout of 30000ms exceeded while setting up "user".

I tested on develop too and had the same error.

Timshel avatar Aug 26 '24 09:08 Timshel

I rebased but the Playwright tests should fail since I'm unable to run those (and update the snapshot); it fails with :

Test timeout of 30000ms exceeded while setting up "user".

I tested on develop too and had the same error.

Why are you unable to run the playwright tests?

florianduros avatar Aug 26 '24 09:08 florianduros

It's failing in:

       at ../element-web-test.ts:230

      228 |     user: async ({ pageWithCredentials: page, credentials }, use) => {
      229 |         await page.goto("/");
    > 230 |         await page.waitForSelector(".mx_MatrixChat", { timeout: 30000 });

Raised the timeout to 60000 but same error.

Edit: I'm running just the preference test with

yarn run test:playwright playwright/e2e/settings/preferences-user-settings-tab.spec.ts

Timshel avatar Aug 26 '24 09:08 Timshel

I think you have this error because the local instance of synapse used by the tests is not here. Normally, it's launched automatically when you launch our playwright tests.

Can you reinstall playwright and also put all the output of yarn run test:playwright playwright/e2e/settings/preferences-user-settings-tab.spec.ts

florianduros avatar Aug 26 '24 09:08 florianduros

The synapse instance appears to be running, the output from the run : https://gist.github.com/Timshel/a4ee0cdac04f7ded5fb4098cb4f74597 I had a quick look and homeserver-stdout.log / homeserver-stderr.log does not appear to contain any errors.

I wiped the node_modules and use yarn cache clean before installing again but no change.

Edit: and I pulled the latest image with docker pull ghcr.io/element-hq/synapse:develop.

Timshel avatar Aug 26 '24 10:08 Timshel

Do you have an ED/EW running when you launch the playwright tests?

  • If no => start it
  • If yes => stop it

(seems weird but trust me :p)

florianduros avatar Aug 26 '24 11:08 florianduros

Not sure what you mean by:

  • ed: editor ?
  • ew: element web ?

Kept only a shell open to run the tests and no change.

Unless you want to debug the issue, might be easier, if you don't mind, if you commit the updated snapshot ? (you should have commit right to the feature branch of the PR).

Sorry to ask it but for now I'm not working on any other modification so I would prefer not to spend the time to dig the issue (I'm using Playwright in another project which control some docker container without any issue).

Edit: ED: guessing it meant Element Desktop ?

Timshel avatar Aug 26 '24 11:08 Timshel

Ho sorry, EW => Element Web. I'll update the snapshots then after the review

florianduros avatar Aug 26 '24 11:08 florianduros

I had missed the errors from Typescript Syntax Check fixing those now.

Timshel avatar Aug 26 '24 11:08 Timshel