Closed Bug 1897595 Opened 5 months ago Closed 4 months ago

Removal of `overflow`/`underflow` events might causes issues in web extensions

Categories

(Core :: Layout: Scrolling and Overflow, defect, P3)

Product:
Core
Component:
Layout: Scrolling and Overflow
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox126 --- unaffected
firefox127 --- unaffected
firefox128 --- fixed

People

(Reporter: cg+zbmvyynohtmvyyn, Unassigned)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 obsolete file)

Made a separate bug on request by Gregory Pappas [:gregp] in bug 1688022

The change made in Bug 1888737 cause Tree Style Tab addon sidebar to not be able to be scrolled with mouse wheel anymore.

Reported for TST: https://github.com/piroor/treestyletab/issues/3554

Regression found by bisecting with mozregress:
2024-05-18T09:51:44.935000: DEBUG : Found commit message:
Bug 1888737 - Disable overflow/underflow events in early beta r=smaug,emilio

Differential Revision: https://phabricator.services.mozilla.com/D206175

2024-05-18T09:51:44.935000: DEBUG : Did not find a branch, checking all integration branches
2024-05-18T09:51:44.937000: INFO : The bisection is done.
2024-05-18T09:51:44.938000: INFO : Stopped

Keywords: regression
Regressed by: 1888737

This is unfortunate. I guess we have a few options here:

  • Give up on bug 1688022
  • Expose these events to extensions but not general web content
    • There is a precedent for this. The non-standard KeyboardEvent.initKeyEvent method was unshipped in bug 1717760 but is still available to extensions (bug 1727024) (because some major password managers broke)
  • Limit this change to only Nightly to avoid shipping this regression to beta. I imagine a lot of Nightly users employ Tree Style Tabs so this would still be pretty annoying

I'm OK to update TST to use something alternative new Web standard technology for the purpose. How we should track changing of overflow/underflow status with standard technology?

It may be too early to unship overflow/underflow If there still be no available standard.

What are you trying to do with these events? The alternative depends on what you're trying to do really.

Flags: needinfo?(yuki)
Assignee: nobody → gregp
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Set release status flags based on info from the regressing bug 1888737

status-firefox126: --- → unaffected
status-firefox127: --- → unaffected
status-firefox128: --- → affected
status-firefox-esr115: --- → unaffected

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

What are you trying to do with these events? The alternative depends on what you're trying to do really.

I have two usecases:

  • Applying "fade out" visual effect to tab labels only when they are really overflow. There is no CSS value like "fade-out" for the text-overflow property. https://developer.mozilla.org/en-US/docs/Web/CSS/text-overflow
  • Switching visibility of the "add tab" button next to the last tab and the end of the tab bar based on the overflow state of the tab bar.

In short, I need ":overflow" pseudo-class, so I used overflow/underflow events as its alternative.

And finally I've found the way with ResizeObserver. I hope this information become a help for other developers used overflow/underflow events.
https://github.com/piroor/treestyletab/commit/58f7f7c6301d6583ec6815891f48a8d6abeee84b

Flags: needinfo?(yuki)

That's great news! I will tentatively abandon attachment 9402609 [details] since it's not needed at the moment. If more extensions that use overflow/underflow events are discovered, we may have to re-evaluate.

Resolving this bug as MOVED; thanks for the quick updates, piro!!

See Also: → https://github.com/piroor/treestyletab/issues/3554
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → MOVED
Assignee: gregp → nobody
Attachment #9402609 - Attachment is obsolete: true

This should have been made visible to the webextensions team as soon as a prominent added experienced problems.. Removals like this run a risk of breaking addons, such as experienced with TST. Can we hold off on moving this into release until we have time to evaluate the risk on our side?

Status: RESOLVED → REOPENED
Flags: needinfo?(gregp)
Resolution: MOVED → ---

This should have been made visible to the webextensions team as soon as a prominent added experienced problems

Agree

Can we hold off on moving this into release until we have time to evaluate the risk on our side?

Yes, there isn't much urgency here.

Does the webextensions team have a searchable index of extensions submitted to AMO that could be checked to get a rough idea of the impact of this removal?

Flags: needinfo?(gregp)

Re-titling to make it more generic.

I am not sure what the course we will take on this bug, it depends on how many extensions are affected? If there are many, we will revert bug 1888737?

Setting P3:S3 for now to be removed from my radar.

Severity: -- → S3
Priority: -- → P3
Summary: Removal of `overflow`/`underflow` events causes issues in Tree Style Tab addon → Removal of `overflow`/`underflow` events might causes issues in web extensions

Thanks! We're doing some scans for other issues, maybe this can fit in with that. Otherwise it might be mid June before more time may free up.

If it appears that there is significant user impact, then we'd need to figure out a mitigation for that. It could range from reverting, to ensuring developer outreach results in updates to affected addons (or some other idea?). Without data I'd rather not presume a direction.

If it appears that there is significant user impact, then we'd need to figure out a mitigation for that

If necessary, we can enable these events in extensions but not web content. This is what the patch in attachment 9402609 [details] does. This would allow more time to do analysis and developer outreach without interfering with unshipping these events from the web.

I agree with Comment 12, I think we should unbreak extensions for now. We need to buy some time to send comms to extension developers at this point anyway.

:emilio thoughts?

Flags: needinfo?(emilio)

Given bug 1888737 doesn't affect release (it's nightly-and-early-beta) and we don't know of any affected extensions right now, I'd rather keep it as is for now, and look into the extensions to try to confirm there's no issue with it before that pref rides the train to release?

Flags: needinfo?(emilio) → needinfo?(wdurand)

we don't know of any affected extensions right now

This isn't how we operate. We've already hit this with one prominent addon, so we work under the assumption there could be many more. There are thousands or tens of thousands of addons. Even a scan for functionality will not catch them all. I also suspect this is not only sidebar addons, but all addons that have UI (browser action, etc) that could be potentially affected.

There are a couple paths,

  1. retain the functionality for extensions indefinitely. (I wouldn't want to go this route)
  2. retain the functionality for extensions long enough to deprecate for extensions (which includes #3 below)
  3. wait to push to release for at least 3 releases so we can prepare comms and notify addon developers

I prefer #2 because it causes the least disruption, however removal in nightly+early beta we can live with (#3).

Needinfo-ing myself - I'm going to take a stab at getting an estimate on the affected extensions. Because "overflow" and "underflow" are quite generic terms, it is going to be non-trivial to get an accurate estimate.

Flags: needinfo?(rob)

Should we add telemetry for overflow/underflow event listeners in extension documents? Use counters don't work in extension documents (bug 1874776). We could add a new telemetry probe around here perhaps?

:emilio see our options in Comment 15, how do we move forward?

Flags: needinfo?(wdurand) → needinfo?(emilio)

I think 2 or 3 are both acceptable. A use counter or probe might be useful too.

Flags: needinfo?(emilio)

:gregp do you want to resume your patch? I'd like to go with #2.

Flags: needinfo?(gregp)

Yes. I will move it to new bug to make tracking easier.

Flags: needinfo?(gregp)
Depends on: 1898445

(In reply to YUKI "Piro" Hiroshi from comment #6)

And finally I've found the way with ResizeObserver. I hope this information become a help for other developers used overflow/underflow events.
https://github.com/piroor/treestyletab/commit/58f7f7c6301d6583ec6815891f48a8d6abeee84b

A bad news for addon developers: the alternative method based on ResizeObserver has a performance regression reported at https://github.com/piroor/treestyletab/issues/3557#issuecomment-2126860836. It depends on clientWidth/clientHeight/scrollWidth/scrollHeight, so touching these properties on every resizing triggers style computations and eats more CPU resource.

Just a note from a regression-triage perspective [this bug is currently on the 128 "new bugs" regression-triage dashboard]: it looks like we'll be mitigating the issues here via bug 1898445 which has a patch in review. Once that's landed, it looks like there aren't any user-facing issues remaining here (pending decisions/investigations on further steps).

status-firefox128: affected → fix-optional

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)

There are a couple paths,

  1. retain the functionality for extensions indefinitely. (I wouldn't want to go this route)
  2. retain the functionality for extensions long enough to deprecate for extensions (which includes #3 below)

This bug, as described in comment 0, should be essentially FIXED via bug 1898445 (which added a pref that we can use for option 1 or 2 here^). (Thanks gregp for the patch!)

I think the remaining work here is investigation to determine affected add-ons (comment 16) and to determine suggested alternative approaches (one approach may have been ruled out per comment 22); and then ultimately, a pref-removal in several releases when we're ready to deprecate fully (which will want to happen in its own bug, to revert bug 1898445, essentially).

In the meantime, I think we can consider this bug FIXED. (It's not really accurate to have it tracked as a still-open regression causing extension breakage in 128 at this point, now that we have bug 1898445 landed.)

Hence: closing as fixed-by-bug 1898445, but we can still use this bug for discussion/investigation as-needed, or file separate bugs for that if folks prefer.

Status: REOPENED → RESOLVED
Closed: 5 months ago → 4 months ago
status-firefox128: affected → fixed
Resolution: --- → FIXED

I analyzed the latest versions of all public add-ons on AMO (latest version on 2024-05-23), and the usage seems negligible. The results are below.

I searched for extensions containing .js files that includes both the "overflow" and "underflow" strings (between quotes, whether single or double quotes). This yielded 101 results (out of 39k). Most of the matches were from a library (BigNumber / ethers), and after excluding these I had 10 extensions left.
I checked the remaining 10, and only 4 of them use "overflow" and "underflow" events (the rest are coincidental occurrences, ranging from word lists to code comments):

I repeated the same analysis, now less strict, to accept "overflow" and "underflow" in different files within the same extension. That yielded the above extensions, plus Tree Style Tabs version 4.0.15. Although the substrings are present, this version of the extension doesn't use these events.

While it is possible for extensions to use only one of the two events, the results suggest that the actual usage of "overflow" and "underflow" events is negligible.

Flags: needinfo?(rob)

This analysis is super neat. Thanks Rob!!

Does the low usage suggest that it'd be ok to try disabling these events in web content and addons in the same release?

Or should we disable in web content first, then wait a few releases before disabling in addons?

Does the low usage suggest that it'd be ok to try disabling these events in web content and addons in the same release?

Or should we disable in web content first, then wait a few releases before disabling in addons?

To minimize confusion it would be nice if the web-facing change and the add-ons-facing change are done in the same release, but this is not a hard requirement.

Let's create a new bug about flipping the layout.overflow-underflow.content.enabled_in_addons pref to false, referencing bug 1898445 and my analysis in https://bugzilla.mozilla.org/show_bug.cgi?id=1897595#c25.

The TST add-on developer mentioned a work-around, but also the caveat of a performance regression in comment 22. Is that a fundamental issue in ResizeObserver that overflow/underflow events don't have, and if so, can that be addressed with implementation changes in Firefox? If yes, then I suggest waiting with flipping the pref until the fix for that work-around is in place.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: