Page MenuHomePhabricator
Create Task
Maniphest T367029

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

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

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)

Details

SubjectRepoBranchLines +/-
Update to a snapshot of Gerrit 3.9.6operations/software/gerritdeploy/wmf/stable-3.9+3 -3
Customize query in gerrit

Related Objects

Event Timeline

kostajh created this task.Mon, Jun 10, 8:48 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMon, Jun 10, 8:48 AM
kostajh updated the task description. (Show Details)Mon, Jun 10, 8:49 AM
kostajh updated the task description. (Show Details)
kostajh added a subscriber: hashar.
kostajh added a comment.Mon, Jun 10, 8:51 AM

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.

kostajh added a comment.EditedMon, Jun 10, 8:53 AM
In T367029#9874485, @kostajh wrote:

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.Mon, Jun 10, 8:58 AM
kostajh updated the task description. (Show Details)
hashar added a comment.EditedMon, Jun 10, 8:59 AM

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 )

kostajh added a comment.Mon, Jun 10, 9:12 AM

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)

hashar added a comment.Mon, Jun 10, 9:42 AM

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.

hashar added a comment.Mon, Jun 10, 11:49 AM

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.Mon, Jun 10, 12:43 PM
hashar added a project: Upstream.Mon, Jun 10, 1:06 PM
hashar updated the task description. (Show Details)
hashar mentioned this in T354887: Upgrade to Gerrit 3.9.Mon, Jun 10, 1:36 PM
hashar moved this task from Backlog to Patch proposed upstream on the Upstream board.Mon, Jun 10, 3:22 PM

I have proposed a patch Upstream: https://gerrit-review.googlesource.com/c/gerrit/+/429157 Fix code comment position with Firefox

hashar claimed this task.Mon, Jun 10, 3:22 PM
hashar moved this task from Patch proposed upstream to Patch merged upstream on the Upstream board.Tue, Jun 11, 1:04 PM
gerritbot added a comment.Thu, Jun 13, 9:46 AM

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

gerritbot added a project: Patch-For-Review.Thu, Jun 13, 9:46 AM
gerritbot added a comment.Thu, Jun 13, 10:00 AM

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

Stashbot added a comment.Thu, Jun 13, 10:08 AM

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

Stashbot mentioned this in T367135: "Collapse" link on add/edit reviewers screen is showing weird icons.Thu, Jun 13, 10:08 AM

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)

hashar closed this task as Resolved.Thu, Jun 13, 10:24 AM

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

kostajh added a comment.Thu, Jun 13, 10:30 AM

Wonderful, thank you for fixing!

Maintenance_bot removed a project: Patch-For-Review.Thu, Jun 13, 10:30 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL