1516258 - Remove XULScrollElement

Status: RESOLVED FIXED

(Core :: XUL, enhancement, P1)

Description

[:Paolo Amadini]

After bug 1454360, XULScrollElement can be easily removed.

Comment 1

[:Paolo Amadini]

Attachment: Bug 1516258 - Part 2 - Remove XULScrollElement. r=NeilDeakin

The only method of XULScrollElement that is still used is in "richlistbox", and can be easily reimplemented. There is still styling associated with the "scrollbox" element, but eventually those instances can be replaced with simple boxes.

Comment 2

[:Paolo Amadini]

Attachment: Bug 1516258 - Part 1 - Stop using XULScrollElement::EnsureElementIsVisible. r=bgrins

Comment 3

[:Paolo Amadini]

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

Comment 4

[Brian Grinstead [:bgrins]]

Jorg, please see https://phabricator.services.mozilla.com/D16840. Calls to:

scrollbox.ensureElementIsVisible(element);

should be replaced with:

element.scrollIntoView({ block: "nearest" });

comm-central has callers at: https://searchfox.org/comm-central/search?q=ensureElementIsVisible&path=
There's a replacement for ensureElementIsVisible (used in comm-central at): https://searchfox.org/comm-central/search?q=ensureElementIsVisible&path=

Comment 5

[Brian Grinstead [:bgrins]]

Paolo, why block: "nearest"? I don't actually see any documentation about what that means in https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView.

Comment 6

[Jorg K (CEST = GMT+2)]

Thanks, I filed bug 1520902. Something went wrong in comment #4:

(In reply to Brian Grinstead [:bgrins] from comment #4)

comm-central has callers at: https://searchfox.org/comm-central/search?q=ensureElementIsVisible&path=
There's a replacement for ensureElementIsVisible (used in comm-central at): https://searchfox.org/comm-central/search?q=ensureElementIsVisible&path=

That's the same link twice, and I thought the replacement was element.scrollIntoView({ block: "nearest" });. Can you please explain.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Jorg K (GMT+1) from comment #6)

Thanks, I filed bug 1520902. Something went wrong in comment #4:

(In reply to Brian Grinstead [:bgrins] from comment #4)

comm-central has callers at: https://searchfox.org/comm-central/search?q=ensureElementIsVisible&path=
There's a replacement for ensureElementIsVisible (used in comm-central at): https://searchfox.org/comm-central/search?q=ensureElementIsVisible&path=

That's the same link twice, and I thought the replacement was element.scrollIntoView({ block: "nearest" });. Can you please explain.

Oh, that was a typo. The callers at https://searchfox.org/comm-central/search?q=ensureElementIsVisible&path= should be replaced with element.scrollIntoView({ block: "nearest" }).

Comment 8

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #5)

Paolo, why block: "nearest"?

If this parameter is omitted, the element would be placed at the top, even if it was out of view at the bottom. The comment before ScrollIntoViewOptions points here:

https://drafts.csswg.org/cssom-view/#extension-to-the-element-interface

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/364d6aef9d05
Part 1 - Stop using XULScrollElement::EnsureElementIsVisible. r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b59993a9659
Part 2 - Remove XULScrollElement. r=NeilDeakin

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to Brian Grinstead [:bgrins] from comment #7)

Oh, that was a typo. The callers at https://searchfox.org/comm-central/search?q=ensureElementIsVisible&path= should be replaced with element.scrollIntoView({ block: "nearest" }).

Sadly it's not that easy. We have two bindings which define ensureElementIsVisible, one being the forked richlistbox. So consumers won't have to change for now. From DXR/Searchfox it's not obvious which should be replaced and which shouldn't.

Comment 11

[Jorg K (CEST = GMT+2)]

I'm seriously confused as to which call sites need replacement. Clearly you haven't touched browserLanguages.js which has five ensureElementIsVisible() calls. We have equivalent code in C-C.

Comment 12

[:aceman]

Is this also killing <scrollbox> element as in https://searchfox.org/comm-central/source/common/bindings/richlistbox.xml#19 ?

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to :aceman from comment #12)

Is this also killing <scrollbox> element as in https://searchfox.org/comm-central/source/common/bindings/richlistbox.xml#19 ?

I'm not sure - but I thought that instance would have been removed by Bug 1472557.

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/364d6aef9d05
https://hg.mozilla.org/mozilla-central/rev/2b59993a9659

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/364d6aef9d05
https://hg.mozilla.org/mozilla-central/rev/2b59993a9659

Comment 16

[:Paolo Amadini]

(In reply to Jorg K (GMT+1) from comment #11)

I'm seriously confused as to which call sites need replacement. Clearly you haven't touched browserLanguages.js which has five ensureElementIsVisible() calls. We have equivalent code in C-C.

Yes, we have similarly named methods on the "richlistbox" Custom Element and the "arrowscrollbox" XBL binding:

https://searchfox.org/mozilla-central/rev/dac799c9f4e9f5f05c1071cba94f2522aa31f7eb/toolkit/content/widgets/richlistbox.js#548
https://searchfox.org/mozilla-central/rev/dac799c9f4e9f5f05c1071cba94f2522aa31f7eb/toolkit/content/widgets/scrollbox.xml#282

The calls affected by this patch are only the ones on the "scrollbox" element itself. There are two more interface methods on XULScrollElement, but all the callers were already removed from mozilla-central in bug 1472557, in particular you can see how this call in "richlistbox" was replaced:

this._scrollbox.scrollToElement(item);