1597318 - Find and get rid of XBL sync binding hacks in front-end code

Status: NEW

(Firefox :: General, task, P3)

Description

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

When XBL was a thing, it used Layout semantics to determine when to attach a binding.

This resulted in "interesting" behaviour, where one would either have to wait for layout to run before calling into binding code, or force layout to run synchronously.

Oftentimes, we went with the latter, using something like el.clientTop; or some such.

emilio just got rid of some of those in bug 1596800, but I think there are more.

Searching for clientTop; in searchfox yields a number of examples: https://searchfox.org/mozilla-central/search?q=clientTop%3B&path=

I'll link to some non-test ones here (excluding the one that emilio is removing in bug 1596800):

• https://searchfox.org/mozilla-central/rev/524bed6dfbc5ae21c62632d83b7573448b29e0ac/browser/components/payments/PaymentUIService.jsm#331
• https://searchfox.org/mozilla-central/rev/524bed6dfbc5ae21c62632d83b7573448b29e0ac/toolkit/components/prompts/src/CommonDialog.jsm#153
• https://searchfox.org/mozilla-central/rev/524bed6dfbc5ae21c62632d83b7573448b29e0ac/toolkit/mozapps/extensions/content/aboutaddons.js#1903

There might be more, but these are the ones I found within a few minutes of searching.

Comment 1

[Sam Foster [:sfoster] (he/him)]

A search for binding in *.js also shows a bunch of workarounds and now-obsolete comments that aren't just the clientTop assignment.
https://searchfox.org/mozilla-central/search?q=binding&case=true&regexp=false&path=*.js

Comment 2

[Florian Quèze [:florian]]

This is potentially very interesting, but I suspect we've already caught the cases that happen during user interactions we care about. The CommonDialog.jsm one seems interesting though. And there's also https://searchfox.org/mozilla-central/rev/0678172d5b5c681061b904c776b668489e3355b0/browser/components/customizableui/CustomizeMode.jsm#419

Comment 3

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] from comment #2)

This is potentially very interesting, but I suspect we've already caught the cases that happen during user interactions we care about. The CommonDialog.jsm one seems interesting though. And there's also https://searchfox.org/mozilla-central/rev/0678172d5b5c681061b904c776b668489e3355b0/browser/components/customizableui/CustomizeMode.jsm#419

Based on this, tentatively marking fxperf:p3 for now.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 5

[Lina Butler (ex-Mozilla)]

Not quite, Bugbug! 😆

Comment 6

[:Gijs (he/him)]

2/3 of the ones in comment 0 still exist:

https://searchfox.org/mozilla-central/rev/b4dadcc4c45a2e381a4866d28ebf3a901decccc1/toolkit/components/prompts/src/CommonDialog.sys.mjs#188

and

https://searchfox.org/mozilla-central/rev/b4dadcc4c45a2e381a4866d28ebf3a901decccc1/toolkit/mozapps/extensions/content/aboutaddons.js#1723-1724

I only spotted one other one using clientTop,

https://searchfox.org/mozilla-central/rev/b4dadcc4c45a2e381a4866d28ebf3a901decccc1/browser/components/customizableui/CustomizeMode.sys.mjs#405

but that's not used for XBL bindings but for an animation. So I think that's still legit?

Anyway, based on this and the perf calculator, I think perf impact is low.

Comment 7

[:Gijs (he/him)]

Hm, although looking with regexes finds some more stuff here, e.g. https://searchfox.org/mozilla-central/rev/b4dadcc4c45a2e381a4866d28ebf3a901decccc1/browser/components/uitour/UITour.sys.mjs#1571-1584 looks suspect. Here's my query in case it's useful in future: https://searchfox.org/mozilla-central/search?q=%5E%5Cs*%5Ba-zA-Z.%5D*%28client%7Coffset%7Cscroll%29%28Width%7CHeight%7CTop%7CLeft%7CRight%29%3B&path=&case=true&regexp=true . There's probably a case for auditing getBoundingClientRect.

Comment 8

[Worcester12345]

(In reply to Mike Conley (:mconley) (:⚙️) (PTO: July 17-21) from comment #0)

When XBL was a thing, it used Layout semantics to determine when to attach a binding.

This resulted in "interesting" behaviour, where one would either have to wait for layout to run before calling into binding code, or force layout to run synchronously.

Oftentimes, we went with the latter, using something like el.clientTop; or some such.

emilio just got rid of some of those in bug 1596800, but I think there are more.

Searching for clientTop; in searchfox yields a number of examples: https://searchfox.org/mozilla-central/search?q=clientTop%3B&path=

I'll link to some non-test ones here (excluding the one that emilio is removing in bug 1596800):

• https://searchfox.org/mozilla-central/rev/524bed6dfbc5ae21c62632d83b7573448b29e0ac/browser/components/payments/PaymentUIService.jsm#331
• https://searchfox.org/mozilla-central/rev/524bed6dfbc5ae21c62632d83b7573448b29e0ac/toolkit/components/prompts/src/CommonDialog.jsm#153
• https://searchfox.org/mozilla-central/rev/524bed6dfbc5ae21c62632d83b7573448b29e0ac/toolkit/mozapps/extensions/content/aboutaddons.js#1903

There might be more, but these are the ones I found within a few minutes of searching.

How does this search work out now? Is this behind us yet?

Comment 9

[:Gijs (he/him)]

(In reply to Worcester12345 from comment #8)

How does this search work out now? Is this behind us yet?

I answered this in comment 6