Page MenuHomePhabricator

"Press c to comment" is placed incorrectly when using Firefox 126 and 128 on macOS
Closed, ResolvedPublicBUG REPORT

Assigned To
Authored By
kostajh
Jun 10 2024, 8:48 AM
Referenced Files
F55163446: gerrit-firefox.gif
Jun 10 2024, 9:12 AM
F55163441: gerrit.gif
Jun 10 2024, 9:12 AM
Restricted File
Jun 10 2024, 8:57 AM
F55162841: image.png
Jun 10 2024, 8:53 AM
F55162741: image.png
Jun 10 2024, 8:50 AM

Description

Upstream issue https://issues.gerritcodereview.com/issues/346174905


Steps to replicate the issue (include links if applicable):

  • Use Firefox 126 (stable) or Firefox 128 (nightly)
  • Open a Gerrit patch
  • Select some text on *file* in a patch

What happens?:

The "press c to comment" button is either at the bottom of the page, or below the viewport, and thus looks like it is not present at all.

Browser console messages (not relevant):

10:40:36.522
Some cookies are misusing the recommended “SameSite“ attribute 5
10:40:37.253 downloadable font: rejected by sanitizer (font-family: "Roboto" style:normal weight:400 stretch:100 src index:0) source: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GlobalBlocking/fonts/roboto/Roboto-Regular.ttf
10:40:37.345 Plugin 'wm-checks-api' was trying to register twice as a Checks UI provider. Ignored. gr-app.js:10963:10943
10:40:37.346 Plugin 'wm-checks-api' was trying to register twice as a Checks UI provider. Ignored. gr-app.js:10963:10943
10:40:37.346 Plugin 'wm-patch-demo' was trying to register twice as a Checks UI provider. Ignored. gr-app.js:10963:10943
10:40:37.353 Plugin 'wm-checks-api' was trying to register twice as a Checks UI provider. Ignored. gr-app.js:10963:10943
10:40:37.353 Plugin 'wm-patch-demo' was trying to register twice as a Checks UI provider. Ignored. gr-app.js:10963:10943
10:40:37.353 Plugin 'wm-pcc' was trying to register twice as a Checks UI provider. Ignored. gr-app.js:10963:10943
10:40:37.362 downloadable font: rejected by sanitizer (font-family: "Roboto" style:normal weight:500 stretch:100 src index:0) source: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GlobalBlocking/fonts/roboto/Roboto-Medium.ttf
10:40:37.934 downloadable font: rejected by sanitizer (font-family: "Roboto Mono" style:normal weight:400 stretch:100 src index:0) source: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GlobalBlocking/fonts/robotomono/RobotoMono-Regular.ttf
10:40:42.267 downloadable font: rejected by sanitizer (font-family: "Roboto Mono" style:normal weight:700 stretch:100 src index:0) source: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GlobalBlocking/fonts/robotomono/RobotoMono-Bold.ttf

What should have happened instead?:

I should see a dialog for adding an inline comment next to the line or region that I've selected.

Other information (browser name/version, screenshots, etc.):

Firefox 126 or Firefox 128 on macOS

Note that commenting on commit messages in a patch works, but the "Add comment" button is placed in an odd location

image.png (1×3 px, 231 KB)

Event Timeline

kostajh updated the task description. (Show Details)
kostajh added a subscriber: hashar.

so, it looks like the element is added to the page, but it's not visible in the viewport. If I press "c", then the comment box appears.

so, it looks like the element is added to the page, but it's not visible in the viewport. If I press "c", then the comment box appears.

Indeed, there it is, you have to scroll down to the very bottom of the page to find it.

image.png (3×5 px, 992 KB)

I see the same behavior on upstream Gerrit, e.g. when trying to comment on https://gerrit-review.googlesource.com/c/k8s-gerrit/+/409747/23/container-images/gerrit-init/tools/gerrit-initializer/initializer/helpers/git.py#55

{F55162991}

kostajh renamed this task from Inline commenting doesn't work on Gerrit 3.9 with Firefox on macOS to "Press c to comment" is placed incorreclty when using Firefox 126 and 128 on macOS.Jun 10 2024, 8:58 AM
kostajh updated the task description. (Show Details)

With Firefox 115:

I can confirm the Popup shows up at the wrong position. The element <gr-selection-action-box> is after the element commenting the diff and its top position move it way below. If I manually change the css position from absolute to fixed it is a bit better but still off.

If I press C, the comment window shows up at the proper position.

With Chromium 124, the popup is at the proper position. I have to press C to insert a comment for the selected text.

Same behavior on upstream instance which runs 3.10.0-rc7 ( https://gerrit-review.googlesource.com )

Here's what it looks like on Chrome:

gerrit.gif (1×1 px, 1 MB)

Compared with Firefox:

gerrit-firefox.gif (1×1 px, 3 MB)

Yes the CSS positioning is off somehow. The element has position: absolute but under Chrome that is relative to the top of the diff while on Firefox that is relative to the bottom of the diff element. I don't have any good candidate nor could I find a way to fix the CSS through the inspector. I am afraid I will have to bisect it.

At a quick glance at polygerrit-ui/app/embed/diff/ they have changed the diff view and moved from a table layout toward a grid one. Firefox must have some slightly different behavior.

I get it fixed by adding style="display: block" to the parent element <gr-diff-element> but that not be the proper way to fix it.

Nemoralis renamed this task from "Press c to comment" is placed incorreclty when using Firefox 126 and 128 on macOS to "Press c to comment" is placed incorrectly when using Firefox 126 and 128 on macOS.Jun 10 2024, 12:43 PM

Change #1042976 had a related patch set uploaded (by Hashar; author: Hashar):

[operations/software/gerrit@deploy/wmf/stable-3.9] Update to a snapshot of Gerrit 3.9.6

https://gerrit.wikimedia.org/r/1042976

Change #1042976 merged by jenkins-bot:

[operations/software/gerrit@deploy/wmf/stable-3.9] Update to a snapshot of Gerrit 3.9.6

https://gerrit.wikimedia.org/r/1042976

Mentioned in SAL (#wikimedia-operations) [2024-06-13T10:08:03Z] <hashar@deploy1002> Started deploy [gerrit/gerrit@ee8252a]: Gerrit to snapshot version 3.9.5-21-g553ea468a1 on gerrit1003 # T367029 T367135

Mentioned in SAL (#wikimedia-operations) [2024-06-13T10:08:09Z] <hashar@deploy1002> Finished deploy [gerrit/gerrit@ee8252a]: Gerrit to snapshot version 3.9.5-21-g553ea468a1 on gerrit1003 # T367029 T367135 (duration: 00m 06s)

I have confirmed the fix on our Gerrit instance. Thank you @kostajh to have reported it!

Wonderful, thank you for fixing!

This issue is occurring again.

For some reason the fix didn't merge up, so it's not in 3.10. I've cherry picked the fix to the branch here https://gerrit-review.googlesource.com/c/gerrit/+/431657.

Change #1050595 had a related patch set uploaded (by Hashar; author: Hashar):

[operations/software/gerrit@deploy/wmf/stable-3.10] Update Gerrit 3.10 snapshot

https://gerrit.wikimedia.org/r/1050595

Change #1050595 merged by jenkins-bot:

[operations/software/gerrit@deploy/wmf/stable-3.10] Update Gerrit 3.10 snapshot

https://gerrit.wikimedia.org/r/1050595

Mentioned in SAL (#wikimedia-operations) [2024-06-28T12:55:19Z] <hashar@deploy1002> Started deploy [gerrit/gerrit@0db053e]: Upgrade Gerrit 3.10.0-32-gf77960412e to 3.10.0-71-gf6e9431fff - T367029 T341291

Mentioned in SAL (#wikimedia-operations) [2024-06-28T12:55:35Z] <hashar@deploy1002> Finished deploy [gerrit/gerrit@0db053e]: Upgrade Gerrit 3.10.0-32-gf77960412e to 3.10.0-71-gf6e9431fff - T367029 T341291 (duration: 00m 09s)

The merge commit was https://gerrit-review.googlesource.com/c/gerrit/+/429757 looking at the diff against the first parent https://gerrit-review.googlesource.com/c/gerrit/+/429757/-1..3 , the fix is not in polygerrit-ui/app/embed/diff/gr-diff/gr-diff-styles.ts so my guess is the merge was not properly done :/ Paladox cherry picked it and I have upgraded our instance. After I have cleared my browser cache, I can confirm the fix is applied.

The merge commit was https://gerrit-review.googlesource.com/c/gerrit/+/429757 looking at the diff against the first parent https://gerrit-review.googlesource.com/c/gerrit/+/429757/-1..3 , the fix is not in polygerrit-ui/app/embed/diff/gr-diff/gr-diff-styles.ts so my guess is the merge was not properly done :/ Paladox cherry picked it and I have upgraded our instance. After I have cleared my browser cache, I can confirm the fix is applied.

thanks again! :)