Closed Bug 1472557 Opened 3 years ago Closed 2 years ago

Remove the "scrollbox" anonymous element from "richlistbox"

Categories

(Toolkit :: XUL Widgets, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1472558
Attached patch Work in progress (obsolete) — Splinter Review
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.
Flags: needinfo?(paolo.mozmail)
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.
Flags: needinfo?(paolo.mozmail)
Depends on: 1516258
Depends on: 1516442
Attachment #9011792 - Attachment is obsolete: true
Depends on: 1516763
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).
Flags: needinfo?(jorgk)
Depends on: 1516813
Thanks for the heads-up, I've filed bug 1516813.
Flags: needinfo?(jorgk)
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.
Flags: needinfo?(bgrinstead)
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b2453d488ae
Remove the "scrollbox" anonymous element from "richlistbox". r=bgrins
https://hg.mozilla.org/mozilla-central/rev/8b2453d488ae
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Assignee: nobody → paolo.mozmail

(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.

Flags: needinfo?(bgrinstead)
Depends on: 1521309
Depends on: 1524210
Depends on: 1537735
No longer depends on: 1537735
Regressions: 1537735
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.