1520902 - Port bug 1516258 - Remove XULScrollElement

Status: RESOLVED INVALID

(Thunderbird :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

As per bug 1516258 comment #4:

scrollbox.ensureElementIsVisible(element);

should be replaced with:

element.scrollIntoView({ block: "nearest" });

https://searchfox.org/comm-central/search?q=ensureElementIsVisible&path=

Comment 1

[Jorg K (CEST = GMT+2)]

OK, looking at the query result we have:
common/bindings/richlistbox.xml
mail/base/content/mailWidgets.xml
That's the forked richlistbox and an extension for attachments. So these won't need to change.

Then we have call sites in calendar/ which most likely use the xbl-richlistbox. Then we have common/bindings/richlistbox.xml, which is the forked richlistbox, now called xbl-richlistbox.

Then we have call sites in mail/ and mailnews/ which look like the use some sort of list, and then there is tabmail.xml which defined an extension of scrollbox.xml, that will need to change, too.

Comment 2

[Jorg K (CEST = GMT+2)]

Hold on, browser/base/content/tabbrowser.xml had
409 this.arrowScrollbox.ensureElementIsVisible(selectedTab, aInstant);
and that wasn't replaced in M-C.

Comment 3

[Jorg K (CEST = GMT+2)]

Well, we have five call sites in messengerLanguages.js, but M-C haven't touched their equivalent browserLanguages.js. I'm confused :-(

Comment 4

[:aceman]

I think arrowscrollbox is different object from scrollbox, but was sometimes used as a replacement for it.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1520902-scrollbox.patch

So this changes 8 call sites and does not include tabmail.xml or messengerLanguages.js since the M-C equivalents didn't change.

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 9037686 [details] [diff] [review]
1520902-scrollbox.patch

Review of attachment 9037686 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine to me, and Comment 4 sounds plausible although I can't say for sure. Paolo can clarify in Bug 1516258.

Comment 7

[Jorg K (CEST = GMT+2)]

Hmm, Aceman tells me that CE richlistbox has an ensureElementIsVisible() method, that's why you didn't touch browserLanguages.js. As far as I can see, I've only touched CE richlistbox here, so I think the entire patch is obsolete.

Comment 8

[:aceman]

Comment on attachment 9037686 [details] [diff] [review]
1520902-scrollbox.patch

Review of attachment 9037686 [details] [diff] [review]:
-----------------------------------------------------------------

I have built with the m-c patch in bug 1516258 and I haven't seen any problems.
E.g. the  addTagCallback() in mail/components/preferences/display.js runs and the tagListBox.ensureElementIsVisible(item); that you touch work without any change.
I have also run all mozmill tests and there were no errors about ensureElementIsVisible not being defined.

It seems this patch isn't needed for now.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 9037686 [details] [diff] [review]
1520902-scrollbox.patch

Looking at bug 1516258 comment #16 this patch is unnecessary.

However, looks like our forked richlistbox needs a tweak. I'll file a new bug.

Comment 10

[Jorg K (CEST = GMT+2)]

I think when we forked richlistbox.xml in bug 1517040 we copied the wrong version :-( - I filed bug 1521309 to correct that.