The bookmarks button moves to the overflow after some clicks

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: mak, Assigned: Gijs)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 30
All
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified, firefox30 verified)

Details

(Whiteboard: [Australis:P2])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
Whiteboard: [Australis:P2]
(Assignee)

Comment 1

4 years ago
(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.
Flags: needinfo?(mak77)
(Reporter)

Comment 2

4 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(mak77)
Resolution: --- → WORKSFORME
(Reporter)

Comment 3

4 years ago
This reproduced again, though STR are very intermittent
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Comment 4

4 years ago
(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.
OS: All → Windows 8
(Reporter)

Comment 5

4 years ago
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...
(Assignee)

Comment 6

4 years ago
(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...
(Assignee)

Comment 7

4 years ago
I can reproduce this on Win7, at least.
(Reporter)

Comment 8

4 years ago
(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
(Assignee)

Comment 9

4 years ago
(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.
(Assignee)

Comment 10

4 years ago
(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...
Blocks: 872617, 931343
(Reporter)

Comment 11

4 years ago
ah yes, I can reproduce if I click exactly when the icon pulses, good catch.
(Reporter)

Comment 12

4 years ago
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.
(Assignee)

Comment 13

4 years ago
(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...
(Assignee)

Comment 14

4 years ago
(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...)
(Reporter)

Comment 15

4 years ago
(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
(Assignee)

Comment 16

4 years ago
(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
Needs an owner --> MattN!
Assignee: nobody → MattN+bmo
(Assignee)

Comment 18

4 years ago
Per dicussion, stealing.
Assignee: MattN+bmo → gijskruitbosch+bugs
Status: REOPENED → ASSIGNED
(Assignee)

Comment 19

4 years ago
Created attachment 8385294 [details] [diff] [review]
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.
Attachment #8385294 - Flags: feedback?(jaws)
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).
Attachment #8385294 - Flags: feedback?(jaws) → feedback+
(Assignee)

Comment 21

4 years ago
Created attachment 8387158 [details] [diff] [review]
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...
Attachment #8387158 - Flags: review?(jaws)
(Assignee)

Updated

4 years ago
Attachment #8385294 - Attachment is obsolete: true
Attachment #8387158 - Flags: review?(jaws) → review+
(Assignee)

Comment 22

4 years ago
remote:   https://hg.mozilla.org/integration/fx-team/rev/d76054e2a322
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-fx-team]
(Reporter)

Comment 23

4 years ago
shouldn't that go into content/browser.css?
(Assignee)

Comment 24

4 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/d76054e2a322
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 30
(Assignee)

Comment 26

4 years ago
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
Attachment #8387158 - Flags: approval-mozilla-aurora?
status-firefox29: --- → affected
status-firefox30: --- → fixed
Attachment #8387158 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

4 years ago
Depends on: 981548
(Assignee)

Updated

4 years ago
Depends on: 981637
(Assignee)

Updated

4 years ago
Depends on: 981520

Updated

4 years ago
Keywords: verifyme
(Reporter)

Comment 28

4 years ago
Ugh, this just happened to me again right now :(
(Reporter)

Updated

4 years ago
Blocks: 986801
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.
Keywords: verifyme

Updated

4 years ago
Keywords: verifyme

Comment 30

4 years ago
Reopening bug per comments 28 and 29.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 31

4 years ago
comment 28 doesn't matter, it was before Gijs applied the final fix to the tree (don't remember the bug # off-hand).
(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?
Sorry, that's invalid although it has pointers to two other bugs.
(Assignee)

Comment 34

4 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Verified fixed on Win7, Win8, Ubuntu 13.10 and Mac OS X using:
#latest Aurora, build ID: 20140425004002
#Firefox 29 RC, build ID: 20140421221237
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
status-firefox30: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.