Closed Bug 983997 Opened 7 years ago Closed 7 years ago

Clicking StarUI does not pop up "Edit Bookmark Panel" when almost finishing of the animation "Bookmarking animation"

Categories

(Firefox :: Toolbars and Customization, defect)

29 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 - verified
firefox30 - verified
firefox31 --- verified

People

(Reporter: alice0775, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: regression, ux-mode-error, Whiteboard: [Australis:P3][good first verify])

Attachments

(3 files, 3 obsolete files)

This is a regression.

Steps to reproduce:
1. Open non-bookmarked page
2. Click Star Icon
3. Click Star Icon when almost finishing of the animation "Bookmarking animation"

Actual Results:
Nothing happens

Expected Results:
"Edit Bookmark Panel" should be opened
Summary: Clicking StarUI does not pop up "Edit Bookmark Panel" during animate "Bookmarking animation" → Clicking StarUI does not pop up "Edit Bookmark Panel" when almost finishing of the animation "Bookmarking animation"
Attached file screencast
This was a tradeoff we made in bug 981548. We need to re-evaluate anyway because of the upcoming resolution of bug 981637. I have some ideas on how to do this, but it's low priority right now.
Blocks: 981548
No longer blocks: 931343
I think this is UX regression.
Bug 931343 is only visual play.

So, It time to decision to back out Bug 931343.
Keywords: ux-mode-error
(In reply to Alice0775 White from comment #3)
> I think this is UX regression.
> Bug 931343 is only visual play.
> 
> So, It time to decision to back out Bug 931343.

I don't think that's the right decision, and neither did Marco, as far as I could tell. Clicking is disabled for approximately 300ms. It's hard to notice unless you're looking for it, and double-clicking the star icon still works the way it used to. I don't think the current problem warrants backing out the entire animation. I will do my best to work on a proper fix and get it on beta 29 before we ship, but I can't promise anything.

As it is, this is a minor issue, but probably still P4 because of the aforementioned bug which will mean the workaround will no longer stop the button from overflowing in the first place, effectively rendering it useless.
Whiteboard: [Australis:P4]
this is an edge case, there are bigger fishes to fry
Bug 981637 made 30, so the fix is now likely to be useless on 30 (I've not tested this, though - I would expect the symptoms would be like bug 970013's). Bumping up the priority of fixing this properly.
Blocks: 981637
Whiteboard: [Australis:P4] → [Australis:P3]
Sigh. Will look at this tomorrow.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Figured out a workaround for the border issue that I ran into for bug 970013... just need to extend this fix to Windows and Linux, which is harder because my VMs are in flux as I'm moving them between machines.
FWIW, I do not believe this should track 29. Can you clarify why you thought it should?
Flags: needinfo?(bkerensa)
(In reply to :Gijs Kruitbosch from comment #8)
> Created attachment 8393444 [details] [diff] [review]
> move pulse animation into animation container, too,
> 
> Figured out a workaround for the border issue that I ran into for bug
> 970013... just need to extend this fix to Windows and Linux, which is harder
> because my VMs are in flux as I'm moving them between machines.

This is a quite complicated workaround, can't we just be more tolerant in the overflow code (allow 3 to 5 pixels of overflow (at least enough for the bookmarking animation)).
(In reply to Tim Nguyen [:ntim] from comment #10)
> (In reply to :Gijs Kruitbosch from comment #8)
> > Created attachment 8393444 [details] [diff] [review]
> > move pulse animation into animation container, too,
> > 
> > Figured out a workaround for the border issue that I ran into for bug
> > 970013... just need to extend this fix to Windows and Linux, which is harder
> > because my VMs are in flux as I'm moving them between machines.
> 
> This is a quite complicated workaround, can't we just be more tolerant in
> the overflow code (allow 3 to 5 pixels of overflow (at least enough for the
> bookmarking animation)).

No, because we rely on overflow events, so if buttons were added without you changing the size of the window (by add-ons or otherwise) and we were already overflown-by-3-to-5-pixels-but-didn't-do-anything, we'd not get a second event, and wouldn't know to check. Plus, it'd look ugly if you made some other thing "underlap" the overflow button by 3-5 pixels - icons would look cut off and generally be sadfaces.

The workaround isn't super-complicated (the WIP I put up is already functioning fine on OS X), I've just been busy firefighting other stuff and therefore haven't come back to this. I'll do that either this weekend or next week.
(In reply to :Gijs Kruitbosch from comment #9)
> FWIW, I do not believe this should track 29. Can you clarify why you thought
> it should?

Gijs, https://wiki.mozilla.org/Release_Management/Tracking_rules
Flags: needinfo?(bkerensa)
This adds Linux support.
Attachment #8393444 - Attachment is obsolete: true
This is taking longer than I thought it would because I realized that both the currently present code and my solution have issues when moving the button to other toolbars, because the distance between the icons varies. :-(
Alright, this seems to be simpler, because putting the two icons in one container messes up the positioning algorithms. This works much better. Has some debug styles because my updates to the Linux and Windows styling were made blind (from OS X), going to test and/or tweak this and then hopefully ask for review.
Attachment #8396084 - Attachment is obsolete: true
Here's a finished patch. I'm actually quite pleased with the result, so I hope you are, too, Marco. :-)
Attachment #8397244 - Flags: review?(mak77)
Attachment #8397206 - Attachment is obsolete: true
Comment on attachment 8397244 [details] [diff] [review]
move pulse animation into animation container, too,

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

this is voodoo
Attachment #8397244 - Flags: review?(mak77) → review+
Voodoo has landed:

remote:   https://hg.mozilla.org/integration/fx-team/rev/48ed4330a417
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/48ed4330a417
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 31
Comment on attachment 8397244 [details] [diff] [review]
move pulse animation into animation container, too,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis, bug 981548
User impact if declined: clicks are disabled on the bookmarks icon while it's animating the dropmarker, and the bookmarks icon might overflow during the animation if it's the last item in the toolbar
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): this is reasonably low on Aurora as soon as it's baked on m-c for a bit and nothing shocking has turned up. After a week of testing on Nightly and/or Aurora, I think we can even uplift to beta for consistency. That'd be week 3 of beta, assuming aurora approval and landing happen quickly. If we get to the middle/end of week 4 of beta, I think I'd punt on this for beta.
String or IDL/UUID changes made by this patch: none
Attachment #8397244 - Flags: approval-mozilla-beta?
Attachment #8397244 - Flags: approval-mozilla-aurora?
Comment on attachment 8397244 [details] [diff] [review]
move pulse animation into animation container, too,

Sounds like a good plan!
Attachment #8397244 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Jared, can you own getting this to beta by the end of next week, assuming no regressions crop up, as I'm going to be out? :-)
Flags: needinfo?(jaws)
Had to rebase the original patch. Pushed it to try server and tested it locally,
https://tbpl.mozilla.org/?tree=Try&rev=f29b9a1dc20e
Flags: needinfo?(jaws)
Flags: needinfo?(jaws)
Comment on attachment 8399676 [details] [diff] [review]
Actual beta patch

Thanks Jared.
I am approving it now to make sure it is in beta5.
Attachment #8399676 - Flags: approval-mozilla-beta+
Flags: needinfo?(jaws)
Attachment #8397244 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Whiteboard: [Australis:P3] → [Australis:P3][good first verify]
I wasn't able to reproduce this issue with the Nightly from 2014-03-15 (build ID: 20140315030204) on a Win 7 x64 machine.

Any thoughts/suggestions?
Flags: needinfo?(alice0775)
...
Flags: needinfo?(alice0775)
(In reply to Manuela Muntean [:Manuela] [QA] from comment #27)
> I wasn't able to reproduce this issue with the Nightly from 2014-03-15
> (build ID: 20140315030204) on a Win 7 x64 machine.
> 
> Any thoughts/suggestions?

I've bookmarked several pages, and everytime I clicked the Star Icon when almost finishing of the animation "Bookmarking animation", "Edit Bookmark Panel" was opened.
I can verify.
I can not reproduce in Aurora30.0a2 abd Nightly31.0a1.
https://hg.mozilla.org/releases/mozilla-aurora/rev/3186bbc50050
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140404004001
https://hg.mozilla.org/mozilla-central/rev/ac6cbaa47f34
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140403030201

However, I can still reproduce in Firefox29.0b4
(29b4 is not include the patch yet)
https://hg.mozilla.org/releases/mozilla-beta/rev/6ca67f136635
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140331125246
(In reply to Alice0775 White from comment #30)
> I can verify.
> I can not reproduce in Aurora30.0a2 abd Nightly31.0a1.
> https://hg.mozilla.org/releases/mozilla-aurora/rev/3186bbc50050
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
> ID:20140404004001
> https://hg.mozilla.org/mozilla-central/rev/ac6cbaa47f34
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
> ID:20140403030201
> 
> However, I can still reproduce in Firefox29.0b4
> (29b4 is not include the patch yet)
> https://hg.mozilla.org/releases/mozilla-beta/rev/6ca67f136635
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
> ID:20140331125246

Could you also try Firefox29.0b5 (https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/29.0b5-candidates/build1/)? Should be fixed there too.
Thanks.
I cannot reproduce the problem in Firefox29b5.-candidates.
https://hg.mozilla.org/releases/mozilla-beta/rev/072687c58007
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140403132807
Marking as Verified based on Alice's testing... thank you Alice.
You need to log in before you can comment on or make changes to this bug.