Last Comment Bug 493304 - Scrolling more than necessary to bottom needs hidden upscroll until pane scrolls again
: Scrolling more than necessary to bottom needs hidden upscroll until pane scro...
Status: VERIFIED FIXED
: regression
Product: Calendar
Classification: Client Software
Component: Calendar Views (show other bugs)
: Trunk
: All All
: -- minor (vote)
: 1.0b1
Assigned To: Decathlon
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-15 15:07 PDT by Sebastian Hengst [:aryx][:archaeopteryx]
Modified: 2010-03-24 10:45 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposal (1.11 KB, patch)
2009-10-15 11:48 PDT, Decathlon
philipp: review-
Details | Diff | Splinter Review
patch_v1 (1.36 KB, patch)
2009-11-02 13:04 PST, Decathlon
philipp: review+
Details | Diff | Splinter Review

Description Sebastian Hengst [:aryx][:archaeopteryx] 2009-05-15 15:07:24 PDT
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 2009-08-04 10:05:51 PDT
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 2009-08-04 10:09:25 PDT
(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 2009-10-15 11:48:27 PDT
Created attachment 406499 [details] [diff] [review]
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] 2009-11-02 01:38:52 PST
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 2009-11-02 13:04:04 PST
Created attachment 409780 [details] [diff] [review]
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] 2009-11-05 05:10:03 PST
Comment on attachment 409780 [details] [diff] [review]
patch_v1

Works like a charm, thanks for the patch

r=philipp
Comment 7 Philipp Kewisch [:Fallen] 2009-11-05 05:10:45 PST
> 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] 2009-11-05 05:11:58 PST
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] 2009-11-05 11:20:09 PST
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/7411890861f4>
and comm-1.9.1 rev e86c91373d53

-> FIXED
Comment 10 Damian Szczepanik 2010-03-24 09:01:46 PDT
verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.10pre) Gecko/20100323 Calendar/1.0b2pre

Note You need to log in before you can comment on or make changes to this bug.