Closed
Bug 983997
Opened 11 years ago
Closed 11 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)
Tracking
()
VERIFIED
FIXED
Firefox 31
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)
143.84 KB,
application/x-shockwave-flash
|
Details | |
11.42 KB,
patch
|
mak
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
10.93 KB,
patch
|
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•11 years ago
|
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"
Reporter | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
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.
Reporter | ||
Comment 3•11 years ago
|
||
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
Assignee | ||
Comment 4•11 years ago
|
||
(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]
Comment 5•11 years ago
|
||
this is an edge case, there are bigger fishes to fry
Assignee | ||
Comment 6•11 years ago
|
||
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]
Updated•11 years ago
|
Assignee | ||
Comment 7•11 years ago
|
||
Sigh. Will look at this tomorrow.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•11 years ago
|
||
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.
Assignee | ||
Comment 9•11 years ago
|
||
FWIW, I do not believe this should track 29. Can you clarify why you thought it should?
Flags: needinfo?(bkerensa)
Comment 10•11 years ago
|
||
(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)).
Assignee | ||
Comment 11•11 years ago
|
||
(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.
Comment 12•11 years ago
|
||
(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
Assignee | ||
Comment 13•11 years ago
|
||
This adds Linux support.
Assignee | ||
Updated•11 years ago
|
Attachment #8393444 -
Attachment is obsolete: true
Assignee | ||
Comment 14•11 years ago
|
||
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. :-(
Assignee | ||
Comment 15•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Attachment #8396084 -
Attachment is obsolete: true
Assignee | ||
Comment 16•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8397206 -
Attachment is obsolete: true
Comment 17•11 years ago
|
||
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+
Assignee | ||
Comment 18•11 years ago
|
||
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]
Comment 19•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/48ed4330a417
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 31
Assignee | ||
Comment 20•11 years ago
|
||
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 21•11 years ago
|
||
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+
Comment 22•11 years ago
|
||
Pushed to Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/253020a7ffe4
Assignee | ||
Comment 23•11 years ago
|
||
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)
Comment 24•11 years ago
|
||
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)
Updated•11 years ago
|
Flags: needinfo?(jaws)
Comment 25•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8397244 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•11 years ago
|
Whiteboard: [Australis:P3] → [Australis:P3][good first verify]
Comment 27•11 years ago
|
||
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)
Comment 29•11 years ago
|
||
(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.
Reporter | ||
Comment 30•11 years ago
|
||
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
Comment 31•11 years ago
|
||
(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.
Reporter | ||
Comment 32•11 years ago
|
||
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
Comment 33•11 years ago
|
||
Marking as Verified based on Alice's testing... thank you Alice.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•