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

gui: Replace JS select-on-click with CSS user-select (fixes #3868) #8544

Merged

Conversation

tomasz1986
Copy link
Contributor

Currently, a custom JS script is used to select the whole device ID on click. However, the current script isn't compatible with all browsers (and in IE in particular), making it impossible to select the ID in them at all. Additionally, the same functionality is already available in CSS with no such drawbacks, as the whole selection process is handled by the Web browser natively, which is lightweight and does not require custom code.

Thus, remove the currently used JS script completely, replacing it with a new CSS class that can be added to an element when required. If the browser does not support the CSS, the user can still select the element manually, which makes it safer than the current behaviour that can block the user from being able to select the element at all.

Signed-off-by: Tomasz Wilczyński twilczynski@naver.com

…#3868)

Currently, a custom JS script is used to select the whole device ID on
click. However, the current script isn't compatible with all browsers
(and in IE in particular), making it impossible to select the ID in them
at all. Additionally, the same functionality is already available in CSS
with no such drawbacks, as the whole selection process is handled by the
Web browser natively, which is lightweight and does not require custom
code.

Thus, remove the currently used JS script completely, replacing it with
a new CSS class that can be added to an element when required. If the
browser does not support the CSS, the user can still select the element
manually, which makes it safer than the current behaviour that can block
the user from being able to select the element at all.

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
@wweich
Copy link
Member

wweich commented Sep 16, 2022

I'm not aware that we use any kind of auto-prefix for CSS, so for this to work in any Safari version and IE 10+, we need vendor prefixes.

@tomasz1986
Copy link
Contributor Author

It doesn't seem that any prefixes are needed for the current version Safari when it comes to user-select: all specifically (see https://caniuse.com/mdn-css_properties_user-select_all). I could add webkit-user-select to support older versions of the browser though. Is it worth it?

As for IE, my idea here is just not to support it at all. The automatic selection is a non-essential feature, as the user can simply double click (or Ctrl+Click in IE) to select the whole string anyway. On the other hand, the current script actually prevents the IE user from any selection at all, which is much worse.

Basically, the clue of this PR is to support modern browsers while not breaking anything in the old ones.

Copy link
Member

@acolomb acolomb left a comment

Choose a reason for hiding this comment

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

Haven't tested myself, but the changes look OK and it's going in the right direction. Great work and thanks for finding that solution!

@bt90
Copy link
Contributor

bt90 commented Sep 16, 2022

Just my two cents but IE support should be best effort only. Microsoft is dropping support for it left, right and center. I don't see a reason to avoid making things better for the 99% of the userbase only to drag along a legacy browser.

@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Sep 16, 2022

It's still the only browser available by default on some editions of Windows (out of which Windows Server is probably the main culprit). On those systems, the browser is going to receive security updates until 2029-32, so while heavily outdated, it's probably much more secure than, for example, the similarly old and outdated WebView that's used on Android 4.x to display the Web GUI.

For the record, the commit here isn't doing anything special to support IE itself. It just moves the code to a more modern CSS solution. Fixing the IE-specific issue is only a byproduct of dropping the JS script.

@calmh calmh merged commit 5baf5fe into syncthing:main Sep 20, 2022
@tomasz1986 tomasz1986 deleted the tomasz86/gui/fix-select-on-click-in-ie11 branch September 21, 2022 19:11
@calmh calmh added this to the v1.22.0 milestone Sep 27, 2022
@acolomb
Copy link
Member

acolomb commented Oct 7, 2022

I just noticed a strange behavior with this change when using WebKit as renderer. It doesn't respect the new select-on-click behavior, but normally one can still select a "whole paragraph" by using triple-click. That however selects two additional line breaks (observed when copy-pasting the text) and looks quite disturbing, as seen here:

Screenshot from 2022-10-07 23-06-26

I tried replacing the <div> element with a proper paragraph <p>, but that doesn't change anything. Any idea what might be going wrong there?

Tested with my gnome-shell Syncthing icon extension, which uses WebKit, and with the GNOME Epiphany browser based on the same library.

@tomasz1986
Copy link
Contributor Author

I can reproduce using Otter Browser in Windows, which also uses the WebKit engine. This looks like a browser/engine bug, to be honest, but I will check if anything could be done to work around it.

@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Oct 8, 2022

I've given this a shot, but this seems to be how WebKit, at least in these browsers, applies double click selection. The same thing happens with all other elements, e.g.

image

I doubt anything can be done about it on the Syncthing side. The good thing is that even if you copy it with the line break, you can still paste it into Syncthing when adding a new device, and the ID will be recognised properly.

Edit:

I will maybe add -webkit-user-select: all; to the CSS, as this makes the automatic selection work (without selecting more than necessary) in those WebKit browsers that don't support just user-select yet.

@acolomb
Copy link
Member

acolomb commented Oct 8, 2022

Thanks @tomasz1986 for looking into this. I'd say adding the WebKit-specific attribute sounds like a good and easy fix.

tomasz1986 added a commit to tomasz1986/syncthing that referenced this pull request Oct 10, 2022
…thing#8544)

Some WebKit browsers select more than needed when using double click to
select device IDs, e.g. new lines and white space. This commit adds a
prefixed version of user-select in CSS in order to add support for those
browsers and allow them to select just device IDs automatically.

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
calmh pushed a commit that referenced this pull request Oct 10, 2022
… (#8597)

Some WebKit browsers select more than needed when using double click to
select device IDs, e.g. new lines and white space. This commit adds a
prefixed version of user-select in CSS in order to add support for those
browsers and allow them to select just device IDs automatically.

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
calmh added a commit to calmh/syncthing that referenced this pull request Oct 13, 2022
* main:
  lib/scanner: More sensible debug output (syncthing#8596)
  gui: Allow automatic device ID selection on WebKit browsers (ref syncthing#8544) (syncthing#8597)
  gui, man, authors: Update docs, translations, and contributors
calmh added a commit to imsodin/syncthing that referenced this pull request Nov 8, 2022
* main: (36 commits)
  lib/protocol: Ignore inode time when xattr&ownership is ignored (fixes syncthing#8654) (syncthing#8655)
  lib/fs: Try to remove read only Windows files (fixes syncthing#3744) (syncthing#8650)
  gui: Add copy to clipboard, share by email, and share by SMS buttons to device IDs (fixes syncthing#2771, ref syncthing#3868) (syncthing#7984)
  gui, man, authors: Update docs, translations, and contributors
  build: Add GitHub actions build for Windows (syncthing#8627)
  gui: Fix connection type icon width (fixes syncthing#8592) (syncthing#8644)
  gui: Adjust connection type icon size scaling and alignment (syncthing#8645)
  docker: Use healthcheck endpoint (syncthing#8640)
  lib/connections: Use adaptive write size for rate limited connections (fixes syncthing#8630) (syncthing#8631)
  gui: Mark devices that haven't connected for a long time (fixes syncthing#7703) (syncthing#8530)
  gui: Fix rescan interval when add encrypted folder with watch for changes enabled (fixes syncthing#8570) (syncthing#8571)
  gui: Always show Out of Sync Items for remote devices (syncthing#8632)
  lib/fs: Let xattr test avoid non-test attributes (fixes syncthing#8601) (syncthing#8628)
  build: Add GitHub actions build for Windows
  gui, man, authors: Update docs, translations, and contributors
  gui: Display folder and device count number (syncthing#8615)
  gui, man, authors: Update docs, translations, and contributors
  lib/model, lib/protocol: Fix file comparisons (fixes syncthing#8594) (syncthing#8603)
  lib/scanner: More sensible debug output (syncthing#8596)
  gui: Allow automatic device ID selection on WebKit browsers (ref syncthing#8544) (syncthing#8597)
  ...
tomasz1986 added a commit to tomasz1986/syncthing that referenced this pull request Aug 31, 2023
The CSS method to select device IDs on click was added in [1]. However,
it was later mistakenly overwritten by [2]. This commit fixes the
regression and also applies the same behaviour to the Edit Device modal
which was omitted in the original commit.

[1] 5baf5fe
[2] 5e384c9

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
calmh pushed a commit that referenced this pull request Aug 31, 2023
The CSS method to select device IDs on click was added in [1]. However,
it was later mistakenly overwritten by [2]. This commit fixes the
regression and also applies the same behaviour to the Edit Device modal
which was omitted in the original commit.

[1] 5baf5fe
[2] 5e384c9

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
calmh added a commit to calmh/syncthing that referenced this pull request Sep 1, 2023
* main:
  build: Update dependencies
  gui: Remove footer and move links to header (fixes syncthing#5607) (syncthing#9067)
  gui: Fix lastSeenDays error due to undefined deviceStats when adding new devices (ref syncthing#8730) (syncthing#9066)
  gui: Automatically select device ID on click (ref syncthing#8544) (syncthing#9065)
  gui: Prevent modifications when saving changes (fixes syncthing#9019) (syncthing#9063)
  gui: Show in GUI if limitBandwidthInLan is enabled (syncthing#9062)
  lib/upgrade: Enable HTTP/2 for upgrade checks (syncthing#9060)
  lib/discover: Enable HTTP/2 for global discovery requests (syncthing#9059)
  cmd/stdiscosrv: Streamline context handling
  cmd/stdiscosrv: Explicitly enable HTTP/2
calmh added a commit to calmh/syncthing that referenced this pull request Sep 1, 2023
* main: (121 commits)
  build: Update dependencies
  gui: Remove footer and move links to header (fixes syncthing#5607) (syncthing#9067)
  gui: Fix lastSeenDays error due to undefined deviceStats when adding new devices (ref syncthing#8730) (syncthing#9066)
  gui: Automatically select device ID on click (ref syncthing#8544) (syncthing#9065)
  gui: Prevent modifications when saving changes (fixes syncthing#9019) (syncthing#9063)
  gui: Show in GUI if limitBandwidthInLan is enabled (syncthing#9062)
  lib/upgrade: Enable HTTP/2 for upgrade checks (syncthing#9060)
  lib/discover: Enable HTTP/2 for global discovery requests (syncthing#9059)
  cmd/stdiscosrv: Streamline context handling
  cmd/stdiscosrv: Explicitly enable HTTP/2
  gui, man, authors: Update docs, translations, and contributors
  cmd/stdiscosrv: Separate HTTPS and replication certificates
  cmd/stdiscosrv: Use larger database settings
  cmd/stdiscosrv: Modernise TLS settings, remove excessive HTTP logging
  cmd/stdiscosrv: Serve compressed responses
  lib/connections: Allow IPv6 ULA in discovery announcements (fixes syncthing#7456) (syncthing#9048)
  lib/beacon: Check FlagRunning (syncthing#9051)
  all: Remove lib/util package (syncthing#9049)
  lib/model: Clean up index handler life cycle (fixes syncthing#9021) (syncthing#9038)
  lib/osutil, lib/upnp: Check FlagRunning (fixes syncthing#8767) (syncthing#9047)
  ...
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Oct 12, 2023
@syncthing syncthing locked and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants