493304 - Scrolling more than necessary to bottom needs hidden upscroll until pane scrolls again

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b5pre) Gecko/20090513 Calendar/1.0pre

related to bug 67752?

STR:
1. Scroll to the bottom of the day pane and keep scrolling down.
2. Try to scroll up.

Actual result:
You have to scroll your mouse wheel some time (seems the same amount like the one you wasted downwards) until the pane moves again.

Comment 1

[Stefan Sitter [:ssitter]]

archaeopteryx, could you retest with the archived nightly builds from <http://ftp.mozilla.org/pub/mozilla.org/calendar/sunbird/nightly/2009/> to narrow down the regression range?

Comment 2

[Stefan Sitter [:ssitter]]

(In reply to comment #0)
> related to bug 67752?

Looks like the patch has been checked in only to mozilla-central repository but not the mozilla-1.9.1 repository. Therefore Sunbird 1.0pre or Thunderbird 3.0* builds should not be affected by the changes.

Comment 3

[Decathlon]

Attachment: proposal

This bug seems caused by this code line [1] inside method "scrollToMinute" in calendar-multiday-view.xml file:

   this.mFirstVisibleMinute = aMinute;

that, along with this one [2] inside "DOMMouseScroll" handler,

  this.scrollToMinute(this.mFirstVisibleMinute +
                                (event.detail < 0 ? -60 : 60));
                                

makes become 'mFirstVisibleMinute' more and more bigger (positive and negative values) every time the mouse wheel is used after the view has reached the upper/lower limit. Therefore, when you change scroll direction with the mouse wheel, 'mFirstVisibleMinute' needs to decrease until the limit value before starting to scroll the view.

Since 'mFirstVisibleMinute' is automatically set (to the right value) by the handler "scroll" when the view is scrolled, I think that it can be forced to the 'aMinute' value only when the 'if' statement above that code line isn't true.
Maybe, generally speaking, it would need also a boundaries check because it should be something like:

  0 < aMinute < (24*60 - minutes_really_displayed_on_the_view)   

Philipp, original code is your patch from bug 462278 http://hg.mozilla.org/comm-central/rev/6ada4b8913e6#l3.285, you know what it needs here.

[1] http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-multiday-view.xml#3230

[2] http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-multiday-view.xml#3303

Comment 4

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

Comment on attachment 406499 [details] [diff] [review]
proposal

Putting this in the else block is probably not the right solution. This means that mFirstVisibleMinute will only be set if the view has not been set up correctly, i.e scrollBoxObject is null or scrollHeight is 0.

I think in the bug you mentioned I needed to do that so that if you end the application and then start it again, the view is scrolled at the right position.

I'd suggest to keep setting mFirstVisibleMinute unconditionally and adding Range checks as you suggested with Math.min/max.

Comment 5

[Decathlon]

Attachment: patch_v1

(In reply to comment #4)
> 
> I'd suggest to keep setting mFirstVisibleMinute unconditionally and adding
> Range checks as you suggested with Math.min/max.

I've done in this way.

I've also considered a little problem that happens when there are events that start next to 0:00 (example: 23:45) and the size of the hours showed in the view is small because there are many of them (let's say more than 12-15 hours but it also depends on screen/window size). In this case those events go out the 0-24 range because they have a minimum height, given by the font size, that doesn't fit the remaining time from start to 23:59.

To show, at least, the summary of these events, the view should scroll a bit more than 23:59 but the amount isn't a constant value.
To fix also this issue I've added an extra hour to figure out the minutes in the view: 25 instead of 24. In my opinion one hour is enough for common cases, but less than this (e.g. 30 minutes) might be too little.

Although, in this way, when there aren't such events, it must scroll one more time the well (only one) before getting the scroll. Is it a tolerable issue or it's better to solve the problem (if it worths) in a different manner and use a 24 hour limit to figure out the minutes to scroll?

Comment 6

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

Comment on attachment 409780 [details] [diff] [review]
patch_v1

Works like a charm, thanks for the patch

r=philipp

Comment 7

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

> Although, in this way, when there aren't such events, it must scroll one more
> time the well (only one) before getting the scroll. Is it a tolerable issue or
> it's better to solve the problem (if it worths) in a different manner and use a
> 24 hour limit to figure out the minutes to scroll?

I think this is tolerable, we can fix this later in case we are bored ;-)

Comment 8

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

I'll check this in later today. If someone else decides to, don't forget to checkin on both c-c and c-191.

Comment 9

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

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/7411890861f4>
and comm-1.9.1 rev e86c91373d53

-> FIXED

Comment 10

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.10pre) Gecko/20100323 Calendar/1.0b2pre