Closed Bug 1476639 Opened 2 years ago Closed 1 month ago

Remove scrollToIndex

Categories

(Toolkit :: XUL Widgets, task, P5)

task

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox63 --- wontfix
firefox75 --- fixed

People

(Reporter: Paolo, Assigned: jcadler)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 1 obsolete file)

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.
Priority: -- → P5
Is there a replacement recommendation for scrollToIndex? This is used a few times in c-c.
Depends on: 1476753
Filed bug 1476753 for TB.
(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.
Type: enhancement → task

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?

Flags: needinfo?(paolo.mozmail)

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

Flags: needinfo?(paolo.mozmail)
Keywords: good-first-bug
Summary: Remove scrollToIndex and scrollToElement → Remove scrollToIndex

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

(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.
Assignee: nobody → jcadler
Status: NEW → ASSIGNED
Attached file Bug 1476639: remove scrollToIndex (obsolete) —

Depends on D64198

Attachment #9128957 - Attachment is obsolete: true
Attachment #9128931 - Attachment description: Bug 1476639: Remove scrollToIndex → Bug 1476639: Remove scrollToIndex Function Fi
Attachment #9128931 - Attachment description: Bug 1476639: Remove scrollToIndex Function Fi → Bug 1476639: Remove scrollToIndex Function on richlistbox
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c572e0307a58
Remove scrollToIndex Function on richlistbox r=bgrins

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.

Attachment #9128931 - Attachment is obsolete: true
Attachment #9128931 - Attachment is obsolete: false

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.

Attachment #9128931 - Attachment is obsolete: true
Attachment #9128931 - Attachment description: Bug 1476639: Remove scrollToIndex Function on richlistbox → Bug 1476639 - Remove scrollToIndex Function on richlistbox
Attachment #9128931 - Attachment is obsolete: false

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

Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
You need to log in before you can comment on or make changes to this bug.