Port |Bug 1454358 - Remove ScrollBoxObject|

RESOLVED FIXED in Thunderbird 63.0

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: jorgk, Unassigned)

Tracking

Trunk
Thunderbird 63.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

+++ 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.
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
Flags: needinfo?(geoff)
Keywords: leave-open
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!
Attachment #8991824 - Flags: review?(bzbarsky)
Attachment #8991824 - Flags: review?(acelists)
Product: Calendar → Thunderbird
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e5e1510b8d91
Port bug 1454358: Replace use of ScrollBoxObject in mail/. rs=bustage-fix DONTBUILD
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.
Attachment #8991824 - Flags: review?(bzbarsky) → review+
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
Flags: needinfo?(frgrahl)
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.
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. :-(
> 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 :(
Flags: needinfo?(frgrahl)
> 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....
Flags: needinfo?(geoff)
Attachment #8992120 - Flags: review?(philipp)
Attachment #8992120 - Flags: review?(philipp) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d88a6041b55f
Port bug 1454358: Replace use of ScrollBoxObject in calendar/. r=philipp
Status: NEW → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 63.0
Comment on attachment 8991824 [details] [diff] [review]
1470371-ScrollBoxObject.patch for mail/

Cancel that review. We have r=bz and no problems reported.
Attachment #8991824 - Flags: review?(acelists)
You need to log in before you can comment on or make changes to this bug.