Closed Bug 1597120 Opened 2 years ago Closed 2 years ago

Remove XUL mousethrough attribute

Categories

(Core :: XUL, task, P3)

Product:
Core
Component:
XUL
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla72
Tracking Flags:
Tracking Status
firefox72 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1597120 - Make nsMenuPopupFrame read pointer-events: none; instead of mousethrough attribute.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1597120 - Replace usages of XUL mousethrough with CSS pointer-events.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1597120 - Remove platform support for XUL mousethrough attribute.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1597120 - Support dynamic changes to CSS pointer-events on nsMenuPopupFrame.
47 bytes, text/x-phabricator-request
Details | Review

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:

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&regexp=false&path=
https://searchfox.org/mozilla-central/search?q=.setAttribute(%22mousethrough%22&case=false&regexp=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:

Depends on: 1597131

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 ?

Flags: needinfo?(emilio)

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...
Flags: needinfo?(emilio) → needinfo?(ntim.bugs)

(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.

Flags: needinfo?(ntim.bugs) → needinfo?(emilio)

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?

Flags: needinfo?(emilio) → needinfo?(ntim.bugs)

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: none on 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.

Thanks! Latest patch implements invalidation for pointer-events: none; on nsMenuPopupFrame.

Ongoing try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74d0aa1f38a09d0113769f7c0519faa2ebbb2180

Flags: needinfo?(ntim.bugs)
Priority: -- → P3

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)

Flags: needinfo?(emilio)

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.

(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

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

Tentative fix for the issue I was seeing (both Win and Linux): https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9d02c58d6f476689f6b4b559c70ab9328012842

(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.

Flags: needinfo?(emilio)

I'd run that through :tnikkel. I suspect other callers of GetWidget() may be doing the wrong thing too for menupopup?

See Also: → 1598488
Depends on: 1598488
See Also: 1598488 → 
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/271dfeed518a
Make nsMenuPopupFrame read pointer-events: none; instead of mousethrough attribute. r=emilio
https://hg.mozilla.org/integration/autoland/rev/eea63573d9d7
Support dynamic changes to CSS pointer-events on nsMenuPopupFrame. r=karlt,jmathies,mstange,emilio
https://hg.mozilla.org/integration/autoland/rev/2e9f758bf8db
Replace usages of XUL mousethrough with CSS pointer-events. r=dao
https://hg.mozilla.org/integration/autoland/rev/64b604856d79
Remove platform support for XUL mousethrough attribute. r=emilio
Assignee: nobody → ntim.bugs
Regressions: 1606554
You need to log in before you can comment on or make changes to this bug.