Gerrit

May I ask what browser you are using ? I can not reproduce this in chrome.

can reproduce on safari, actually this is an existing problem with safari... the multiple comment bug was fixed, but the select to comment is still broken

Tao, can you have another look? Is this still a problem? Just trying to get the issue triaged properly.

This is definitely a problem using Gerrit 3.1.0 in Safari 13.0.3 under Mac OS 10.14.

I am working on this now, will send a fix as soon as i can.

https://bugs.webkit.org/show_bug.cgi?id=163921

there seems to be some issue with safari on `getSelection` on shadow DOMs. And we are relying on that to make the select to copy to work.

and `getSelection` is not working correctly on shadowRoot for safari which makes it hard to fix this for safari.

As mentioned in this spec: https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot/getSelection but the support of `getSelection` on shadowRoot instances is missing in safari.

All I know, is that selection worked fine in the PolyGerrit UI in version 2.15, and that it doesn't work in 3.1.

Yes @cozmic72, we upgraded from polymer 1 to polymer 2 and one of the biggest changes is the shadow DOM. And the issue with safari right now is the support on getting selection within shadow DOM. I will see if we can have any workaround for it.

[Empty comment from Monorail migration]

[Empty comment from Monorail migration]

[Empty comment from Monorail migration]

There seems to be a polyfill for this issue: https://github.com/GoogleChromeLabs/shadow-selection-polyfill

Though the solution is not perfect, is this something that you can consider as a way to patch the issue on Safari? It seems as though this issue is a pretty sizable regression in functionality for Gerrit on a subset of users.

[Empty comment from Monorail migration]

Any update on this issue?

I think the best option would be to depend on https://github.com/GoogleChromeLabs/shadow-selection-polyfill

Let's investigate how hard this would be.

[Empty comment from Monorail migration]

Fix for stable-3.1:
https://gerrit-review.googlesource.com/c/gerrit/+/293242

Fix for stable-3.2:
https://gerrit-review.googlesource.com/c/gerrit/+/293183

The approach is the one that you guys have already identified.

According to your definitions of priorities, this should be a P1
(see https://www.gerritcodereview.com/support.html#response-time-and-slo)

For a user on Safari (20% of our user-base) this represents a regression of the code-review feature, blocking the migration from v3.0 (EOL) and v3.1.
As we don't support v3.0 anymore, it becomes urgent to fix the issue in our supported releases.

We can also, of course, declare Gerrit not compatible with Safari in v3.1, but that would cut 20% of our user base off the platform, which isn't something we are willing to do.

Reviews and feedback is more than welcome :-)

Luca.

The following revision refers to this bug:
  https://gerrit.googlesource.com/gerrit/+/96ccc2388a4c6038e3c49e52c6031fda959ecd15

commit 96ccc2388a4c6038e3c49e52c6031fda959ecd15
Author: Luca Milanesio <luca.milanesio@gmail.com>
Date: Mon Jan 11 14:08:05 2021

Add shadow-selection-polyfill

The selection on shadow-DOM is not implemented
in Safari and the document.getSelection() fallback
is not suitable for use.

The GoogleChromeLabs provided a library to solve
the problem: shadow-selection-polyfill.

What the library does is to manage the selection
change events and calculate and cache the results
for the shadow root elements, creating a custom
event '-shadow-selectionchange' once the selection
has been completed.

There is one gotcha that makes things slightly
more complicated: what the library provides is a Range
and not a Selection object. There are a couple of
adjustments needed in gr-diff and gr-diff-highlight
to use the range directly instead of getting it
from the selection.

NOTE: Even though the shadow-selection-polyfill could
also manage Chrome and Firefox, keep the existing
logic to avoid any possible regression, which is not
desireable on a stable branch.

On stable-3.1, because of the problems related to the inclusion
of the dependency via Bower, shadow.js has been removed
from its exports and used as pure JS file included in the
gr-diff.html.

This change needs would not be applied to stable-3.2 where,
thanks to the npm package mangement, it would be consumed
from the NPM registry directly.

Bug: https://crbug.com/gerrit/11811
Change-Id: I41c4e94343010972c8a9f0f1ba3a059ca7af5292

[add] https://gerrit.googlesource.com/gerrit/+/96ccc2388a4c6038e3c49e52c6031fda959ecd15/polygerrit-ui/app/scripts/shadow.js
[modify] https://gerrit.googlesource.com/gerrit/+/96ccc2388a4c6038e3c49e52c6031fda959ecd15/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
[modify] https://gerrit.googlesource.com/gerrit/+/96ccc2388a4c6038e3c49e52c6031fda959ecd15/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
[modify] https://gerrit.googlesource.com/gerrit/+/96ccc2388a4c6038e3c49e52c6031fda959ecd15/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js

The following revision refers to this bug:
  https://gerrit.googlesource.com/gerrit/+/40f0df7e0d9cac2ddcba45c19fcbf96566a3b690

commit 40f0df7e0d9cac2ddcba45c19fcbf96566a3b690
Author: David Ostrovsky <david@ostrovsky.org>
Date: Wed Jan 13 18:00:40 2021

Add shadow-selection-polyfill

This is a backport of change 293242 to stable-3.2 branch and to
consume shadow-selection-polyfill module with npm.

Bug: https://crbug.com/gerrit/11811
Change-Id: Iaf62137e48b7d119be3d8be96ad03a7f316a5e69

[modify] https://gerrit.googlesource.com/gerrit/+/40f0df7e0d9cac2ddcba45c19fcbf96566a3b690/Documentation/licenses.txt
[modify] https://gerrit.googlesource.com/gerrit/+/40f0df7e0d9cac2ddcba45c19fcbf96566a3b690/polygerrit-ui/app/yarn.lock
[modify] https://gerrit.googlesource.com/gerrit/+/40f0df7e0d9cac2ddcba45c19fcbf96566a3b690/polygerrit-ui/app/package.json
[modify] https://gerrit.googlesource.com/gerrit/+/40f0df7e0d9cac2ddcba45c19fcbf96566a3b690/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
[modify] https://gerrit.googlesource.com/gerrit/+/40f0df7e0d9cac2ddcba45c19fcbf96566a3b690/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
[modify] https://gerrit.googlesource.com/gerrit/+/40f0df7e0d9cac2ddcba45c19fcbf96566a3b690/Documentation/js_licenses.txt
[modify] https://gerrit.googlesource.com/gerrit/+/40f0df7e0d9cac2ddcba45c19fcbf96566a3b690/polygerrit-ui/app/node_modules_licenses/licenses.ts

[Empty comment from Monorail migration]

[Empty comment from Monorail migration]

[Empty comment from Monorail migration]

[Empty comment from Monorail migration]

Seems like adding that polyfil seems to have made things worse, when trying to copy any text on https://gerrit-review.googlesource.com/c/gerrit/+/293183/7/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js and even to comment, it just doesn't work. (Regression being when i try to copy it selects everything).

I'm wrong this bug seems to happen only on master, gerrithub works fine for me.

[Empty comment from Monorail migration]

Edits were made to reflect the following in Monorail: auto-CCs.