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

VERIFIED FIXED in 1.0b1

Status

Calendar
Calendar Views
--
minor
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: aryx, Assigned: Decathlon)

Tracking

({regression})

Trunk
1.0b1
regression

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.36 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
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

8 years ago
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?
Keywords: regressionwindow-wanted

Comment 2

8 years ago
(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.
(Assignee)

Comment 3

8 years ago
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
Attachment #406499 - Flags: review?(philipp)
Assignee: nobody → bv1578
Status: NEW → ASSIGNED
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.
Attachment #406499 - Flags: review?(philipp) → review-
(Assignee)

Comment 5

8 years ago
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?
Attachment #406499 - Attachment is obsolete: true
Attachment #409780 - Flags: review?(philipp)
Comment on attachment 409780 [details] [diff] [review]
patch_v1

Works like a charm, thanks for the patch

r=philipp
Attachment #409780 - Flags: review?(philipp) → review+
> 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 ;-)
Keywords: regressionwindow-wanted
OS: Windows XP → All
Hardware: x86 → All
I'll check this in later today. If someone else decides to, don't forget to checkin on both c-c and c-191.
Keywords: checkin-needed
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/7411890861f4>
and comm-1.9.1 rev e86c91373d53

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.0

Comment 10

7 years ago
verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.10pre) Gecko/20100323 Calendar/1.0b2pre
Status: RESOLVED → VERIFIED

Updated

7 years ago
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.