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

RESOLVED FIXED in Firefox 66

Status

()

task
P1
normal
RESOLVED FIXED
Last year
17 days ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks 1 bug)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.

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
This code is tested by "test_menulist.xul" which still passes locally. Full tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bfcb916475634c3df0c69b2c6632d47e53ca1e8
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.

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.
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 https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
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
Status: NEW → ASSIGNED
Summary: Don't use the scrollByIndex method in the "popup-scrollbars" binding → Use "arrowscrollbox" in the "popup-scrollbars" binding
Priority: P5 → P1
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24b9b8cb6c61
Use "arrowscrollbox" in the "popup-scrollbars" binding. r=NeilDeakin
Status: ASSIGNED → RESOLVED
Closed: 6 months 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.