Closed
Bug 1316459
Opened 8 years ago
Closed 7 years ago
Animation on previously highlighted term shouldn't bounce
Categories
(Toolkit :: Find Toolbar, defect, P2)
Toolkit
Find Toolbar
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: Dolske, Assigned: mikedeboer)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
In the new modal find-in-page UI, the currently highlighted term has a yellow hilight bubble. The bubble has a little expansion/bounce animation when moving between terms. I think the animation is being shown on the wrong term. Looking closely, when pressing Command-G to move to the next term, this animation is performed on the _currently_ highlighted term, and then it instantly jumps to the next term. I think it should be the other way around -- instantly jump to the next term, and perform the animation there so that the eye is drawn to the new position. As is, this draws the eye to the current term (which you're probably already looking at), and doesn't help (or even distracts from) locating the new term.
Assignee | ||
Updated•8 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8820273 [details] Bug 1316459 - play range outline animations after its properties have been set as the result of a scheduled repaint of the modal highlighter. https://reviewboard.mozilla.org/r/99812/#review100264 ::: toolkit/modules/FinderHighlighter.jsm:997 (Diff revision 1) > + let animation; > + if (dict.animations) { > + for (animation of dict.animations) > + animation.finish(); > + } > + dict.animations = []; Shouldn't pending animations finish immediately? ::: toolkit/modules/FinderHighlighter.jsm:999 (Diff revision 1) > + for (animation of dict.animations) > + animation.finish(); for...of is not safe against splicing, and you add onfinish handlers to these that splice them out of the array, so does this actually work?
Attachment #8820273 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #2) > Shouldn't pending animations finish immediately? What do you mean by this? I think they are... > for...of is not safe against splicing, and you add onfinish handlers to > these that splice them out of the array, so does this actually work? Yeah, since the handler is called asynchronously. That's why I'm binding the index to splice to the handler function.
Comment 4•7 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #3) > (In reply to :Gijs Kruitbosch from comment #2) > > Shouldn't pending animations finish immediately? > > What do you mean by this? I think they are... I mean it seems to me that the .finish() calls and the clearing of dict.animations should be in .update(). > > for...of is not safe against splicing, and you add onfinish handlers to > > these that splice them out of the array, so does this actually work? > > Yeah, since the handler is called asynchronously. OK, so calling .finish() manually doesn't immediately call the onfinish callback? That's... surprising. > That's why I'm binding the > index to splice to the handler function. Those two things seem like they're not necessarily related - you need to bind the index also because the index isn't local, right? So like, the value of i will be whatever it is at the end of the loop by the time onfinish is called.
Flags: needinfo?(mdeboer)
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to :Gijs from comment #4) > Those two things seem like they're not necessarily related - you need to > bind the index also because the index isn't local, right? So like, the value > of i will be whatever it is at the end of the loop by the time onfinish is > called. Please evaluate the following in your JS console: ```js let anims = []; for (let i = 0, l = 10; i < l; ++i) { anims.push(function(idx) { console.log(idx); }.bind(null, i)); } for (let anim of anims) anim(); ```
Flags: needinfo?(mdeboer)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7b35b631ae7
Comment 8•7 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #5) > (In reply to :Gijs from comment #4) > > Those two things seem like they're not necessarily related - you need to > > bind the index also because the index isn't local, right? So like, the value > > of i will be whatever it is at the end of the loop by the time onfinish is > > called. > > Please evaluate the following in your JS console: > ```js > let anims = []; for (let i = 0, l = 10; i < l; ++i) { > anims.push(function(idx) { console.log(idx); }.bind(null, i)); } for (let > anim of anims) anim(); > ``` I don't follow your point, for two reasons. First, re-reading this, I think block-scoped let was finally fixed. So this: let anims = []; for (let i = 0, l = 10; i < l; ++i) { anims.push(function() { console.log(i); }); } for (let anim of anims) anim(); gives the same answer as your snippet. Second, my point was that calling array.splice() against e.g. index 9 is going to break if, by the time that it's executed, the array has already had other items removed and now has length 5. I don't see how it relates to your use of bind().
Flags: needinfo?(mdeboer)
Assignee | ||
Comment 9•7 years ago
|
||
Well, I also pushed a new version of the patch... the answer should be in there. I've confirmed it works as expected.
Flags: needinfo?(mdeboer)
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8820273 [details] Bug 1316459 - play range outline animations after its properties have been set as the result of a scheduled repaint of the modal highlighter. https://reviewboard.mozilla.org/r/99812/#review104978 ::: toolkit/modules/FinderHighlighter.jsm:996 (Diff revision 2) > + dict.animations = []; > + for (let i = rectsAndTexts.rectList.length - 1; i >= 0; --i) { > + animation = dict.modalHighlightOutline.setAnimationForElement(kModalOutlineId + i, > + Cu.cloneInto(kModalOutlineAnim.keyframes, window), kModalOutlineAnim.duration); > + animation.onfinish = function (idx) { dict.animations.splice(idx, 1); }.bind(null, i); > + dict.animations.push(animation); So, you don't need the bind. More generally, I wonder if this code would be simpler if you used a `Set` (so you could just call `dict.animations.delete(this)`) and used a clone of the set to iterate over when trying to finish all of them (ie `for (let animation of [...dict.animations]) { animation.finish(); }`). This would make it unambiguous that you're always removing the right thing, and that iteration and removal aren't interfering with each other. As it is, if the `onfinish` callback was sync, it messes with removals from within an iteration. If it's async, and removals happen after iteration, it seems like it'd be possible for the array to have been cleared and to now be filled with different items at the same index, which will be removed instead...
Attachment #8820273 -
Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
So I think I got your comment now and the latest patch should've addressed it. I'm still more than willing to chat about it briefly over IRC ;-)
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8820273 [details] Bug 1316459 - play range outline animations after its properties have been set as the result of a scheduled repaint of the modal highlighter. https://reviewboard.mozilla.org/r/99812/#review105182
Attachment #8820273 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 15•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4df4bb231abd
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f97c6e68946b play range outline animations after its properties have been set as the result of a scheduled repaint of the modal highlighter. r=Gijs
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f97c6e68946b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•