Last Comment Bug 782152 - Replace DOMMouseScroll with wheel event
: Replace DOMMouseScroll with wheel event
Status: RESOLVED FIXED
[good first bug][mentor=Fallen][lang=js]
:
Product: Calendar
Classification: Client Software
Component: Calendar Views (show other bugs)
: unspecified
: All All
: -- normal (vote)
: 2.1
Assigned To: zeyu [:car]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-12 14:18 PDT by Philipp Kewisch [:Fallen]
Modified: 2012-10-21 04:38 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
replace DOMMouseScroll with wheel event (9.67 KB, patch)
2012-09-06 07:07 PDT, zeyu [:car]
philipp: review-
Details | Diff | Review
modify wheel event handlers (11.15 KB, patch)
2012-09-20 04:26 PDT, zeyu [:car]
philipp: review-
Details | Diff | Review
update handlers for wheel event when pixel scrolling is fired (16.53 KB, patch)
2012-09-25 07:27 PDT, zeyu [:car]
no flags Details | Diff | Review
add ceil in case of scrolling by 0 minute (16.51 KB, patch)
2012-09-28 08:09 PDT, zeyu [:car]
no flags Details | Diff | Review
Fixed pixel scrolling (13.13 KB, patch)
2012-09-29 13:59 PDT, Philipp Kewisch [:Fallen]
no flags Details | Diff | Review
fix for pixel scrolling (18.47 KB, patch)
2012-10-01 08:31 PDT, zeyu [:car]
philipp: review-
Details | Diff | Review
Final Fix (17.23 KB, patch)
2012-10-21 04:36 PDT, Philipp Kewisch [:Fallen]
philipp: review+
Details | Diff | Review

Description Philipp Kewisch [:Fallen] 2012-08-12 14:18:46 PDT
Quote from dev.platform:




Hi, let me tell you about the new D3E WheelEvent landing.
https://bugzilla.mozilla.org/show_bug.cgi?id=719320
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-wheelevents
https://developer.mozilla.org/en-US/docs/DOM/WheelEvent
https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference/wheel 


All developers should use "wheel" event instead of "DOMMouseScroll" and "MozMousePixelScroll" if it's possible.  If you need pixel delta values and line or page delta values in any deltaMode, please file a bug, it might be nice for internal code.



New "wheel" event replaces legacy mouse scroll events such as "DOMMouseScroll" and "MozMousePixelScroll" internally. I mean that widget/* will never dispatch the legacy events anymore and nsEventStateManager will never handle them too. But for web application's compatibility, the legacy events are fired almost same timing. The event order is:

1. content "wheel" event
2. content/chrome "DOMMouseScroll" (if deltaX/deltaY of "wheel" event is over 1 line)
3. content/chrome "MozMousePixelScroll"
4. chrome "wheel" event

If somebody calls preventDefault() in #1, #2 and #3 are skipped.
If somebody calls preventDefault() in #1, #2 or #3, chrome wheel event's defaultPrevented attribute becomes true.
# I'll document the detail in MDN later.



There is additional important change you should know.

By the changes, some prefs of mousewheel.* don't make sense now. They are replaced with new prefs:

* mousewheel.(horizscroll.)?(withshiftkey|withaltkey|withmetakey|withcontrolkey|withnokey).sysnumlines

  Just removed.
Comment 1 zeyu [:car] 2012-08-18 04:36:36 PDT
hi, i am interested to work on this bug, but this is my first bug, can you guide me to start pls? thanks.
Comment 2 Philipp Kewisch [:Fallen] 2012-08-18 15:24:27 PDT
This is a matter of looking through these DOMMouseScroll events:

http://mxr.mozilla.org/comm-central/search?string=DOMMouseScroll&find=calendar&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

And replacing them with a "wheel" event, possibly changing the event parameters used. See https://developer.mozilla.org/en-US/docs/Mozilla_event_reference/wheel for more details.
Comment 3 zeyu [:car] 2012-08-25 02:52:11 PDT
Hi,

I think for 
/calendar/base/content/dialogs/calendar-event-dialog-attendees.js (View Hg log or Hg annotations)
line 28 -- window.addEventListener("DOMMouseScroll", onMouseScroll, true);
line 859 -- * @param event The DOMMouseScroll event caused by scrolling.
(http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-attendees.js)

we just replace "DOMMouseScroll" with "WheelEvent" in line 28 will do.am i correct? And i am not familiar with xml, but the codes are like JS, so may i assume that they are basically js codes? 

Another question is how can i change the code and apply them locally to see if my change to the codes works?
Comment 4 zeyu [:car] 2012-09-06 07:07:14 PDT
Created attachment 658866 [details] [diff] [review]
replace DOMMouseScroll with wheel event
Comment 5 Philipp Kewisch [:Fallen] 2012-09-17 11:28:59 PDT
Comment on attachment 658866 [details] [diff] [review]
replace DOMMouseScroll with wheel event

Review of attachment 658866 [details] [diff] [review]:
-----------------------------------------------------------------

Changing the event name is not enough. You will need to react to the changes to the event properties. As you can read here:

https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference/DOMMouseScroll

previously event.detail was used to show how many *lines* the frame was scrolled by.

As you can read here: 

https://developer.mozilla.org/en-US/docs/Mozilla_event_reference/wheel

on the new event, event.detail is always 0. You need to use the deltaX/deltaY values instead and possibly only react to such events if enough delta has passed. See also the deltaMode parameter, possibly you can only react if the deltaMode is lines and then check deltaX/deltaY.

This needs to be done for each wheel event where event.detail is used. Please fix this and upload a new patch.
Comment 6 zeyu [:car] 2012-09-18 10:56:49 PDT
Hi Fallen,

Thanks for your feedback.
I have a question regarding the amount of delta passed.

> on the new event, event.detail is always 0. You need to use the
> deltaX/deltaY values instead and possibly only react to such events if
> enough delta has passed. See also the deltaMode parameter, possibly you can
> only react if the deltaMode is lines and then check deltaX/deltaY.

In DOMMouseScroll, if event.detail>0 it's scrolling down and if event.detail<0 it's scroll up.

In wheel event, can I use the similar approach? If deltaMode=line and deltaY>0 it's scrolling down and if deltaY<0 it's scrolling up.

Or there is a specific amount of delta to indicates it?
Comment 7 Philipp Kewisch [:Fallen] 2012-09-19 02:42:29 PDT
TBH I have never tested a wheel event. you can use cal.ERROR(event.deltaMode + " - " + event.deltaY); to find out what values are being passed.

Then you should just try and see what happens and see if the scrolling still feels natural.
Comment 8 zeyu [:car] 2012-09-20 04:26:02 PDT
Created attachment 662961 [details] [diff] [review]
modify wheel event handlers
Comment 9 Philipp Kewisch [:Fallen] 2012-09-21 07:45:07 PDT
Comment on attachment 662961 [details] [diff] [review]
modify wheel event handlers

Review of attachment 662961 [details] [diff] [review]:
-----------------------------------------------------------------

I've tested the patch and have noticed some issues:

* The day and week views have the possibility to rotate the view (View > Calendar > Current View > Rotate View). For me the views do not scroll horizontally using the mouse wheel, this used to work.

* When scrolling, no matter what direction you scroll in it only goes in one direction. This one is quite obvious.

* On mac, scrolling via Trackpad no longer works with the patch. Please check the documentation of the wheel event and find out what the expected process is for scrolling with the touchpad.

r- to fix above issues and code nits.

::: calendar/base/content/calendar-base-view.xml
@@ +763,5 @@
>                  break;
>          }
>        ]]></handler>
> +      <handler event="wheel"><![CDATA[
> +        if (event.shiftKey && getPrefSafe('calendar.view.mousescroll', true)&&event.deltaMode==event.DOM_DELTA_LINE) {

Please split this into multiple lines and align it to the (, like this:

if (event.shiftKey &&
    event.deltaMode == event.DOM_DELTA_LINE &&
    cal.getPrefSafe('calendar.view.mousescroll", true)) {

Also, while you are here change from getPrefSafe to cal.getPrefSafe (you couldn't have known this, I just thought it would be nice to change sine you are changing the line anyway).

::: calendar/base/content/calendar-month-view.xml
@@ +1006,5 @@
>              !event.shiftKey &&
>              !event.altKey &&
>              !event.metaKey &&
> +            scrollEnabled &&
> +	    event.deltaMode==event.DOM_DELTA_LINE) {

Please add a space before and after operators like == and &&, here and in the remaining patch.

::: calendar/base/content/widgets/minimonth.xml
@@ +1121,5 @@
>              rows = 1;
>            } else {
>              // In this case event.detail contains the default number of lines
>              // to scroll.  We always want to only scroll 1 month though
>              rows = (rows > 0) ? 1 : -1;

You can likely condense this whole block now with the new event. First check if the deltaMode is either DOM_DELTA_LINE or DOM_DELTA_PAGE and then use

rows = (event.deltaY > 0 ? 1 : -1);

::: calendar/resources/content/datetimepickers/datetimepickers.xml
@@ +535,5 @@
>              rows = 1;
>            } else { 
>              // In this case event.detail contains the default number of lines
>              // to scroll.  We always want to only scroll 1 hour though
>              rows = (rows > 0) ? 1 : -1;

Same here

@@ +565,5 @@
>              rows = 1;
>            } else { 
>              // In this case event.detail contains the default number of lines
>              // to scroll.  We always want to only scroll 1 minute though
>              rows = (rows > 0) ? 1 : -1;

Same here.
Comment 10 zeyu [:car] 2012-09-24 06:34:11 PDT
Hi Fallen,

I believe my lightning is correctly located, but when I add cal.ERROR(); in the wheel event handlers in calendar-base-view.xml and calendar-multiday-view.xml, it seemed did not take effect. As I scroll down&up nothing appears.
Comment 11 Philipp Kewisch [:Fallen] 2012-09-25 01:16:50 PDT
I've tested it locally and adding that error message works for me. It must be something with your build. If you have a moment this afternoon, maybe we can use Teamviewer to fix your build.

I also have some updated information on this bug: There is a difference between pixel scrolling and line scrolling, and depending on platform and type of mouse the one or other is used.

Back when the original code was written, I only had mouse wheel (line) scrolling in mind, since I didn't have a mac. This is why I set the value to 60 minutes per line.

What needs to be done nowadays is to react to both: if its pixel scrolling, then scroll by minutes (using event.deltaY / this.mPixPerMin), if its line scrolling then scroll by 60 minutes, regardless of how many lines are scrolled. Also watch out for rounding errors, you never want to scroll by 0 minutes.

I've also checked, you don't have to do anything special for the rotated week view, with some slight modifications (on your original patch there was still one event.detail) it works for me.
Comment 12 zeyu [:car] 2012-09-25 07:27:40 PDT
Created attachment 664498 [details] [diff] [review]
update handlers for wheel event when pixel scrolling is fired
Comment 13 zeyu [:car] 2012-09-28 08:09:46 PDT
Created attachment 665909 [details] [diff] [review]
add ceil in case of scrolling by 0 minute
Comment 14 Philipp Kewisch [:Fallen] 2012-09-29 13:59:56 PDT
Created attachment 666246 [details] [diff] [review]
Fixed pixel scrolling

I've tested your patch with pixel scrolling since you don't have the platform available. There were some small fixes needed and I've also cleaned up the style a bit. There are still some things to take care of though:

* I only updated the view code, you will have to copy it to the other places that still use your code to scroll, i.e minimonth
* Please test this with line scrolling again
* The scrolling in the attendees dialog doesn't work. I think you might have to stop propagation of more than one event, I think there is documentation on this on MDN.
* There are a lot of tabs where there should be spaces

I think we are getting closer to the goal, keep it up :-) Just as a hint, before you upload the diff take a look at it in the text editor. Its sometimes easier to catch typos like this.moveVIew(1) this way, the same with tabs.
Comment 15 zeyu [:car] 2012-10-01 08:31:41 PDT
Created attachment 666565 [details] [diff] [review]
fix for pixel scrolling
Comment 16 zeyu [:car] 2012-10-01 08:34:00 PDT
In the latest patch, I have to add:

>            if (event.deltaX != 0) {
>                deltaView = 0;
>            }

in minimonth, otherwise scrolling horizontally will make minimonth move towards only one direction. However, in other views I do not need to add these lines and scrolling horizontally won't take effect.
Comment 17 zeyu [:car] 2012-10-18 10:09:26 PDT
Hi Fallen,

Could you please review the latest patch I submitted? Thanks.
Comment 18 Philipp Kewisch [:Fallen] 2012-10-21 04:34:22 PDT
Comment on attachment 666565 [details] [diff] [review]
fix for pixel scrolling

Pixel scrolling still does not work for anything but the view. Just copy and paste doesn't do it. Also, the scrollYears function wasn't adapted.

I've taken the liberty to fix pixel scrolling and have created a patch for checkin. Coming up...
Comment 19 Philipp Kewisch [:Fallen] 2012-10-21 04:36:18 PDT
Created attachment 673682 [details] [diff] [review]
Final Fix

This patch takes care. For your future patches, please make sure tabs are converted to 4 spaces, indentation is correct and follow the spacing as you can see in other parts of the file (i.e  "if (foo != bar) {" instead of "if(foo!=bar){" )
Comment 20 Philipp Kewisch [:Fallen] 2012-10-21 04:38:55 PDT
Pushed to comm-central changeset c8c2a3ed3d9a

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