1514791 - Opening a container tab opens two tabs: a normal one and a contained one

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Marco Castelluccio [:marco]]

STR:
1) Long press on the '+' button
2) Select a container, e.g. "Work"

Only one new tab should be opened, in the "Work" container.

Instead, two tabs are opened. One in the default container, one in the "Work" container.

Comment 1

[Darkspirit]

mozregression --good 2018-12-10 --bad 2018-12-17
> 5:34.95 INFO: Last good revision: f6eebcc14c22762f521fb567a9588963b8720592
> 5:34.95 INFO: First bad revision: 61570c16c2d564c24fab36713fb169c4144453e9
> 5:34.95 INFO: Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f6eebcc14c22762f521fb567a9588963b8720592&tochange=61570c16c2d564c24fab36713fb169c4144453e9

> 61570c16c2d5	Olli Pettay — Bug 1512259 - No need to special case <button> in marionette, r=ato
> eb4344bead3e	Olli Pettay — Bug 1511388, ensure datetime reset button is the target for the click, r=timdream
> 7029e8b4d7d0	Olli Pettay — Bug 1498383 - clear.py relies on old Gecko's <button> hit testing, r=ato
> 36b949bef798	Olli Pettay — Bug 1089326, make <button> hit testing similar to other elements which may have some content, and for click target find the common (interactive) ancestor, r=masayuki

Comment 2

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

I can't reproduce this. Anything special one needs to do?

Comment 3

[Marco Castelluccio [:marco]]

The only two things I can think of that might be related:
- I'm on Linux with titlebar disabled;
- I have the "Firefox Multi-Account Containers" extension.

Comment 4

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

oh, now I managed to reproduce. Don't yet know what I did differently.

Comment 5

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Ah, this could be actually a bug in XUL menus, in https://searchfox.org/mozilla-central/source/layout/xul/nsMenuFrame.cpp

Comment 7

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Oh, so this happens only when long press and move mouse and release. Makes very much sense, since the click target is then the common ancestor.

Comment 8

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: xul_menu_click_target_approach_3.diff

I'm trying to have at least a temporary solution before Christmas.
Using the "interactive content" check, we don't end up creating click event when mousedown happens on the main UI area and mouseup on the menu.

It is possible that one could also just modify gClickAndHoldListenersOnElement 
https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/browser/base/content/browser.js#416
but didn't find any too nice solutions yet.

crossing fingers tryserver likes this approach ;)
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=874872fec1757ac4d45241c08423fdb762083830

no tests yet, sorry.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9032851 [details] [diff] [review]
xul_menu_click_target_approach_3.diff

Well, if IsInteractiveHTMLContent() will keep returning true even if it's a XUL element's, I think that "HTML" should be dropped from its name later.

I tested similar thing with <select> element in my test suite:
https://d-toybox.com/studio/lib/pointing_device_event_viewer.html
i.e., mousedown at arrow button of <select>, move on the popup, then, mouseup.

On Gecko, click event is fired on the <option> element rather than <select> element. On Chrome, click event is not fired. I guess that Chrome's behavior is what most web developers expect.

Perhaps, the reason is, mousedown widget and mouseup widget are different. Cannot you check the widget difference? If not possible, r+ if the tryserver will be all green.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, the <select> element's case is just invalid because the mousedown content should be <select> rather than <option>. So, not dispatching click event on <option> is valid behavior. I reproduce it with 65 Beta. So, not a regression of bug 1089326, though.

Comment 11

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #9)
> Comment on attachment 9032851 [details] [diff] [review]
> xul_menu_click_target_approach_3.diff
> 
> Well, if IsInteractiveHTMLContent() will keep returning true even if it's a
> XUL element's, I think that "HTML" should be dropped from its name later.
> 
Sure but I'm trying to find a better solution. Just want the have the UI broken over the Christmas.


<select> on the web pages is different, since the popup is shown in the parent process, and <select> element is in child process, at least right now.
https://bugzilla.mozilla.org/show_bug.cgi?id=1421229#c63 may change that

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/f256f0e01e82
don't generate click if path from mousedown.target to mouseup.target contains a menupopup - mark menupopup interactive content for now, r=masayuki

Comment 13

[Raul Gurzau (:RaulG)]

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