Remove XUL mousethrough attribute
Categories
(Core :: XUL, task, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox72 | --- | fixed |
People
(Reporter: ntim, Assigned: ntim)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
The mousethrough attribute takes 2 values: always and never. See: https://developer.mozilla.org/en-US/docs/Archive/Mozilla/XUL/Attribute/mousethrough
After some extensive testing and lots of confusion playing around with the mousethrough attribute, here's what I got of it:
mousethrough=alwaysis similar topointer-events: nonemousethrough=neveris similar to explicitly settingpointer-events: auto(since this overrides the parent pointer-events setting)- Not setting
mousethroughis equivalent to not settingpointer-eventsorpointer-events: unset - The code behind
mousethroughonly applies to XUL boxes: https://searchfox.org/mozilla-central/rev/492214c05cde6e6db5feff9465ece4920400acc3/layout/painting/nsDisplayList.cpp#3344 meaning any HTML element will still be clickable. - nsButtonBoxFrame and nsTitlebarFrame ignore the mousethrough attribute, and are always clickable, see: https://searchfox.org/mozilla-central/search?q=AddStateBits(NS_FRAME_MOUSE_THROUGH_NEVER)
I think something like this emulates the current behaviour:
xul|*[mousethrough=always] {
pointer-events: none;
}
xul|*[mousethrough=never],
html|*,
xul|titlebar, /* nsTitlebarFrame */
/* elements that use nsButtonFrame */
xul|button:not([type=menu-button]):not([type=menu]),
xul|toolbarbutton:not([type=menu-button]):not([type=menu]),
xul|thumb,
xul|checkbox,
xul|radio,
xul|treecolpicker {
pointer-events: auto;
}
Usages:
https://searchfox.org/mozilla-central/search?q=mousethrough%3D&case=false®exp=false&path=
https://searchfox.org/mozilla-central/search?q=.setAttribute(%22mousethrough%22&case=false®exp=false&path=
Given how random and undocumented this behavior is (and I suspect not all of it is needed by current usages), and the relatively few usages, I suggest:
- Rewriting current usages (using a class or by setting pointer-events explicitly where needed)
- Rewriting https://searchfox.org/mozilla-central/rev/492214c05cde6e6db5feff9465ece4920400acc3/layout/xul/nsMenuPopupFrame.cpp#284 to check for something else (pointer-events or an attribute)
- Remove the platform code for
mousethrough
|
Assignee | |
Comment 1•5 years ago
|
||
|
|
Assignee | |
Comment 2•5 years ago
|
||
Depends on D53347
|
|
Assignee | |
Comment 3•5 years ago
|
||
Depends on D53348
|
|
Assignee | |
Comment 4•5 years ago
•
|
||
|
|
Assignee | |
Comment 5•5 years ago
|
||
CC'ing some TB folks as I see usages there: https://searchfox.org/comm-central/search?q=mousethrough&case=false®exp=false&path=
|
|
Assignee | |
Comment 6•5 years ago
|
||
While investigating the browser_stayopenmenu.js failure from the try push, I think I found a bug involving shadow parts and pointer-events.
It looks like the failure is due to this rule added by the second patch in combination with the first patch (this doesn't reproduce without the first patch):
menupopup::part(drop-indicator) {
[...]
pointer-events: none;
}
This causes the whole bookmarks popup (which you can drag out from the customize mode) to act like it has pointer-events: none.
If I remove pointer-events: none and re-init the menupopup frame (by toggling display: none on it for instance), then the popup works fine again. We do want to apply pointer-events: none on the drop-indicator however.
Emilio, do you have time to look into this ?
|
||
Comment 7•5 years ago
|
||
Can you give me concrete STR? If so, sure, I can take a quick look, I guess, but I need to know:
- Which patches do I need to apply? Are just the first and second enough? Or all of them?
- How do I repro the issue? I see that the relevant part in the bookmark popup is display: none by default, and it seems to be inside the popup. I have no idea how to show the popup in customize mode...
|
|
Assignee | |
Comment 8•5 years ago
•
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)
Can you give me concrete STR? If so, sure, I can take a quick look, I guess, but I need to know:
Thanks! Sorry for the lack of details. My bad :(
- Which patches do I need to apply? Are just the first and second enough? Or all of them?
First+second should be enough, but you can also download the build from the try push
- How do I repro the issue? I see that the relevant part in the bookmark popup is display: none by default, and it seems to be inside the popup.
STR:
- Enter the customization mode (☰ > Customize)
- Drag "Bookmarks Menu" to the toolbar
- Exit the customization mode
- Open the Bookmarks Menu
ER:
- Hovering over the menu items should have a hover effect or open the submenus (for folders)
AR:
- Hovering the menu items does nothing
This seems to be caused by the CSS rule I mentioned in comment 6.
|
|
||
Comment 9•5 years ago
|
||
What I see is that in customize mode the menupopup gets style="pointer-events: none". Quite possibly from here.
I have no idea what that code is trying to accomplish, but given we only consider the value of the pointer-events property at the time of widget creation (D53347 doesn't invalidate anything when the style changes), the behavior makes sense to me.
Or am I missing something?
|
|
||
Comment 10•5 years ago
|
||
In terms of fixing this, we could:
- Implement invalidation for that. That may be easier or harder, it seems like it is right now only looked at widget creation time, which seems unfortunate. For gtk it should be fixable, not sure for windows and mac.
- Avoid setting
pointer-events: noneon the bookmarks menupopup itself? It seems it's set during the transition that is enabled on windows and mac, to avoid getting flickering on the items probably? If so, setting the property on the content of the menupopup rather than the menupopup itself may be a better short-term fix.
|
|
Assignee | |
Comment 11•5 years ago
|
||
|
|
Assignee | |
Comment 12•5 years ago
•
|
||
Thanks! Latest patch implements invalidation for pointer-events: none; on nsMenuPopupFrame.
Ongoing try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74d0aa1f38a09d0113769f7c0519faa2ebbb2180
| Comment hidden (obsolete) |
| Comment hidden (obsolete) |
| Comment hidden (obsolete) |
|
|
Assignee | |
Comment 16•5 years ago
•
|
||
| Comment hidden (obsolete) |
|
||
Updated•5 years ago
|
| Comment hidden (obsolete) |
|
|
Assignee | |
Comment 19•5 years ago
|
||
|
|
Assignee | |
Comment 20•5 years ago
•
|
||
Hi Emilio,
I'm encountering an issue with the last patch where having the bookmarks menu in the toolbar causes the whole browser to be unclickable.
STR:
- Download the try build from comment 19 on Linux/Windows (or apply all 4 patches)
- Make sure "Bookmarks Menu" is in the toolbar (by following the steps from comment 8)
AR:
Whole browser window doesn't react to any click
ER:
Browser window should react to clicks
Note that turning off manually pointer-events: none from #BMB_bookmarksPopup using the browser toolbox fixes the issue.
Re-adding pointer-events: none on #BMB_bookmarksPopup and then forcing frame reconstruction (e.g by toggling display: none) on the popup makes the issue happen again (edit: this only applies on Linux).
Emilio, do you have an idea what could be happening here ? (note that latest revision has null-checks as you suggested)
|
|
||
Comment 21•5 years ago
|
||
No, but the way I'd go for this is stepping through the pointer-events setting with rr (into gtk and such).
I can try to do it, but I won't have time until friday at least, I think.
|
|
Assignee | |
Comment 22•5 years ago
•
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #21)
No, but the way I'd go for this is stepping through the pointer-events setting with rr (into gtk and such).
I can try to do it, but I won't have time until friday at least, I think.
Thanks!
Rebased on top of bug 1597160: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81adc9f7f3caaf8c9128b3346b01fc563cdbefce
|
|
Assignee | |
Comment 23•5 years ago
|
||
I think I fixed the issue mentioned in comment 20, since I can't reproduce on Linux anymore. I'll clear the needinfo when I can confirm this is fixed on Windows as well.
Turns out checking against mMouseTransparent was unreliable, and it was better to check against aOldStyle->StyleUI->[...].
|
|
Assignee | |
Comment 24•5 years ago
|
||
Tentative fix for the issue I was seeing (both Win and Linux): https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9d02c58d6f476689f6b4b559c70ab9328012842
|
|
Assignee | |
Comment 25•5 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #24)
Tentative fix for the issue I was seeing (both Win and Linux): https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9d02c58d6f476689f6b4b559c70ab9328012842
I can confirm this fixes the issues I've been seeing on Windows and Linux, I'll land this as soon as the trees reopen and the try results come in.
|
|
||
Comment 26•5 years ago
|
||
I'd run that through :tnikkel. I suspect other callers of GetWidget() may be doing the wrong thing too for menupopup?
|
||
Comment 27•5 years ago
|
||
|
||
Comment 28•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/271dfeed518a
https://hg.mozilla.org/mozilla-central/rev/eea63573d9d7
https://hg.mozilla.org/mozilla-central/rev/2e9f758bf8db
https://hg.mozilla.org/mozilla-central/rev/64b604856d79
|
|
Assignee | |
Updated•5 years ago
|
Description
•