Closed Bug 782152 Opened 12 years ago Closed 12 years ago

Replace DOMMouseScroll with wheel event

Categories

(Calendar :: Calendar Frontend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Fallen, Assigned: car)

Details

(Whiteboard: [good first bug][mentor=Fallen][lang=js])

Attachments

(1 file, 6 obsolete files)

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.
hi, i am interested to work on this bug, but this is my first bug, can you guide me to start pls? thanks.
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.
Assignee: nobody → zeyufly
Status: NEW → ASSIGNED
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?
Attached patch replace DOMMouseScroll with wheel event (obsolete) β€” β€” Splinter Review
Attachment #658866 - Flags: review?(philipp)
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.
Attachment #658866 - Flags: review?(philipp) → review-
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?
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.
Attached patch modify wheel event handlers (obsolete) β€” β€” Splinter Review
Attachment #662961 - Flags: review?(philipp)
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.
Attachment #662961 - Flags: review?(philipp) → review-
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.
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.
Attachment #664498 - Flags: review?(philipp)
Attached patch add ceil in case of scrolling by 0 minute (obsolete) β€” β€” Splinter Review
Attachment #664498 - Attachment is obsolete: true
Attachment #664498 - Flags: review?(philipp)
Attachment #665909 - Flags: review?(philipp)
Attached patch Fixed pixel scrolling (obsolete) β€” β€” Splinter Review
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.
Attachment #665909 - Attachment is obsolete: true
Attachment #665909 - Flags: review?(philipp)
Attachment #662961 - Attachment is obsolete: true
Attachment #658866 - Attachment is obsolete: true
Attached patch fix for pixel scrolling (obsolete) β€” β€” Splinter Review
Attachment #666565 - Flags: review?(philipp)
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.
Hi Fallen,

Could you please review the latest patch I submitted? Thanks.
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...
Attachment #666565 - Flags: review?(philipp) → review-
Attached patch Final Fix β€” β€” Splinter Review
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){" )
Attachment #666246 - Attachment is obsolete: true
Attachment #666565 - Attachment is obsolete: true
Attachment #673682 - Flags: review+
Pushed to comm-central changeset c8c2a3ed3d9a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: