1657992 - Middle click on session history item of Back/Forward button opens a unwanted tab

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: duplication tab from session history menu is broken

Reproducible: always

Steps To reproduce::

1. Navigate several site so that several session histories are made.
2. Long mouse press click on Back/Forward button to pop up the session history menu.
   e.g,

    ・session history 3
    □session history 2
    □session history 1
    □New Tab

3. Middle mouse click on a session history entry (e.g, session history 3)

Actual Results:
The "session history 3" is duplicated in a new tab as expected.
However,
if the popup from Back button in step2, "session history 2" is also duplicated in a next new tab.
if the popup from Forward button in step2, "session history 1" is also duplicated in a next new tab.

Expected Results:
Only "session history 3" should be duplicated in a new tab

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cf217b49d014efd5b82557b6bb9d85788852dcfd&tochange=f8e83f5b68e0e151df25cf6021fc3ab985a6a012

Comment 1

[Rob Wu [:robwu]]

I cannot reproduce this issue - only one tab is opened/duplicated as expected.

Are you able to reproduce this in a clean/default profile? If not, have you made any changes to Firefox (preferences or add-ons) that may cause this issue?

Comment 2

[Alice0775 White]

(In reply to Rob Wu [:robwu] from comment #1)

I cannot reproduce this issue - only one tab is opened/duplicated as expected.

Are you able to reproduce this in a clean/default profile? If not, have you made any changes to Firefox (preferences or add-ons) that may cause this issue?

I can reproduce on Nightly81.0a1(20200810213634) Windows10 with clean newly created profile.

screen capture: https://youtu.be/s4e9T1U3FkA

Comment 3

[Rob Wu [:robwu]]

Thanks, my copy of Nightly was too old (80.0a1 20200719090414).
I checked again and confirmed it in 81.0a1 20200811091731 on Linux.

STR:

1. Open a new tab.
2. Visit example.com
3. Long-press the back button to get the history items.
4. Middle-mouse-click on the previous history item

Expected:

• One tab.

Actual:

• Two tabs.

I'll investigate.

Comment 4

[Rob Wu [:robwu]]

I see why the bug is happening but I'm puzzled why bug 1640665 triggered the ultimate regression.
Even without bug 1640665, I can reproduce the issue (e.g. by briefly spinning the message loop or putting a breakpoint in the duplicateTabIn method and then following the STR from comment 3.
The only "reason" for not observing the reported bug until now is because a gBrowser.selectedTab = assignment somehow manages to avoid the real bug.

The real bug is that the history menu popup is a child of the #back-button / #forward-button element, and that clicks on that menu popup propagate upwards. As a result, the onclick="checkForMiddleClick(this, event)" handlers of both buttons get activated, and each of which open a new tab.
This was originally fixed in bug 415444 by adding an event.stopPropagation() call to the oncommand handler. The oncommand handler was originally not merely handling command events, but also middle-click events because bug 263942 used an eval-like method to run the oncommand handler with the event from the click event.
This hack was removed in bug 1523813, and since then the click event is no longer forwarded to the oncommand handler, and consequently the event.stopPropagation() call does not work for middle-click any more. This is the root cause of the bug, and to fix this bug we should ensure that handled middle-click events aren't handled again, e.g. by adding a stopPropagation() call in the middle-click handler.

Comment 5

[Rob Wu [:robwu]]

Attachment: Bug 1657992 - Handle middle-click on long-press backForwardMenu only once + tests

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c75bd47455a8
Handle middle-click on long-press backForwardMenu only once + tests r=dao

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c75bd47455a8

Comment 8

[Julien Cristau [:jcristau]]

Rob, should we uplift this?

Comment 9

[Rob Wu [:robwu]]

Comment on attachment 9169580 [details]
Bug 1657992 - Handle middle-click on long-press backForwardMenu only once + tests

Beta/Release Uplift Approval Request

• User impact if declined: Middle-click on the history menu item opens an undesired extra tab. Regression in 80.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Problem is well-understood, change is in code specific to the affected feature, and the patch includes a unit test for the functionality.
• String changes made/needed:

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 9169580 [details]
Bug 1657992 - Handle middle-click on long-press backForwardMenu only once + tests

approved for 80.0b8

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8fc9a02e24ba

Comment 12

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I successfully reproduced the issue on Firefox (2020-08-07) under macOS 10.15.6 by following the STR from Comment 0.

The issue is fixed on Firefox 80.0b8 and Nightly 81.0a1 (2020-08-17). Tests were performed on macOS 10.15.6, Windows 10 and Ubuntu 20.04.