Closed
Bug 1514791
Opened 6 years ago
Closed 6 years ago
Opening a container tab opens two tabs: a normal one and a contained one
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox64 | --- | unaffected |
firefox65 | --- | unaffected |
firefox66 | --- | fixed |
People
(Reporter: marco, Assigned: smaug)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
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•6 years ago
|
||
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
status-firefox64:
--- → unaffected
status-firefox65:
--- → unaffected
status-firefox-esr60:
--- → unaffected
Flags: needinfo?(bugs)
Updated•6 years ago
|
Assignee | ||
Comment 2•6 years ago
|
||
I can't reproduce this. Anything special one needs to do?
Flags: needinfo?(mcastelluccio)
Reporter | ||
Comment 3•6 years ago
|
||
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.
Flags: needinfo?(mcastelluccio)
Assignee | ||
Comment 4•6 years ago
|
||
oh, now I managed to reproduce. Don't yet know what I did differently.
Assignee | ||
Updated•6 years ago
|
Component: DOM: Security → Tabbed Browser
Flags: needinfo?(bugs)
Product: Core → Firefox
Assignee | ||
Comment 5•6 years ago
|
||
Ah, this could be actually a bug in XUL menus, in https://searchfox.org/mozilla-central/source/layout/xul/nsMenuFrame.cpp
Assignee | ||
Comment 7•6 years ago
|
||
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.
Assignee | ||
Comment 8•6 years ago
|
||
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.
Assignee: nobody → bugs
Attachment #9032851 -
Flags: review?(masayuki)
Comment 9•6 years ago
|
||
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.
Attachment #9032851 -
Flags: review?(masayuki) → review+
Comment 10•6 years ago
|
||
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.
Assignee | ||
Comment 11•6 years ago
|
||
(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
Updated•6 years ago
|
Component: Tabbed Browser → XUL
Product: Firefox → Core
Comment 12•6 years ago
|
||
Pushed by rgurzau@mozilla.com:
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•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in
before you can comment on or make changes to this bug.
Description
•