jaredhirsch
commented
9 years ago I'm reconsidering even tackling this bug.
The behavior we're talking about modifying doesn't have anything to do with visible styles applied to the iframed page. This stylesheet only does one thing: it defines a -moz-binding rule that connects our XBL to a particular element ID (specifically, #PopupAutoCompleteRichResultUniversalSearch), see here.
The id <-> XBL connection is half of what's needed to insert our popup into the XUL DOM in place of the standard issue popup.
The second half involves connecting our popup to the XUL DOM, which is done by:
- setting the value of the urlbar's
autocompletepopupattribute to the ID mentioned above; - 'refreshing' the urlbar element (removing it from the XUL DOM, then immediately reinserting it at the same point in the DOM tree).
When our code is unloaded, say when a user disables or uninstalls this addon, we do not attempt to unload our CSS -moz-binding rule. However, we do reset the value of the urlbar's autocompletepopup attribute to refer to the ID of the standard issue popup, and we 'refresh' the urlbar element again.
This works reliably, meaning uninstalling our addon does reliably restore the original popup.
As far as I can tell, so long as we don't make any changes to the XBL file itself, we can leave things as they are and just not worry about XBL file caching.
Why might we need to make changes to our XBL file? We might need to get pointers to new 'anonymous' elements, as seen here, or to override certain behavior that's defined in XBL classes from which we inherit, as seen here. This code hasn't changed much in the past 15 years (possibly because XBL is an undocumented language, and the autocomplete code is also used for autocompleting form fields, so any change is high risk and high visibility), and no changes were needed to accommodate the recent huge refactoring by the platform search team. TL;DR: I think we will probably never need to touch our XBL file again.
One possible problem: the CSS file also sets the default height of the popup, so if we change it as part of an update, the initial render might be the wrong height. However, we're planning to let the iframe control the height by Mozlando (#19), so this might only result in an initial flicker in the case where (1) your addon updated without a restart, (2) we changed the default height of the popup, (3) and only the already-opened windows would be affected.
Finally, while we can cleanly uninstall our addon without a restart, I do see one bug with the current approach: if you disable or uninstall, then re-enable/reinstall without restarting the browser, our popup will not reattach to any currently-open windows (they will still have the regular popup). New windows will have our popup, though. The problem might be that we don't try to detect whether our stylesheet is already in the XUL DOM, so that trying to attach it a second time throws some kind of error. (As is typical for XBL-related bugs, the error itself is swallowed, so you just have to guess at what the problem might be.)
I'll file these two possible bugs as separate issues, and we can triage in our next planning session.
Wrapping up, the XBL integration piece probably (no joke) cost me 3-4 weeks earlier this year, and I think it's worth leaving the CSS handling as it is, possibly with some window-global state tracking (hasAttachedBinding, perhaps?) to work around its shortcomings.
We currently append stylesheets to each window context by attaching stylesheet nodes to the DOM, but we don't clean them up when the extension is unloaded / uninstalled.
The recommended approach is to use the StyleSheet Service, which exposes an API to load and unload styles.