979587 - Australis: sometimes bookmarks sidebar opens with bookmarks menu

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

+++ This bug was initially created as a clone of Bug #973217 +++

This is definitely improved since bug 973217 landed, but I've still hit it twice in the last day or so and Gijs tells me he has too.

Comment 1

[:Gijs (he/him)]

Don't know how/why this is still broken. :-\

Comment 2

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #1)
> Don't know how/why this is still broken. :-\

Mike and I debugged this, because it's very reproducible on his machine (and not on mine).

Basically what seems to happen is the click event fires before the popupshown event, which means our pointerEvents disabling isn't working.

I have a cunning plan:

0) set style="pointer-events: none" on the popup;
1) remove that in popupshown, and do our current dance. Add an attribute to the popup indicating whether pointer-events should be disabled until the transitionend
2) in popupshowing, check for the attribute and if truthy, disable pointer-events
3) in popupshown, check for the attribute instead of the expensive getComputedStyle checks.

This is still pretty sad but on the upside, it /should/ work.

Comment 3

[:Gijs (he/him)]

Attachment: attempt to fix sidebar opening from Australis bookmarks menu,

Ploink.

Comment 4

[:Gijs (he/him)]

This still needs a removeProperty call for the case where we don't need to disable the events for the initial show.

Comment 5

[:Gijs (he/him)]

Attachment: attempt to fix sidebar opening from Australis bookmarks menu,

Think I forgot the linux case which doesn't have a transition, but this should still work, I think? Please test. :-)

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8385809 [details] [diff] [review]
attempt to fix sidebar opening from Australis bookmarks menu,

Review of attachment 8385809 [details] [diff] [review]:
-----------------------------------------------------------------

This didn't work at first on Ubuntu, but that's because you need to switch to using this.style.removeProperty when removing pointer-events property. I tried that locally, and it seemed to work just fine.

So r=me with that fixed.

::: browser/components/places/content/menu.xml
@@ +586,5 @@
> +          let container = document.getAnonymousElementByAttribute(this, "anonid", "container");
> +          let cs = getComputedStyle(container);
> +          let transitionProp = cs.transitionProperty;
> +          let transitionTime = parseFloat(cs.transitionDuration);
> +          disablePointerEvents = (transitionProp.indexOf("transform") > -1 ||

Can just use transitionProp.contains("transform") here.

@@ +594,5 @@
> +        } else {
> +          disablePointerEvents = this.getAttribute("disablepointereventsfortransition") == "true";
> +        }
> +        if (!disablePointerEvents) {
> +          this.removeProperty("pointer-events");

This needs to be this.style.removeProperty.

Comment 7

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/2c6243a37618

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2c6243a37618

Comment 9

[:Gijs (he/him)]

Comment on attachment 8385809 [details] [diff] [review]
attempt to fix sidebar opening from Australis bookmarks menu,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: bookmarks menu button doesn't respond consistently
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 10

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/978cb9774a23