1470371 - Port |Bug 1454358 - Remove ScrollBoxObject|

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1469014 +++

ScrollBoxObject is going away, so we need to find a replacement, see bug 1454358 comment #22.

Comment 1

[Jorg K (CEST = GMT+2)]

Bug 1454358 has landed and there are usages in mail/ and calendar/. I'll leave the Calendar to Geoff.

https://dxr.mozilla.org/comm-central/search?q=scrollBoxObject&redirect=false

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1470371-ScrollBoxObject.patch for mail/

Looks like we ignored this until it got too late assuming only Calendar is affected.

I'm really confused by https://hg.mozilla.org/mozilla-central/rev/3af5036936c1 and can't work out what needs to be done here.

So let's go with this as a bustage fix attempt. This is done purely by pattern matching without any understanding whatsoever :-(

this.arrowScrollbox.boxObject is used here:
https://dxr.mozilla.org/comm-central/rev/a6cd3f2e51c9d878310691599ab36edf76a93460/mail/base/content/tabmail.xml#2171
so that can't be far off to use is instead of .scrollBoxObject.

arrowScrollbox.ensureElementIsVisible() is used here:
https://dxr.mozilla.org/comm-central/rev/a6cd3f2e51c9d878310691599ab36edf76a93460/mail/base/content/tabmail.xml#2156
so I've just removed the .scrollBoxObject.

Help welcome!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e5e1510b8d91
Port bug 1454358: Replace use of ScrollBoxObject in mail/. rs=bustage-fix DONTBUILD

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8991824 [details] [diff] [review]
1470371-ScrollBoxObject.patch for mail/

r=me as far as it goes, but you should look at all the things removed from ScrollBoxObject and see if mailnews uses any of them.

Calendar certainly does (e.g. search calendar code for positionX), but I dunno whether you care.

Comment 5

[Jorg K (CEST = GMT+2)]

Thanks, Boris. Calendar uses positionX on the same line as scrollBoxObject and our new staff member Geoff will look into that.

As I said, I'm really confused by https://hg.mozilla.org/mozilla-central/rev/3af5036936c1 and all the replacements made there.

I removed all scrollBoxObject in C-C apart from Calendar (and suite/) and looked for |boxObject.scrollByIndex| and |boxObject.ensureElementIsVisible| (no matches) as per bug 1454358 comment #22.

Now I've also checked all the removed methods and attributes from ScrollBoxObject.webidl:
scrollTo, scrollBy, scrollByIndex, scrollToElement, positionX, positionY, scrolledWidth, scrolledHeight and only found remaining use in Calendar. window.scrollTo(), view.scrollBy() must be something else.

ensureElementIsVisible() is a special beast as it seems to apply to various elements, but I think none of them are scrollBoxObjects.

FRG, there are scrollBoxObjects in suite/
https://dxr.mozilla.org/comm-central/search?q=scrollBoxObject&redirect=false

Comment 6

[Boris Zbarsky [:bzbarsky]]

What's the confusing part?  Some methods and properties got removed (the ones unused in m-c or that could be replaced with other APIs).  Others moved from the boxobject to the element.  Then the ScrollBoxObject API was removed.

Comment 7

[Jorg K (CEST = GMT+2)]

Well, it's a large patch that wasn't split into logical units.

Why for example this change:
-    const boxObject = this._list.boxObject;
-    boxObject.ensureElementIsVisible(aElement);
-    boxObject.scrollBy(-this._list.clientWidth, 0);
+    const scrollbox = this._list;
+    scrollbox.ensureElementIsVisible(aElement);
+    scrollbox.scrollBy(-this._list.clientWidth, 0);

when boxObject seems to be an attribute of some element. I'm using it in my patch which you approved.

And then there is the confusing _scrollbox like here, also using .boxObject.
-              this.scrollBoxObject.scrollBy(0, this.scrollBoxObject.height * aDirection);
+              this._scrollbox.scrollBy(0, this._scrollbox.boxObject.height * aDirection);

I didn't work through all the details of the M-C bug, but I can't see a clear pattern. Remove .boxObject, add .boxObject, use _scrollbox. :-(

Comment 8

[Frank-Rainer Grahl (:frg)]

> FRG, there are scrollBoxObjects in suite/

Thanks. Put it in the comm-central SeaMonkey bug 1452448 for later fixing. Given the current dev resources and bugs piling up probably very later but that is another story :(

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Why for example this change:

Because the ensureElementIsVisible and scrollBy APIs moved from the boxobject to the element.

> I didn't work through all the details of the M-C bug, but I can't see a clear pattern.

The pattern is "use the element instead of the boxobject".  How one gets the element depends on the exact code.  Note that all those changes weren't even reviewed by my, so it might have made more sense to ask their reviewer to review the c-c equivalents....

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1470371-scrollbox patch for calendar/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d88a6041b55f
Port bug 1454358: Replace use of ScrollBoxObject in calendar/. r=philipp

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8991824 [details] [diff] [review]
1470371-ScrollBoxObject.patch for mail/

Cancel that review. We have r=bz and no problems reported.