1472557 - Remove the "scrollbox" anonymous element from "richlistbox"

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P3)

Description

[:Paolo Amadini]

The "scrollbox" element in "richlistbox" is a legacy artifact and probably not needed anymore, since its scrolling features are now available on all normal CSS frames.

This is the only anonymous element in "richlistbox", and removing this will unblock the conversion to Custom Element.

Comment 1

[:Paolo Amadini]

Attachment: Work in progress

Comment 2

[Brian Grinstead [:bgrins]]

Does this generally work with the patch applied? This would be great to do since I'm guessing Bug 1472558 would be relatively easy after this, now that we can implement the interface on it.

Comment 3

[:Paolo Amadini]

No, there are two things to figure out:

1. Since "position: sticky;" is not supported in XUL layout (bug 987339), we need a way to keep the headers fixed in the few places where we actually have them. If there is no easy way to do that without anonymous content, we can still solve this by moving the headers outside of the <richlistbox> element. We need to touch the consumers anyways at some point, since we have to migrate them away from <treecol>. Example:

https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/browser/components/preferences/permissions.xul#56-60

2. We need to replace the scrollBy and scrollToElement calls with something else. This is probably quite easy to reimplement using standard DOM methods.

Comment 4

[:Paolo Amadini]

Attachment: Bug 1472557 - Remove the "scrollbox" anonymous element from "richlistbox". r=bgrins

Comment 5

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=723bf13f1654738332ef2a4bc7b4f8d6a66a2a1d

Comment 6

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7a86386a99d9f4e2ac668ae9ac96e4a9027cdce

Comment 7

[Brian Grinstead [:bgrins]]

Once this lands, the CSS changes in https://phabricator.services.mozilla.com/D15388 will need to be ported to TB - mostly this is flattening `richlistbox > scrollbox` selectors to `richlistbox` (see https://searchfox.org/comm-central/search?q=%3E+scrollbox&path=).

In some cases margins that were previously split between the two elements will need to be merged into just `richlistbox` (see toolkit/themes/linux/mozapps/update/updates.css).

Comment 8

[Jorg K (CEST = GMT+2)]

Thanks for the heads-up, I've filed bug 1516813.

Comment 9

[:Paolo Amadini]

I have a XUL reftest failure on Android only, I'll disable just this test for now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ebbf40ddbec11d28f79ac681c1eeac0071d7c1b&selectedJob=219293558

Brian, what is the latest thinking in bug 1465457 about disabling more XUL reftests? It's not clear to me how much of XUL is still used on Android, I just see a minimal "browser.xul" file there, and maybe other regression tests already cover this usage without any need for reftests.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b2453d488ae
Remove the "scrollbox" anonymous element from "richlistbox". r=bgrins

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8b2453d488ae

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #9)

I have a XUL reftest failure on Android only, I'll disable just this test
for now:

https://treeherder.mozilla.org/#/
jobs?repo=try&revision=8ebbf40ddbec11d28f79ac681c1eeac0071d7c1b&selectedJob=2
19293558

Brian, what is the latest thinking in bug 1465457 about disabling more XUL
reftests? It's not clear to me how much of XUL is still used on Android, I
just see a minimal "browser.xul" file there, and maybe other regression
tests already cover this usage without any need for reftests.

Sorry for the delay. From everything I can gather, there's not really any reason for stuff that's XUL-specific to run on Android. The current patch in bug 1465457 skips anything that we flagged as "chrome" (i.e. run in the parent process over a chrome:// URI) in Android.