1476639 - Remove scrollToIndex

Status: RESOLVED FIXED

(Toolkit :: XUL Widgets, task, P5)

Description

[:Paolo Amadini]

With bug 1472748 and bug 1472555 landed, there is only one test file left using the scrollToIndex method. The test can be changed, allowing the removal of the scrollToElement method as well.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Is there a replacement recommendation for scrollToIndex? This is used a few times in c-c.

Comment 2

[Jorg K (CEST = GMT+2)]

Filed bug 1476753 for TB.

Comment 3

[:Paolo Amadini]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #1)
> Is there a replacement recommendation for scrollToIndex? This is used a few
> times in c-c.

Yes, with "richlistbox" you can use the usual DOM and CSS methods like scrollTop and getBoundingClientRect.

Comment 4

[Brian Grinstead [:bgrins]]

Is this referring only to the scrollToIndex method in richlistbox.js or also nsListControlFrame::ScrollToIndex? I see a number of callers to the latter so I'm assuming only the former?

Comment 5

[:Paolo Amadini]

Yes, just the richlistbox.js method. Also, scrollToElement was already removed elsewhere in the meantime, so the fix only involves inlining the method call inside the scroll test:

https://searchfox.org/mozilla-central/search?q=scrollToIndex&case=true

Comment 6

[Shrey Gupta]

Hello,
I would like to work on this. Could you please what exactly has to be done as it is not clear.
Thank you

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Shrey Gupta from comment #6)

Hello,
I would like to work on this. Could you please what exactly has to be done as it is not clear.
Thank you

Hi, here is an outline of what needs to be done:

1. Remove this function https://searchfox.org/mozilla-central/rev/cfd1cc461f1efe0d66c2fdc17c024a203d5a2fd8/toolkit/content/widgets/richlistbox.js#598.
2. Run ./mach build faster and then ./mach mochitest toolkit/content/tests/chrome/test_mousescroll.xhtml. This should fail.
3. There are two places in the test that use the helper function. Here https://searchfox.org/mozilla-central/rev/cfd1cc461f1efe0d66c2fdc17c024a203d5a2fd8/toolkit/content/tests/chrome/test_mousescroll.xhtml#106 and here https://searchfox.org/mozilla-central/source/toolkit/content/tests/chrome/test_mousescroll.xhtml#130. Both of those calls should be replaced with what that function was doing, which should translate into something like listbox.ensureElementIsVisible(listbox.getItemAtIndex(aStart), true) in that test. After changing the test you can re-run ./mach mochitest toolkit/content/tests/chrome/test_mousescroll.xhtml and confirm it's working.

Comment 8

[jcadler]

Attachment: Bug 1476639 - Remove scrollToIndex Function on richlistbox

Comment 9

[jcadler]

Attachment: Bug 1476639: remove scrollToIndex

Depends on D64198

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c572e0307a58
Remove scrollToIndex Function on richlistbox r=bgrins

Comment 11

[Phabricator Automation]

Comment on attachment 9128931 [details]
Bug 1476639 - Remove scrollToIndex Function on richlistbox

Revision D64198 was moved to bug 1609821. Setting attachment 9128931 [details] to obsolete.

Comment 12

[Phabricator Automation]

Comment on attachment 9128931 [details]
Bug 1476639 - Remove scrollToIndex Function on richlistbox

Revision D64198 was moved to bug 1609821. Setting attachment 9128931 [details] to obsolete.

Comment 13

[Jorg K (CEST = GMT+2)]

You're going to remove scrollToElement in another bug? If so, please let the Thunderbird folks know.

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/c572e0307a58