1716883 - Middle-clicking on SVG <use> wrapped by <a> unexpectedly starts autoscroll

Status: RESOLVED FIXED

(Toolkit :: General, defect, P3)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: bug-middle-click.html

0. Make sure general.autoScroll is turned on (by default on Windows)
1. Open the attachment
2. Middle-click somewhere inside the upper bird and see a new tab opens
3. Middle-click somewhere inside the lower bird and see a new tab opens

Expected: Each should open a new tab
Actual: Only the lower one opens a tab, and the upper one instead starts autoscrolling

This is affected by bug 1528289 (specifically the part 4 patch) where autoscrolling prevents click event, but it really shouldn't even start autoscrolling.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

How do you know middle click isn't firing? Or is it just that middle click isn't handled? That latter would be Firefox UI issue.

Comment 2

[Kagami Rosylight [:saschanaz] (they/them)]

Hmm, TIL middle click on <a> actually never fires a web-visible click event. (Ah yeah because auxclick.) Changing the component per mozbuild.

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

(It indeed does not fire auxclick because of autoscrolling, BTW.)

Comment 4

[Botond Ballo [:botond]]

The difference in behaviour between the top image and the bottom image hinges on whether isAutoscrollBlocker returns true or false for the event's target.

This code predates APZ and I'm not really familiar with it, but based on a quick look, it looks like isAutoscrollBlocker() walks the content tree to find an <a href>, and in the case of the first image, the target is inside the used SVG so the traversal never encounters the <a>.

So, I guess the traversal should be doing something more involved than node = node.parentNode to handle this case?

Comment 5

[Botond Ballo [:botond]]

Moving to Toolkit :: General, per discussion with Gijs about what to do with autoscroll bugs unrelated to APZ.

(ni?gijs, just in case you'd like to adjust the priority or ping someone specific, otherwise feel free to leave as P3/backlog)

Comment 6

[:Gijs (he/him)]

Attachment: Bug 1716883 - fix autoscroll behaviour over various SVG elements, r?masayuki

This avoids starting autoscroll on SVG links, and on HTML links that
contain SVG, by sharing the code that browser's ClickHandlerChild uses
to detect links into BrowserUtils, and using that from AutoScrollChild
to determine if the click happened on top of a link. It also adds a
test so we avoid regressing this in future.

When running the test, I noticed an error from ClickHandlerParent
when the browser for which we receive a click has gone away, and
fixed it by adding a nullcheck and early return.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57d00cc40a56
fix autoscroll behaviour over various SVG elements, r=masayuki

Comment 8

[Iulian Moraru]

Backed out for causing linting failures on browser_autoscroll_disabled_on_links.js

Push with failures

Failure log

Backout link

It also caused some bc failures on browser_autoscroll_disabled_on_editable_content.js
Failure log

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32025eb4c69c
fix autoscroll behaviour over various SVG elements, r=masayuki

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/32025eb4c69c

Comment 11

[Alice0775 White]

[Tracking Requested - why for this release]:
No longer open a product in new tab with middle mouse button click on a product image of https://jp.mercari.com/.

Please up lift to 91.esr if possible.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Gijs, did you want to nominate this for uplift? It grafts cleanly.

Comment 13

[:Gijs (he/him)]

Comment on attachment 9231441 [details]
Bug 1716883 - fix autoscroll behaviour over various SVG elements, r?masayuki

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Broken autoscroll / link behaviour for popular Japanese websites
• User impact if declined: See above
• Fix Landed on Version: 92
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Grafts cleanly, moves & reuses existing logic for link clicks in the autoscroll code to fix the bug & adds an automated test.
• String or UUID changes made by this patch: Nope

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9231441 [details]
Bug 1716883 - fix autoscroll behaviour over various SVG elements, r?masayuki

Fixes a broken site in the wild and has had lots of bake time w/ automated test. Approved for 91.4esr.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/164a707288b0