STR:

1. Long-press the new tab button and hold.
2. Select a container from the menu and then release the mouse button.

Expected:
One container tab is created.

Actual:
Two tabs are created, a normal tab and a container tab.

Does not occur when right-clicking the new tab button to create a container tab.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=728f4a4d08f5b369ce5c7bea17a23cdfbb30f196&tochange=92ad9c748611fc9b5be547415a651f136a27346f

Regressed by Bug 1792110.

:sefeng, since you are the author of the regressor, bug 1792110, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Attached the simplified demo.

STR:

1. Hold and long press the box
2. Release mouse on the button

A click on the div will be logged to devtool, which is not the case before.

So this behaviour change is actually intentional, Chrome behaves the same. Apparently this breaks the new tab button this way because we used to not getting a click event on the new tab button but does now.

I think one way to fix this is to use mouseup event on these elements. The dropdown items could use stopPropagation() to prevent from the parent element receiving mouseup events.

What do you think Gijs?

(In reply to Sean Feng [:sefeng] from comment #4)

So this behaviour change is actually intentional, Chrome behaves the same. Apparently this breaks the new tab button this way because we used to not getting a click event on the new tab button but does now.

Can you provide more specifics about what events are happening where / what code is involved? From your comment, I am struggling to follow what element is receiving a click that didn't before (and why that causes us to open another tab). I cannot reproduce the issue myself on nightly (tested on macOS - only 1 tab opens), so it's hard to know what is going on where.

Switching events may be tricky because the click-and-hold stuff is implemented generically in gClickAndHoldListenersOnElement. Does (a variation of) this bug also affect the back/fwd button dropdowns which open the same way?

If the issue is that a click event from the menu item is bubbling up to the button that opened the popup, wouldn't a simpler fix be to call stopPropagation from the click handler on the menuitem, or for the button's handler to ignore events that came from inside its menupopup? Or even to move click handling for the menuitems into the same click handler? I don't really understand what we'd gain by changing the event we use. It's additionally baffling that I can't reproduce on macOS, so clearly something is different there (perhaps the native menus?).

Can you provide more specifics about what events are happening where / what code is involved? From your comment, I am struggling to follow what element is receiving a click that didn't before (and why that causes us to open another tab). I cannot reproduce the issue myself on nightly (tested on macOS - only 1 tab opens), so it's hard to know what is going on where.

If you try my attached testcase with STR, you'll see a click event is generated on the div, which is not the case before Bug 1792110. The explanation is that the mousedown happens on the div, and the mouseup happens on the button. Bug 1792110 updated the common ancestor algorithm, so we start to generate a click event for the div now. I guess you don't see it on Mac because these buttons have different DOM tree structures on Mac than Window/Linux. (perhaps the native menus as you said).

Switching events may be tricky because the click-and-hold stuff is implemented generically in gClickAndHoldListenersOnElement. Does (a variation of) this bug also affect the back/fwd button dropdowns which open the same way?

I tried back/fwd button dropdowns, it seemed fine for me.

If the issue is that a click event from the menu item is bubbling up to the button that opened the popup, wouldn't a simpler fix be to call stopPropagation from the click handler on the menuitem, or for the button's handler to ignore events that came from inside its menupopup? Or even to move click handling for the menuitems into the same click handler? I don't really understand what we'd gain by changing the event we use. It's additionally baffling that I can't reproduce on macOS, so clearly something is different there (perhaps the native menus?).

So if you check my updated testcase, you'll see that the menuitem button doesn't get a click event and only mouseup. So this is why I mentioned switching to mouseup earlier. It could be that I don't understand the structure of the new-tab-button correctly, and we do get a click event for the menuitem items, then I think stopPropagation() works. Can you provide me to where we handle the click events for the menuitems, and I can try?

(In reply to Sean Feng [:sefeng] from comment #6)

I tried back/fwd button dropdowns, it seemed fine for me.

No, it does not work as expected.
It appears that the command of the long-pressed button(Back, Forward) is also executed after navigates the selected entry to change depending on the position of the current and selected entry and the long-pressed button.

See Bug 1847067

So stopPropagation won't work because the menuitem doesn't get click event. menuitem uses mouseup event to trigger a command, so openNewUserContextTab can be called.

Only the new tab button is getting a click event in this case. Gijs, I attached a small sample patch which prevents the click event from handling by checking to see if the menu is opened. Do you think this is the right way to go? This patch fixes this bug and also Bug 1847067 as well.

If the menu is opened, the corresponding mouseup event that generates
this click event will be handled by the menuitem, so we don't
need to handle this click event.

(In reply to Sean Feng [:sefeng] from comment #9)

Created attachment 9347250 [details]
bug1846714.patch

So stopPropagation won't work because the menuitem doesn't get click event. menuitem uses mouseup event to trigger a command, so openNewUserContextTab can be called.

Only the new tab button is getting a click event in this case. Gijs, I attached a small sample patch which prevents the click event from handling by checking to see if the menu is opened. Do you think this is the right way to go? This patch fixes this bug and also Bug 1847067 as well.

Could we instead check if the click event originated in the subtree, instead of checking if the subtree is hidden? That feels like it'll be less fragile in terms of the timing of .hidden becoming true vs. the event firing.

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

[Tracking Requested - why for this release]: Long press mouse new tab and Back/Forward button is broken.

The patch landed in nightly and beta is affected.
:sefeng, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox117 to wontfix.

For more information, please visit BugBot documentation.

Comment on attachment 9347461 [details]
Bug 1846714 - Make the newtab button to not handle click event if the menu is open r=Gijs

Beta/Release Uplift Approval Request

• User impact if declined: Long press the new tab button to open a tab in container is broken
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Long press the new tab button

2. Wait for the container menu pops up
3. Move the cursor to one of the container item
4. Release the mouse

Only one tab should be opened.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change itself is trivial
• String changes made/needed:
• Is Android affected?: Unknown

Comment on attachment 9347461 [details]
Bug 1846714 - Make the newtab button to not handle click event if the menu is open r=Gijs

Approved for 117.0b8.

Reproduced the issue on Nightly 118.0a1 (20230811095324) and Beta 117.0b7 (20230813180142).
Verified the fix on Nightly 118.0a1 (20230815091726) and Beta 117.0b8 (20230815180041).
Setting the flags accordingly.