1374230 - Wrong tooltip position after resizing browser window

Status: VERIFIED FIXED

(Firefox :: Settings UI, enhancement, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

Attachment: wrong-tooltip-position.png

As picture, search tooltip position is wrong after resizing browser window. We should re-calculate tooltip position when resizing window to make sure it always floats on right place.

Comment 1

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1374230 - Wrong tooltip position after resizing browser window

Review commit: https://reviewboard.mozilla.org/r/150388/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150388/

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8879066 [details]
Bug 1374230 - Wrong tooltip position after resizing browser window

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150388/diff/1-2/

Comment 3

[Ricky Chien [:rickychien] (inactive)]

We calculate tooltip position manually due to unsupported absolute position in XUL. There are some cases that might affect the tooltip position and that position needs to be re-calculated.

Mike, feel free to review this patch. Thanks!

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8879066 [details]
Bug 1374230 - Wrong tooltip position after resizing browser window

https://reviewboard.mozilla.org/r/150388/#review155256

Looks good, but I'm worried about frequent firing of the resize event when manually resizing the window. We should debounce the function by

::: browser/components/preferences/in-content-new/findInPage.js:21
(Diff revision 2)
> +      window.addEventListener("resize", () => {
> +        this.listSearchTooltips.forEach((anchorNode) => {
> +          this.calculateTooltipPosition(anchorNode);
> +        });
> +      });

This will fire very frequently if the user is resizing the window manually. We should probably debounce this, perhaps using requestIdleCallback.

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8879066 [details]
Bug 1374230 - Wrong tooltip position after resizing browser window

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150388/diff/2-3/

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Mike, I just wrap the calculateTooltipPosition within requestIdleCallback(), and hope I'm doing the right thing.

Please take a look again. Thanks!

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8879066 [details]
Bug 1374230 - Wrong tooltip position after resizing browser window

https://reviewboard.mozilla.org/r/150388/#review156868

::: browser/components/preferences/in-content-new/findInPage.js:22
(Diff revision 3)
> +        window.requestIdleCallback(() => {
> +          this.listSearchTooltips.forEach((anchorNode) => {
> +            this.calculateTooltipPosition(anchorNode);
> +          });
> +        });

This is close, but isn't exactly what I meant.

resize events might come in bursts, or in great number, while the user is resizing the window. Yes, we should queue a response with requestIdleCallback - but if another resize event comes back before that callback is fired, we should cancel that idle callback, and queue another one.

So I'm suggesting something like this (untested) code:

```js
let callbackId = null;
window.addEventListener("resize", () => {
  if (callbackId) {
    window.cancelIdleCallback(callbackId);
    callbackId = null;
  }

  callbackId = window.requestIdleCallback(() => {
    callbackId = null;
    // ...
  });
});
```

That way, we reduce the probability of re-calculating tooltip positions for _every single_ resize event.

Does that make sense?

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8879066 [details]
Bug 1374230 - Wrong tooltip position after resizing browser window

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150388/diff/3-4/

Comment 9

[Ricky Chien [:rickychien] (inactive)]

According to doc [1], the callback of requestIdleCallback will be run after current frame has already finished drawing, so we should avoid changing layout within the callback. Instead, window.requestAnimationFrame is a better option to schedule that.

I'm going to use window.requestAnimationFrame to re-calculate tooltip layout without disrupting performance. I hope it makes sense to you.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Background_Tasks_API#Getting_the_most_out_of_idle_callbacks

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8879066 [details]
Bug 1374230 - Wrong tooltip position after resizing browser window

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150388/diff/4-5/

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8879066 [details]
Bug 1374230 - Wrong tooltip position after resizing browser window

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150388/diff/5-6/

Comment 12

[Ricky Chien [:rickychien] (inactive)]

Btw, using throttle rather than debounce is a better solution to cooperate with resize event.

https://css-tricks.com/debouncing-throttling-explained-examples/

Comment 13

[Mike Conley (:mconley) (:⚙️)]

(In reply to Ricky Chien [:rickychien] from comment #9)
> I'm going to use window.requestAnimationFrame to re-calculate tooltip layout
> without disrupting performance. I hope it makes sense to you.
> 

requestAnimationFrame is called just before the "natural" style and layout flushes occur and before paint, and is where we should normally make DOM changes.

This is unfortunate. One of the first things calculateTooltipPosition does is read style / layout data from the DOM using getBoundingClientRect, so we've essentially maximized our potential for doing expensive flushes here.

Once this ships, we should really refactor this tooltip code to ensure that reads are done at the beginning of the pipeline (perhaps using requestIdleCallback), and writes are done using requestAnimationFrame.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8879066 [details]
Bug 1374230 - Wrong tooltip position after resizing browser window

https://reviewboard.mozilla.org/r/150388/#review157534

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4961b6529b1a
Wrong tooltip position after resizing browser window r=mconley

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4961b6529b1a

Comment 17

[Hani Yacoub, Desktop QA]

Build ID: 20170627030209
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

Verified as fixed on Firefox Nightly 56.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.