Closed Bug 1657992 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

80 Branch
Desktop
Windows 10
defect

Tracking

()

VERIFIED FIXED
81 Branch
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox79 --- unaffected
firefox80 + verified
firefox81 --- verified

People

(Reporter: alice0775, Assigned: robwu)

References

(Regression)

Details

(Keywords: nightly-community, regression)

Attachments

(1 file)

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

Flags: needinfo?(dao+bmo)
Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(rob)

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?

Flags: needinfo?(rob) → needinfo?(alice0775)

(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

Flags: needinfo?(alice0775)

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.

Assignee: nobody → rob
Flags: needinfo?(dao+bmo)

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.

Status: NEW → ASSIGNED
Regressed by: 1523813
Pushed by rob@robwu.nl: https://hg.mozilla.org/integration/autoland/rev/c75bd47455a8 Handle middle-click on long-press backForwardMenu only once + tests r=dao
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch

Rob, should we uplift this?

Flags: needinfo?(rob)

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:
Attachment #9169580 - Flags: approval-mozilla-beta?
Flags: needinfo?(rob)

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

approved for 80.0b8

Attachment #9169580 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: