1454360 - Use "arrowscrollbox" in the "popup-scrollbars" binding

Status: RESOLVED FIXED

(Core :: XUL, task, P1)

Description

[:Paolo Amadini]

The scrollByIndex method of ScrollBoxObject is used by the "popup-scrollbars" binding to provide drag-to-scroll behavior in the popup of HTML <select> elements and XUL <menulist> elements.

This can be replaced with other ways of scrolling the content.

Comment 1

[:Paolo Amadini]

This seems to be the last consumer of the scrollByIndex method of ScrollBoxObject. There are other calls to the scrollByIndex method of the "arrowscrollbox" binding, that doesn't call the method of the same name on the boxObject.

https://dxr.mozilla.org/mozilla-central/search?q=scrollByIndex

The instance in the devtools tests seems to be an "arrowscrollbox" too:

https://dxr.mozilla.org/mozilla-central/rev/e96685584bf7d3c1d7a4c1861716da89fd650c51/devtools/client/shared/widgets/BreadcrumbsWidget.jsm#33

Comment 2

[:Paolo Amadini]

Attachment: Bug 1454360 - Don't use the scrollByIndex method in the "popup-scrollbars" binding.

This changes the amount of scrolling done when dragging the pointer vertically outside of the area of <select> elements and XUL <menulist> elements, keeping it close to the current value based on the height of the contained elements.

Review commit: https://reviewboard.mozilla.org/r/236870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236870/

Comment 3

[:Paolo Amadini]

This code is tested by "test_menulist.xul" which still passes locally. Full tryserver build:

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

Comment 4

[:Paolo Amadini]

Note that I also considered measuring the height of the first "menuitem" element synchronously to determine the scroll amount, but that just seems unnecessary for a probably quite rarely used feature.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8968189 [details]
Bug 1454360 - Don't use the scrollByIndex method in the "popup-scrollbars" binding.

https://reviewboard.mozilla.org/r/236870/#review242706

::: toolkit/content/widgets/popup.xml:745
(Diff revision 1)
>            }
>  
>            if (this._draggingState == this.DRAG_OVER_POPUP &&
>                (event.screenY <= popupRect.top || event.screenY >= popupRect.bottom)) {
>              let scrollAmount = event.screenY <= popupRect.top ? -1 : 1;
> -            this.scrollBox.scrollByIndex(scrollAmount);
> +            this.scrollBox.scrollTop += this.AUTOSCROLL_DISTANCE * scrollAmount;

This behaves very strangely when the number you picked (16) is off-by-one (or a few) from the 'real' line height (like it apparently is on my mac) - the scrolling is very quick, but the amount of scroll is "just off", making the lines appear to scroll very slowly, though the scrollbar scrolls very quickly.

If we're going to remove the binding anyway, I'm not sure how valuable it is to fix this bug separately. Otherwise, I would suggest determining the height once (I guess synchronously because it's not clear how to do it async without introducing re-entrancy and all kinds of issues?) and caching it while the popup stays open (clear on popuphiding, for which there's already a handler). In fact, could potentially do a lazy async read of the line height onpopupshowing, which means it'll be ready by the time we hit this anyway.

Comment 6

[:Paolo Amadini]

In fact, even measuring the "menuitem" box height gives a slightly off result for me.

If we want to keep the alignment when scrolling, we have to figure out how to correct for this, and also if the first item is hidden we have to iterate until we find the first one that is visible. We may also reimplement what the scrollByIndex implementation in ScrollBoxObject does, that is basically a linear scan of the items to identify which one is the top one, and then scrolling to the next one.

Comment 7

[Firefox Bug Husbandry Bot]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

This method is on XULScrollElement now.

Comment 9

[:Paolo Amadini]

It still makes sense to keep this bug open to remove the code, this can help with the plan I outlined in bug 1454358 comment 1 to remove the "scrollbox" element eventually.

Comment 10

[:Paolo Amadini]

Attachment: Bug 1454360 - Use "arrowscrollbox" in the "popup-scrollbars" binding. r=NeilDeakin

This prepares this binding for the unification with the "popup" binding, and removes the last consumer of the scrollByIndex method of XULScrollElement.

Comment 11

[:Paolo Amadini]

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

Comment 12

[:Paolo Amadini]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24b9b8cb6c61
Use "arrowscrollbox" in the "popup-scrollbars" binding. r=NeilDeakin

Comment 14

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/24b9b8cb6c61