Closed Bug 1374230 Opened 5 years ago Closed 5 years ago

Wrong tooltip position after resizing browser window

Categories

(Firefox :: Preferences, enhancement, P1)

56 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox56 --- verified

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(2 files)

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.
Flags: qe-verify+
Summary: Search sign and arrow indicator aren't displayed with high contrast theme → Wrong tooltip position after resizing browser window
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!
Flags: needinfo?(mconley)
QA Contact: hani.yacoub
Whiteboard: [photon-preference][triage] → [photon-preference]
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.
Attachment #8879066 - Flags: review?(mconley) → review-
Mike, I just wrap the calculateTooltipPosition within requestIdleCallback(), and hope I'm doing the right thing.

Please take a look again. Thanks!
Flags: needinfo?(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?
Attachment #8879066 - Flags: review?(mconley) → review-
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
Btw, using throttle rather than debounce is a better solution to cooperate with resize event.

https://css-tricks.com/debouncing-throttling-explained-examples/
(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 on attachment 8879066 [details]
Bug 1374230 - Wrong tooltip position after resizing browser window

https://reviewboard.mozilla.org/r/150388/#review157534
Attachment #8879066 - Flags: review?(mconley) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4961b6529b1a
Wrong tooltip position after resizing browser window r=mconley
https://hg.mozilla.org/mozilla-central/rev/4961b6529b1a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.