1454357 - Remove scrollbox binding

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Description

[:Paolo Amadini]

The "scrollbox" binding just contains a box, while the "scrollbox" element provides access to some native functions through the ScrollBoxObject object.

We could remove the binding by updating the consumers to use just a simple box, although we may have to update some styling accordingly.

Comment 1

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The element cannot be removed because some of the implementations have moved to XULScrollElement in bug 1454358. We have to keep the tag name for this.

What's the right approach here instead? Should we have all the consumer construct the <box> element on their own? Should we convert the remaining tiny binding to custom element?

Comment 3

[:Paolo Amadini]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #2)
> The element cannot be removed because some of the implementations have moved
> to XULScrollElement in bug 1454358. We have to keep the tag name for this.

If I remember correctly, callers that rely on XULScrollElement can use DOM functions instead, sometimes reusing existing JavaScript variants of the XULScrollElement methods.

> What's the right approach here instead? Should we have all the consumer
> construct the <box> element on their own? Should we convert the remaining
> tiny binding to custom element?

This bug is about getting rid of XULScrollElement, but it doesn't exclude converting the binding in a different bug.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am a bit confused. Removing the XULScrollElement class that was just implemented in C++?

Anyway, given that we are not going to remove the tag name, and we will not be able to remove the one and only <box> element in anon content, I am going to try to convert the remaining binding into custom element, with <content> into Shadow DOM.

It would be the first instance of Shadow DOM usage in chrome, we will be able to see what breaks.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1454357 - Create BaseControlMixin, allowing BaseControl to inherit other native XULElement classes

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1454357 - (Broken) Convert scrollbox to custom element

This does what's intended, moving scrollbox to a custom element implementation

The debug browser will fail to launch with

Assertion failure: !content->IsActiveChildrenElement(), at /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:3033

and the non-debug browser will not render the tabs at all.

With the inspector I can see non-XBL instance of <scrollbox> can be constructed correctly. We'll need to figure out how
XBL anonymous content and Shadow DOM slots could work together.

Depends on D14925

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The other approach I am exploring right now is to remove the anonymous content altogether.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4510107ce3633d22237da1170f6819a266f2eac&selectedJob=217805141

Looks like it's ... manageable? Correcting _scrollbox.boxObject.height and add padding into it should make the failures go away.

I am running mozscreenshots right now to see if there are uncaught changes.

https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=1b90cc2ca758a0c05323b4da4bf175fae6041ea8&newProject=try&newRev=5cee57b9320767b9c8ed5f57deb4256119a79d63

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #6)
> Created attachment 9032307 [details]
> Bug 1454357 - (Broken) Convert scrollbox to custom element
> 
> This does what's intended, moving scrollbox to a custom element
> implementation
> 
> The debug browser will fail to launch with
> 
> Assertion failure: !content->IsActiveChildrenElement(), at
> /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:3033
> 
> and the non-debug browser will not render the tabs at all.
> 
> With the inspector I can see non-XBL instance of <scrollbox> can be
> constructed correctly. We'll need to figure out how
> XBL anonymous content and Shadow DOM slots could work together.

Even if we end up being able to remove the anonymous node in this case (as per Comment 7), I'd like to get a bug on file about this issue since it's going to prevent us from using SD in cases where it's not possible to remove the anonymous content (stuff like <wizard> as an example).

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Even if we end up being able to remove the anonymous node in this case (as
> per Comment 7), I'd like to get a bug on file about this issue since it's
> going to prevent us from using SD in cases where it's not possible to remove
> the anonymous content (stuff like <wizard> as an example).

Yeah. I am fairly sure I can come up with a test case for this.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #9)
> Yeah. I am fairly sure I can come up with a test case for this.

Please see test case and bug at bug 1515518.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The patch in comment 7 breaks bug 490178, i.e. <richlistbox dir="reverse">. For some reasons without the <box> as anonymous content, the direction of the list doesn't really update sometimes.

For that, I am more leaning toward removing the feature altogether since it is not used.

I am still looking at other test failures.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

It also breaks the assumed frame tree expected by XULScrollElement::ScrollByIndex() I think. I am trying to figure out a fix.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1454357 - Remove the feature to set list direction on a rishlistbox

This undo the change implemented in bug 490178 (f8f1a9113a1c) because there is no user of this feature.

For some reason when scrollbox has its inner box removed, XUL doesn't really respect the direction set, and the rendering of the items (now the direct children of scrollbox) will be incorrect.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1454357 - Remove the scrollbox binding

With all the previous efforts, the scrollbox binding now does nothing but to create an inner box element holding the scrolling content.

It turned out that inner box be easily removed. The padding set by the document sheets can be moved to the srollbox element directly.

The only gatcha is XULScrollElement::ScrollByIndex() -- it can now reach the child item frames directly from the scrolled frame.

Depends on D15169

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 17

[:Paolo Amadini]

Note that I'm removing the last consumer of XULScrollElement::ScrollByIndex in bug 1454360.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Cool, if so removing the method makes more sense.

Comment 19

[:Paolo Amadini]

Since you're touching the scrollbox styling anyways, you may also consider converting scrollbox uses to simple boxes and remove styling dependencies on the "scrollbox" tag. I'm removing XULScrollElement in bug 1516258, although "richlistbox" was already the only remaining consumer after bug 1454360.

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Sure, I'll wait for dependencies to settle.

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Rebased.

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

Let's uncomplicate things by breaking up the dependency —- I believe this and bug 1516258 can land independently (and easily rebased on top of).

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e99aed6c14d
Remove the scrollbox binding r=NeilDeakin

Comment 23

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/3e99aed6c14d

Comment 24

[veron]

«toolkit.tabbox.switchByScrolling",true» not completely working correctly. Moving between tabs with the mouse wheel works only in the visibility zone, when there are a lot of open bookmarks and goes beyond the navigation panel, the function stops working. It is necessary that the mouse wheel works on all tabs and it doesn’t matter how much is open in the navigation bar.
Please fix it.

Comment 25

[veron]

Forgot to add, I'm using FF 71 x64

Comment 26

[Brian Grinstead [:bgrins]]

(In reply to veron from comment #24)

«toolkit.tabbox.switchByScrolling",true» not completely working correctly. Moving between tabs with the mouse wheel works only in the visibility zone, when there are a lot of open bookmarks and goes beyond the navigation panel, the function stops working. It is necessary that the mouse wheel works on all tabs and it doesn’t matter how much is open in the navigation bar.
Please fix it.

This is tracked in Bug 1601184.