970013 - The bookmarks button moves to the overflow after some clicks

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

So I saw this the other day, then today I read of another user with the same issue and he was suggesting the issue was with double click.
Actually I can reproduce with these steps:
1. move the bookmarks button at the end of the customizable area in the navbar
2. double click on the star
3. click on remove this bookmark
4. repeat steps 2 and 3 until the button moves to the overflow

I didn't try to debug this yet

Comment 1

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #0)
> So I saw this the other day, then today I read of another user with the same
> issue and he was suggesting the issue was with double click.
> Actually I can reproduce with these steps:
> 1. move the bookmarks button at the end of the customizable area in the
> navbar
> 2. double click on the star
> 3. click on remove this bookmark
> 4. repeat steps 2 and 3 until the button moves to the overflow
> 
> I didn't try to debug this yet

I can't reproduce this after repeating this 10-20 times... What platform did you reproduce this on, seeing as this is filed all/all? I'm testing on OS X.

Comment 2

[Marco Bonardo [:mak]]

I am on Windows 8, though I cannot reproduce it anymore on latest nightly, could have been fixed by some other patch, since I could reproduce that constantly.
I'll resolve for now and reopen in case I see it again.

Comment 3

[Marco Bonardo [:mak]]

This reproduced again, though STR are very intermittent

Comment 4

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #3)
> This reproduced again, though STR are very intermittent

It'd be useful if we understood the STR here better... :-(

Marking this as win-only for now.

Comment 5

[Marco Bonardo [:mak]]

well, I can reproduce it again with steps in comment 0 after about 10 tries. entering and exiting customize mode puts it back into the original position, then I can reproduce it again (after some tries)

when starring I see some unexpected onOverflow calls coming from the customizableUI toolbar.xml::handleEvent, most of them are skipped by the this._target.scrollLeftMax > 0 check... and here is the very funny part, when the bug happens the this._target.scrollLeftMax > 0 check passes, though if I examine this._target.scrollLeftMax in the debugger, it is 0, so we should have not entered the loop...

Comment 6

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #5)
> well, I can reproduce it again with steps in comment 0 after about 10 tries.
> entering and exiting customize mode puts it back into the original position,
> then I can reproduce it again (after some tries)
> 
> when starring I see some unexpected onOverflow calls coming from the
> customizableUI toolbar.xml::handleEvent, most of them are skipped by the
> this._target.scrollLeftMax > 0 check... and here is the very funny part,
> when the bug happens the this._target.scrollLeftMax > 0 check passes, though
> if I examine this._target.scrollLeftMax in the debugger, it is 0, so we
> should have not entered the loop...

The debugger spins the event loop, so there's probably just a reflow event that happens.

This implies that when bookmarking, something is changing size... does the star button do that when bookmarking? I didn't think so, but maybe I'm missing something... if we're using different styles for the starred/non-starred state, then that could explain something.

We had a similar issue with the downloads button, which turned out to be due to the notification. However, then we moved the notification from the stack inside the button into its own element, and stuff Just Worked from then on. The bookmarks star notification has always been in its own element (which isn't inside the navbar) so it shouldn't be causing a problem here, I don't think...

Comment 7

[:Gijs (he/him)]

I can reproduce this on Win7, at least.

Comment 8

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #6)
> This implies that when bookmarking, something is changing size... does the
> star button do that when bookmarking? I didn't think so, but maybe I'm
> missing something... if we're using different styles for the
> starred/non-starred state, then that could explain something.

then it would reproduce every time, not intermittently. also the overflow event is intermittent, it doesn't always happen

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #6)
> We had a similar issue with the downloads button, which turned out to be due
> to the notification. However, then we moved the notification from the stack
> inside the button into its own element, and stuff Just Worked from then on.
> The bookmarks star notification has always been in its own element (which
> isn't inside the navbar) so it shouldn't be causing a problem here, I don't
> think...

Ahhh. But the bookmarks dropmarker icon is transformed to scale and pulses. I also noticed that I can only reproduce if I click during the animation. I imagine that the click causes an :active change which causes a reflow which causes the overflow event. The pulse only takes 300ms, however, which is probably why it's hard to reproduce. Let me try to confirm that I can't reproduce if I get rid of that part of the animation.

Comment 10

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to :Gijs Kruitbosch from comment #6)
> > We had a similar issue with the downloads button, which turned out to be due
> > to the notification. However, then we moved the notification from the stack
> > inside the button into its own element, and stuff Just Worked from then on.
> > The bookmarks star notification has always been in its own element (which
> > isn't inside the navbar) so it shouldn't be causing a problem here, I don't
> > think...
> 
> Ahhh. But the bookmarks dropmarker icon is transformed to scale and pulses.
> I also noticed that I can only reproduce if I click during the animation. I
> imagine that the click causes an :active change which causes a reflow which
> causes the overflow event. The pulse only takes 300ms, however, which is
> probably why it's hard to reproduce. Let me try to confirm that I can't
> reproduce if I get rid of that part of the animation.

Yup, removing that animation fixes this issue. I think we can do the pulse animation in the other bit of the overlay, too, and that would fix this...

The other option would be trying to come up with a way to detect this kind of situation from within the overflow handler. I'm not sure how easy/hard that is...

Comment 11

[Marco Bonardo [:mak]]

ah yes, I can reproduce if I click exactly when the icon pulses, good catch.

Comment 12

[Marco Bonardo [:mak]]

a slightly hacky fix may be to delay onoverflow/onunderflow calls from toolbar.xml, if you gen an underflow in less than 500ms from an overflow, don't notify anything, otherwise they are just notified 500ms later.

Comment 13

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #12)
> a slightly hacky fix may be to delay onoverflow/onunderflow calls from
> toolbar.xml, if you gen an underflow in less than 500ms from an overflow,
> don't notify anything, otherwise they are just notified 500ms later.

Mmmm... I also anticipate that that is prone to other issues (what if the animation takes longer, and/or the general suckiness of our setTimeout implementation (esp. on Windows)).

I mean, to a certain degree the interesting thing is that something that doesn't cause neighbouring nodes to move, when there are any, *is* causing an overflow event. I don't really understand that to begin with. If it's not affecting other nodes, why is it affecting overflow?

CC'ing some folks because more ideas would help...

Comment 14

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Marco Bonardo [:mak] from comment #12)
> > a slightly hacky fix may be to delay onoverflow/onunderflow calls from
> > toolbar.xml, if you gen an underflow in less than 500ms from an overflow,
> > don't notify anything, otherwise they are just notified 500ms later.
> 
> Mmmm... I also anticipate that that is prone to other issues (what if the
> animation takes longer, and/or the general suckiness of our setTimeout
> implementation (esp. on Windows)).

(that, and 500ms is well above the 'user can tell' 150-200ms threshold, so it'll seem like we're just being slow for the 99% case which isn't complicated...)

Comment 15

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #13)
> Mmmm... I also anticipate that that is prone to other issues (what if the
> animation takes longer, and/or the general suckiness of our setTimeout
> implementation (esp. on Windows)).

The former should not be an issue, we want animations to look fast, one taking more than 500ms would likely make the product look slow (though it's true the kind of animation may change the perception, but globally we use quick animations).
The latter is an issue only if you fight the timer resolution, that is about 16ms (iirc) on windows, for the kind of timeout we need it would not be an issue.

(In reply to :Gijs Kruitbosch from comment #14)
> (that, and 500ms is well above the 'user can tell' 150-200ms threshold, so
> it'll seem like we're just being slow for the 99% case which isn't
> complicated...)

I don't think this applies to the overflow case, since it's unpredictable at which moment the toolbar will start to overflow for the user, can you shrink the window and predict when it will overflow with a 500ms precision? I can't. If you don't expect something at a given time, you can't notice it being late.

That said, it was also a possible suggestion, there are very likely alternatives.

(In reply to :Gijs Kruitbosch from comment #13)
> I mean, to a certain degree the interesting thing is that something that
> doesn't cause neighbouring nodes to move, when there are any, *is* causing
> an overflow event. I don't really understand that to begin with. If it's not
> affecting other nodes, why is it affecting overflow?

Could be the node itself is generating an overflow event and it just bubbles up to the toolbar? we don't seem to filter the events in toolbar.xml

Comment 16

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #15)
> (In reply to :Gijs Kruitbosch from comment #13)
> > I mean, to a certain degree the interesting thing is that something that
> > doesn't cause neighbouring nodes to move, when there are any, *is* causing
> > an overflow event. I don't really understand that to begin with. If it's not
> > affecting other nodes, why is it affecting overflow?
> 
> Could be the node itself is generating an overflow event and it just bubbles
> up to the toolbar? we don't seem to filter the events in toolbar.xml

We do: http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/src/CustomizableUI.jsm#3515

Comment 17

[Justin Dolske [:Dolske]]

Needs an owner --> MattN!

Comment 18

[:Gijs (he/him)]

Per dicussion, stealing.

Comment 19

[:Gijs (he/him)]

Attachment: move pulse animation into animation container, too, f?jaws

This is the least generic but also the most guaranteed-to-work way to solve this. I'm really wary of inserting timeouts into the overflow code, but if you prefer we could go that route... Asking for feedback only because I've not done Windows or Linux bits, nor (for OS X) bookmarks toolbar bits, but Mike tells me he's unifying the animations on OS X in Bug 909349 anyway, so then that won't be necessary.

Comment 20

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

Comment on attachment 8385294 [details] [diff] [review]
move pulse animation into animation container, too, f?jaws

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

This looks alright, although as we talked in person I'm pretty sure you can remove some of the redundancy in the animation properties. Also, check for RTL, and see if you can use left:18px instead of the translateX(18px).

Comment 21

[:Gijs (he/him)]

Attachment: prevent pointer-events on bookmarks menu button while animating to avoid it moving into the overflow panel,

Well, here's something that's a lot easier to grok...

Comment 22

[:Gijs (he/him)]

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

Comment 23

[Marco Bonardo [:mak]]

shouldn't that go into content/browser.css?

Comment 24

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #23)
> shouldn't that go into content/browser.css?

Don't think so, as the animation is defined in the theme files, and that's what makes this necessary.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d76054e2a322

Comment 26

[:Gijs (he/him)]

Comment on attachment 8387158 [details] [diff] [review]
prevent pointer-events on bookmarks menu button while animating to avoid it moving into the overflow panel,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis' bookmarks animation (bug 931343)
User impact if declined: if you click the bookmarks star button while it's animating and when the star is close to the overflow, it'll disappear into the overflow unnecessarily
Testing completed (on m-c, etc.): locally, on m-c
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

Comment 27

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/9207865a22fe

Comment 28

[Marco Bonardo [:mak]]

Ugh, this just happened to me again right now :(

Comment 29

[Camelia Badau [:cbadau], Desktop QA]

I reproduced the bug on Win7, Win 8 and Ubunutu x32 on Firefox 29(29.0b1 and 29.0b2)and Firefox 30. The bug isn't reproducible on Mac OS X.

Comment 30

[Ioana (away)]

Reopening bug per comments 28 and 29.

Comment 31

[Marco Bonardo [:mak]]

comment 28 doesn't matter, it was before Gijs applied the final fix to the tree (don't remember the bug # off-hand).

Comment 32

[Matthew N. [:MattN]]

(In reply to Marco Bonardo [:mak] from comment #31)
> comment 28 doesn't matter, it was before Gijs applied the final fix to the
> tree (don't remember the bug # off-hand).

Probably bug 986801 from the "blocks" field?

Comment 33

[Matthew N. [:MattN]]

Sorry, that's invalid although it has pointers to two other bugs.

Comment 34

[:Gijs (he/him)]

This was fixed properly in bug 983997. If people can reproduce with builds post that bug, they should open a new bug that blocks that bug.

Comment 35

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Win7, Win8, Ubuntu 13.10 and Mac OS X using:
#latest Aurora, build ID: 20140425004002
#Firefox 29 RC, build ID: 20140421221237