Closed Bug 1454360 Opened 3 years ago Closed 2 years ago

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


(Core :: XUL, task, P1)




Tracking Status
firefox66 --- fixed


(Reporter: Paolo, Assigned: Paolo)




(1 file, 1 obsolete file)

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.
Blocks: 1454363
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.

The instance in the devtools tests seems to be an "arrowscrollbox" too:
This code is tested by "test_menulist.xul" which still passes locally. Full tryserver build:
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 on attachment 8968189 [details]
Bug 1454360 - Don't use the scrollByIndex method in the "popup-scrollbars" binding.

::: toolkit/content/widgets/popup.xml:745
(Diff revision 1)
>            }
>            if (this._draggingState == this.DRAG_OVER_POPUP &&
>                (event.screenY <= || event.screenY >= popupRect.bottom)) {
>              let scrollAmount = event.screenY <= ? -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.
Attachment #8968189 - Flags: review?(gijskruitbosch+bugs)
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.
Priority: -- → P5
Moving to Core:XUL per
Component: XP Toolkit/Widgets: XUL → XUL
This method is on XULScrollElement now.
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.
No longer blocks: 1454358
Depends on: 1454358
This prepares this binding for the unification with the "popup" binding, and removes the last consumer of the scrollByIndex method of XULScrollElement.
Blocks: 1454357
Attachment #8968189 - Attachment is obsolete: true
Blocks: 1516258
Assignee: nobody → paolo.mozmail
Summary: Don't use the scrollByIndex method in the "popup-scrollbars" binding → Use "arrowscrollbox" in the "popup-scrollbars" binding
Priority: P5 → P1
Pushed by
Use "arrowscrollbox" in the "popup-scrollbars" binding. r=NeilDeakin
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Type: enhancement → task
Regressions: 1562557
You need to log in before you can comment on or make changes to this bug.