973217 - Australis: sometimes bookmarks sidebar opens with bookmarks menu

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Sören Hentzschel]

Since yesterday, sometimes the bookmarks sidebar opens when I click the "show your bookmarks" icon in the main toolbar. I can't reproduce every time, it happens sometimes, in most cases it works as expected. I don't know, I think that I hit the bottom part of the icon or so, anyway, today I opened the bookmarks sidebar at least five times and I never before used the bookmarks sidebar.

Mac OS X 10.9.

Comment 1

[:Gijs (he/him)]

I suspect this is bug 940379's fault. :-(

Jared?

Comment 2

[Marc Auslander]

This sounds like https://bugzilla.mozilla.org/show_bug.cgi?id=941051

Just click down and see if the menu opens so the check box for sidebar is under the cursor.  If so, it's this bug which is supposedly fixed.

Comment 3

[:Gijs (he/him)]

(In reply to Marc Auslander from comment #2)
> This sounds like https://bugzilla.mozilla.org/show_bug.cgi?id=941051
> 
> Just click down and see if the menu opens so the check box for sidebar is
> under the cursor.  If so, it's this bug which is supposedly fixed.

The bug was fixed; it just reappears because we've adjusted the vertical positioning of the menu when it is opened. It makes sense to have a separate bug for the reoccurrence.

The basic issue is that we're using a menu as a panel. We could take Philipp's suggestion in bug 971260 and just eat mouseup events in the popup that occur within the first ~250ms of the panel appearing? Jared, is that crazy? Do you see a better option?

Comment 5

[Justin Dolske [:Dolske]]

(In reply to :Gijs Kruitbosch from comment #3)

> The basic issue is that we're using a menu as a panel. We could take
> Philipp's suggestion in bug 971260 and just eat mouseup events in the popup
> that occur within the first ~250ms of the panel appearing? Jared, is that
> crazy? Do you see a better option?

Or bug 962124?

Comment 6

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #5)
> (In reply to :Gijs Kruitbosch from comment #3)
> 
> > The basic issue is that we're using a menu as a panel. We could take
> > Philipp's suggestion in bug 971260 and just eat mouseup events in the popup
> > that occur within the first ~250ms of the panel appearing? Jared, is that
> > crazy? Do you see a better option?
> 
> Or bug 962124?

I tried this, but after popupshown happens, we're still transitioning, it seems... so I added a setTimeout, and that works. Patch coming up.

Comment 7

[:Gijs (he/him)]

Attachment: bookmarks menu shouldn't allow for mouse events during the transition,

How is this, Jared?

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

may I say this looks really hacky? Why does using a menu make a difference here compared to an arrow panel?
To me looks like the bug is the clickable area of the buttons, the arrow overlaps it, this may become an issue for other panels.
Btw, honestly compared to this patch I'd prefer having the panel misaligned.

Comment 9

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #8)
> may I say this looks really hacky? Why does using a menu make a difference
> here compared to an arrow panel?

Because menus active on just mouseup, and buttons don't.

Comment 10

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Marco Bonardo [:mak] from comment #8)
> > may I say this looks really hacky? Why does using a menu make a difference
> > here compared to an arrow panel?
> 
> Because menus active on just mouseup, and buttons don't.

*activate, d'oh.

Comment 11

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #8)
> To me looks like the bug is the clickable area of the buttons, the arrow
> overlaps it, this may become an issue for other panels.

The arrow might overlap it, but the buttons don't. The reason this is happening at all is that the panel moves (transitions?) downward from its initial point. You can see a latent hover effect on other panels with buttons, but they don't activate the items because the mouseup event doesn't cause anything else to happen. Bug 971260 aims to change that, and once that happens, those panels will have the same problem.

Comment 12

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #11)
> The reason this is happening at all is that the panel moves (transitions?)
> downward from its initial point.
If that animation is the problem, then bug 610545 might fix it as a colateral.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to :Gijs Kruitbosch from comment #11)
> Bug 971260 aims to change that, and once that
> happens, those panels will have the same problem.

That's why I don't think adding a timeout here brings to maintainable code. IT may instead introduce subtle bugs, and it's not scalable to solve the issue more globally.
May we make the transition start from a lower position, bounce down and then move up to point at the origin? (provided it's really the issue)

Comment 14

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #13)
> (In reply to :Gijs Kruitbosch from comment #11)
> > Bug 971260 aims to change that, and once that
> > happens, those panels will have the same problem.
> 
> That's why I don't think adding a timeout here brings to maintainable code.
> IT may instead introduce subtle bugs, and it's not scalable to solve the
> issue more globally.

Why not? The other panels have similar code, the bookmarks toolbar one is just special because it doesn't share the same binding. We'd have to fix the panel binding to do the same if a similar issue happened there.

We could use a transitionend notification instead if that felt less fragile to you.

> May we make the transition start from a lower position, bounce down and then
> move up to point at the origin? (provided it's really the issue)

That seems like an extremely odd UX to me, but let's ask them.

Comment 15

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #14)
> > May we make the transition start from a lower position, bounce down and then
> > move up to point at the origin? (provided it's really the issue)
> 
> That seems like an extremely odd UX to me, but let's ask them.

I agree that this sounds odd. Bug have a look at this: http://phlsa.github.io/ff-bubble-animation
(Try clicking the menu button)
Using a behavior like this means that the menu is never positioned above the button which should solve the problem, right?

Comment 16

[:Gijs (he/him)]

Attachment: Australis' bookmarks menu shouldn't allow for mouse events during the transition (transition event handler variant),

I actually think I might prefer this over the setTimeout.

Comment 17

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] from comment #15)
> (In reply to :Gijs Kruitbosch from comment #14)
> > > May we make the transition start from a lower position, bounce down and then
> > > move up to point at the origin? (provided it's really the issue)
> > 
> > That seems like an extremely odd UX to me, but let's ask them.
> 
> I agree that this sounds odd. Bug have a look at this:
> http://phlsa.github.io/ff-bubble-animation
> (Try clicking the menu button)
> Using a behavior like this means that the menu is never positioned above the
> button which should solve the problem, right?

Actually, in your mockup the expanded panel still overlaps the bottom of the anchor, as seen by the outlines from the devtools. The collapsed panel does this too. Perhaps it could be implemented so that it doesn't, but it'll require adjusting the skews and other transitions.

In any case, that's significantly more complicated than what Marco suggested, and not something I'd want to risk shipping on 29 at this point considering the performance issues we're already having with animating customize mode. We do need a solution for aurora/29, because the current state of the bookmarks menu isn't really shippable.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8378375 [details] [diff] [review]
Australis' bookmarks menu shouldn't allow for mouse events during the transition (transition event handler variant),

Clearing review? because this needs to only do something if there's a transform + transition defined (which there isn't on Linux, and there might not be on custom themes)

Comment 19

[Justin Dolske [:Dolske]]

Comment on attachment 8378375 [details] [diff] [review]
Australis' bookmarks menu shouldn't allow for mouse events during the transition (transition event handler variant),

> /* Give this menupopup an arrow panel styling */
> #BMB_bookmarksPopup {
[...]
>+  pointer-events: none;

Should we be doing this for all arrow panels that animate? Or is this problem somehow unique to just the bookmarks panel?

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Justin Dolske [:Dolske] from comment #19)
> Comment on attachment 8378375 [details] [diff] [review]
> Australis' bookmarks menu shouldn't allow for mouse events during the
> transition (transition event handler variant),
> 
> > /* Give this menupopup an arrow panel styling */
> > #BMB_bookmarksPopup {
> [...]
> >+  pointer-events: none;
> 
> Should we be doing this for all arrow panels that animate? Or is this
> problem somehow unique to just the bookmarks panel?

This problem is unique because we are using a menu as a panel here, which activates on just mouseup (see comment #9).

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

After bug 971260 this will be a general issue. The bookmarks panel is the only one acting properly atm. We should at least have a bug filed to investigate a fix for the general issue and remove the bookmarks panel workaround.

Comment 22

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #19)
> >+  pointer-events: none;
> 
> Should we be doing this for all arrow panels that animate? Or is this
> problem somehow unique to just the bookmarks panel?

bug 962124

(In reply to Marco Bonardo [:mak] from comment #21)
> After bug 971260 this will be a general issue. The bookmarks panel is the
> only one acting properly atm. We should at least have a bug filed to
> investigate a fix for the general issue and remove the bookmarks panel
> workaround.

We can let either bug 962124 or bug 971260 deal with that.

Comment 23

[:Gijs (he/him)]

Attachment: Australis' bookmarks menu shouldn't allow for mouse events during the transition (transition event handler variant),

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8380618 [details] [diff] [review]
Australis' bookmarks menu shouldn't allow for mouse events during the transition (transition event handler variant),

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

::: browser/components/places/content/menu.xml
@@ +576,5 @@
>        <handler event="popupshowing" phase="target"><![CDATA[
>          this.adjustArrowPosition();
>        ]]></handler>
>        <handler event="popupshown" phase="target"><![CDATA[
> +        let container = document.getAnonymousElementByAttribute(this, "anonid", "container");

please define just before first use

@@ +585,5 @@
> +        let transitionProp = cs.transitionProperty;
> +        let transitionTime = parseFloat(cs.transitionDuration);
> +        if ((transitionProp.indexOf("transform") > -1 || transitionProp == "all") &&
> +            transitionTime > 0) {
> +          this.removeAttribute("observemouseevents");

why doing the attribute dance if you could just directly set style.pointerEvents? Since this is just a workaround it would be better to keep it in a single point of the code.

Comment 25

[:Gijs (he/him)]

Attachment: Australis' bookmarks menu shouldn't allow for mouse events during the transition (transition event handler variant),

Like this?

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8380724 [details] [diff] [review]
Australis' bookmarks menu shouldn't allow for mouse events during the transition (transition event handler variant),

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

::: browser/components/places/content/menu.xml
@@ +591,5 @@
> +      ]]></handler>
> +      <handler event="transitionend"><![CDATA[
> +        if (event.originalTarget.getAttribute("anonid") == "container" &&
> +            event.propertyName == "transform") {
> +          this.style.pointerEvents = 'auto';

this.style.removeProperty("pointer-events");

Comment 27

[:Gijs (he/him)]

w/ nit:

remote:   https://hg.mozilla.org/integration/fx-team/rev/706f36fe921f

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

btw, yes, like that. I appreciate the fact this ended up as a monolithic change to a single file.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/706f36fe921f

Comment 30

[:Gijs (he/him)]

Comment on attachment 8380724 [details] [diff] [review]
Australis' bookmarks menu shouldn't allow for mouse events during the transition (transition event handler variant),

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis' bookmarks menu panel-ness
User impact if declined: opening items in the bookmarks menu when opening the panel
Testing completed (on m-c, etc.): on m-c, local
Risk to taking this patch (and alternatives if risky): low. Contained fix, left to bake for several days before requesting approval precisely because it's delicate...
String or IDL/UUID changes made by this patch: none

Comment 31

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9ddb243fe60a