Closed Bug 1716883 Opened 7 months ago Closed 6 months ago

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

Categories

(Toolkit :: General, defect, P3)

x86
Windows 10
defect

Tracking

()

RESOLVED FIXED
92 Branch
Tracking Status
firefox-esr91 95+ fixed
firefox92 --- fixed

People

(Reporter: saschanaz, Assigned: Gijs)

References

Details

Attachments

(2 files)

Attached file bug-middle-click.html
  1. Make sure general.autoScroll is turned on (by default on Windows)
  2. Open the attachment
  3. Middle-click somewhere inside the upper bird and see a new tab opens
  4. 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.

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.

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

Component: DOM: Events → Panning and Zooming
Summary: Middle-clicking on SVG <use> wrapped by <a> does not fire a click event → Middle-clicking on SVG <use> wrapped by <a> unexpectedly starts autoscroll

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

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?

Severity: -- → S3
Priority: -- → P3

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)

Component: Panning and Zooming → General
Flags: needinfo?(gijskruitbosch+bugs)
Product: Core → Toolkit
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)

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.

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/57d00cc40a56
fix autoscroll behaviour over various SVG elements, r=masayuki

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

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/32025eb4c69c
fix autoscroll behaviour over various SVG elements, r=masayuki
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 92 Branch

[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.

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

Flags: needinfo?(gijskruitbosch+bugs)

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
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #9231441 - Flags: approval-mozilla-esr91?

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.

Attachment #9231441 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
You need to log in before you can comment on or make changes to this bug.