Closed Bug 981548 Opened 6 years ago Closed 6 years ago

Bookmark star animation disables click on bookmarks widget during transition

Categories

(Firefox :: Theme, defect)

All
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: ge3k0s, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(1 file)

When you click the bookmark star the animation begins and during it you are unable to click either the star or the menu. It's painful especially when you want to quickly click on the star a second time to open the panel. The button were previously clickable during the animation interval.
Summary: Bookmark star animation disables click during transition → Bookmark star animation disables click on bookmarks widget during transition
the patch should have disabled mouse events only on the popup, not on the button... this is indeed very wrong
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Marco Bonardo [:mak] from comment #1)
> the patch should have disabled mouse events only on the popup, not on the
> button... this is indeed very wrong

No, we disabled mouse events on the button in bug 970013. Considering the alternative, I don't think this was wrong. I also don't see a way to fix this, and I think the impact of this is extremely low.
Blocks: 970013
the impact of this is not extremely low, not even low. Double click on the star to add and edit has been there from FX3, it's an important part of the starring ui. So I'd define it pretty high, this is primary ui.
(In reply to Marco Bonardo [:mak] from comment #3)
> the impact of this is not extremely low, not even low. Double click on the
> star to add and edit has been there from FX3, it's an important part of the
> starring ui. So I'd define it pretty high, this is primary ui.

It historically working doesn't magically make this "primary" UI. There's no separate handling for the 'double click' case, it just happened to work. I didn't know this was a thing, nobody else (including you!) brought it up in bug 970013, I never noticed it when testing - in fact, in certain cases without this fix the behaviour was already broken because the item would go into the overflow panel.

Anyway, I'd love to hear of ways to fix this instead of debating impact. I don't know of one. We could try moving the pointer-events bit to the dropmarker only, but I suspect that won't fix the original issue in bug 970013.

I originally had a different approach for bug 970013, which involved moving the animation to a different layer. That was made difficult by Windows theming which has borders and padding as part of the icon. If you move the animation to a separate element, you need to hide the original icon (because the icon is half-transparent and therefore animating a second one over the top looks terrible). Hiding the original icon means the borders of the icon suddenly disappear for the duration of the animation, which looks blindingly obviously glitchy. We could hack the borders onto the animating element inside the animation container, but that becomes extremely hacky and sadmaking and technical debt and prone to theme bitrotting and... well. This is why I abandoned that approach.

I don't think adding timeouts to the overflow handling is the way to go, either (for similar hackiness/technical debt/sadmaking reasons).

I don't recall hearing other suggestions, but now would be a great time to bring some up.
(In reply to :Gijs Kruitbosch from comment #4)
> Anyway, I'd love to hear of ways to fix this instead of debating impact. I
> don't know of one. We could try moving the pointer-events bit to the
> dropmarker only, but I suspect that won't fix the original issue in bug
> 970013.

I just tried it and it indeed doesn't fix the original issue.
Depends on: 981637
I filed bug 981637 for the layout issue. Jared looked at adding a doubleclick event handler, but those are triggered after the two clicks, which means they'll likely race with our setting pointer-events: none on the element.

Actually, upon thinking more about this... I *think* we could mitigate by setting pointer-events: none only when the pulse animation is running (300ms). That gives you 600ms from the start of the animation. CSS fires animation events (animationstart, animationend) which can help make this not suck as badly as using a setTimeout to do the same. I'll check if this helps.
So this reintroduces bug 970013 but only if you try really hard / are really unlucky. Double-clicking works again, and there's only a 300ms window where clicking the button doesn't do anything. A trade-off, if you will. I had to add the call within the setTimeout because if you are unlucky, the animation doesn't complete and the button moves to the menu and ends up never getting its pointerEvents reset, which is Bad.
Attachment #8388797 - Flags: review?(mak77)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
out of curiosity, I understand that disabling pointer-events on the popup doesn't work, since you tested it in comment 5, did you try to do that on the dropmarker button instead?
An alternative: what if you set "pointer-events: auto" on the star button when setting "none" on the whole button?
(In reply to Marco Bonardo [:mak] from comment #8)
> out of curiosity, I understand that disabling pointer-events on the popup
> doesn't work, since you tested it in comment 5, did you try to do that on
> the dropmarker button instead?
> An alternative: what if you set "pointer-events: auto" on the star button
> when setting "none" on the whole button?

comment 5 was about the dropmarker. The popup isn't involved here at all.

The reason setting it on the dropmarker has no effect is because the dropmarker doesn't get events in the first place because of XBL. It's the outer button that catches the events we use for the dropmarker, and then we check if they didn't hit the main inner button (star). Only the inner button (star) has allowevents set to true. So I don't think that'll help. We could try to copy the binding and mess with the allowevents attribute but that's much higher risk. I'm not sure why the button isn't already setting that attribute on the dropmarker but presumably there's a reason.
Comment on attachment 8388797 [details] [diff] [review]
bookmark star animation disables click on bookmarks widget during transition,

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

This actually looks better than I thought. yes, there is a brief time where it won't work, and likely we'll get a bug filed about "star intermittently not opening the popup", but covers the most common double click case.
Exactly since I expect intermittent issues, I think a nice comment above the handlers explaining why they are there and that this may cause the above issue, would be very welcome.
I hope one day we'll get a more general fix for these overflow issues so that we can remove these local workarounds.

::: browser/base/content/browser-places.js
@@ +1323,5 @@
>      PlacesCommandHook.updateBookmarkAllTabsCommand();
>    },
>  
>    _showBookmarkedNotification: function BUI_showBookmarkedNotification() {
> +    let onDropmarkerAnimationEnd = (e) => {

nit: you don't need to wrap e into parentheses, actually you are not even using it, so even just () would do.

@@ +1327,5 @@
> +    let onDropmarkerAnimationEnd = (e) => {
> +      this.button.removeEventListener("animationend", onDropmarkerAnimationEnd);
> +      this.button.style.removeProperty("pointer-events");
> +    };
> +    let onDropmarkerAnimationStart = (e) => {

ditto
Attachment #8388797 - Flags: review?(mak77) → review+
Landed with a longer comment and with s#(e)#()#

remote:   https://hg.mozilla.org/integration/fx-team/rev/00d3279dfeef
Whiteboard: [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/00d3279dfeef
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
Comment on attachment 8388797 [details] [diff] [review]
bookmark star animation disables click on bookmarks widget during transition,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis / bug 970013
User impact if declined: bookmarks menu button isn't clickable for a long-ish time after initial starring, breaking double-click-to-add-and-edit-bookmark routine
Testing completed (on m-c, etc.): locally, on m-c
Risk to taking this patch (and alternatives if risky): reasonably low
String or IDL/UUID changes made by this patch: none
Attachment #8388797 - Flags: approval-mozilla-aurora?
Attachment #8388797 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 983997
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.