Closed Bug 1316459 Opened 6 years ago Closed 6 years ago

Animation on previously highlighted term shouldn't bounce

Categories

(Toolkit :: Find Toolbar, defect, P2)

defect

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.
Blocks: 1291278
Priority: -- → P2
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
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)
(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.
(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)
(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)
(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)
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 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)
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/f97c6e68946b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.