Closed Bug 505391 Opened 11 years ago Closed 10 years ago

Implement support for touch sensitive click wheel on HTC phones

Categories

(Core :: Widget: Win32, defect)

Other
Other
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: alexandru.cristei, Assigned: alexandru.cristei)

References

()

Details

Attachments

(2 files, 15 obsolete files)

3.00 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
4.73 KB, patch
dougt
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US) AppleWebKit/530.5 (KHTML, like Gecko) Chrome/2.0.172.37 Safari/530.5
Build Identifier: 

Component for extending HTC Diamond capability, making Fennec use the clickwheel for zooming.

Reproducible: Always

Actual Results:  
I finished the XPCOM component and integrated it in javascript code (InputHandler.js) and now it should be all about fixing bugs.
Summary: Work In Progress HtcZoom → Implement support for touch sensitive click wheel on HTC phones
Attached patch 24.07.2009 (obsolete) — Splinter Review
It generates me an error from js code : TypeError: Components.classes['@alexandrucristei.com/phone/HTCZoom;1'] is undefined
Attachment #390450 - Attachment is obsolete: true
Attachment #390651 - Attachment is obsolete: true
My patch does nothing practically, but the messages are being delivered to the top window.
Attachment #390958 - Attachment is obsolete: true
Patches are already being attached to this bug, so moving this to NEW.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Try www.google.com and then search something. After that, use the navigation wheel to scroll up and down. You may nottice it doesn't scroll all the page.
Attachment #391090 - Attachment is obsolete: true
It zooms kind of akward.
Attachment #392743 - Attachment is obsolete: true
Attachment #395931 - Attachment is obsolete: true
Attachment #395932 - Attachment is obsolete: true
Comment on attachment 396025 [details] [diff] [review]
HTC Zoom - the diff from mozilla-central folder. -working-

>-  mPageZoom = aFullZoom;
>+  mPageZoom = aFullZoom; 

This looks to be a white space issue, fix it
>+ 		//distance /= 20;
>+ 		//event.button    = 0;
>+ 		//DispatchEvent(&event, status);
>+ 		//DispatchPendingEvents();
>+ 		//printf("\n Event status :%d\n\n", (int)status);
>+
>+		/*
>+		nsEventStatus status2;
>+ 		nsMouseScrollEvent event2(PR_TRUE, NS_MOUSE_SCROLL, this);
>+ 		event2.delta = distance;
>+ 		event2.scrollFlags = nsMouseScrollEvent::kNoLines;
>+ 		event2.isShift   = IS_VK_DOWN(NS_VK_SHIFT);
>+ 		event2.isControl = IS_VK_DOWN(NS_VK_CONTROL);
>+ 		event2.isMeta    = PR_FALSE;
>+ 		event2.isAlt     = IS_VK_DOWN(NS_VK_ALT);
>+ 		//event.button    = 0;
>+ 		event2.time = PR_Now() / 1000;
>+ 		InitEvent(event2);
>+ 		DispatchEvent(&event2, status2);
>+ 		//DispatchPendingEvents();
>+ 		printf("\n Event status :%d\n\n", (int)status);
>+
>+		nsEventStatus status3;
>+ 		nsMouseScrollEvent event3(PR_TRUE, NS_MOUSE_SCROLL, this);
>+ 		event3.delta = distance;
>+		event3.scrollFlags = nsMouseScrollEvent::kNoLines || nsMouseScrollEvent::kHasPixels;
>+ 		event3.isShift   = IS_VK_DOWN(NS_VK_SHIFT);
>+ 		event3.isControl = IS_VK_DOWN(NS_VK_CONTROL);
>+ 		event3.isMeta    = PR_FALSE;
>+ 		event3.isAlt     = IS_VK_DOWN(NS_VK_ALT);
>+ 		//event.button    = 0;
>+ 		event3.time = PR_Now() / 1000;
>+ 		InitEvent(event3);
>+ 		DispatchEvent(&event3, status3);
>+ 		//DispatchPendingEvents();
>+ 		printf("\n Event status :%d\n\n", (int)status);
>+		*/
>+ 
>+ 		/*if ( (int)status == 1 )
>+ 		{
>+ 			HWND parentWnd = ::GetParent(GetWindowHandle());
>+ 			if ( parentWnd != NULL )
>+ 				::SendMessageW(parentWnd, msg, wParam, lParam);
>+ 		}*/
>+ 			//if ( ::GetParent(GetWindowHandle()) != NULL )
>+ 				//::SendMessageW(::GetParent(GetWindowHandle()), msg, wParam, lParam);
>+			//else
>+				//DispatchEvent(&event, status);

delete the commented out code, post a new patch and request review from dougt
Comment on attachment 396026 [details] [diff] [review]
HTC Zoom - the diff from mobile folder. -working-


>+
>+pref("javascript.options.showInConsole", true);
>+pref("nglayout.debug.disable_xul_cache", true);
>+pref("browser.dom.window.dump.enabled", true);
>+pref("javascript.options.strict", true);
>+pref("extensions.logging.enabled", true);

these should be in your profile's prefs.js, remove them

>   //let useEarlyMouseMoves = gPrefService.getBoolPref("browser.ui.panning.fixup.mousemove");

delete commented out code

>+		dump("We got a Scroll event!!!Scroll Module:");
>+		dump(evInfo.event.type);

delete all dumps
>+		//Browser.zoom(evInfo.event.detail);
more commented out code
>+		dump("We got a Scroll event!!!HTC Module:");
>+		dump(evInfo.event.type);
more dumps
>+	//window.addEventListener("DOMMouseScroll", function (e) {dump("yeeeeeeeeees! DMS");}, true);
>+	//window.addEventListener("MozMousePixelScroll", function (e) {dump("yeeeeeeeeees! MMPS");}, true);
more commented out code


clean this up, post a new patch and ask for review from mfinkle or bcombee
Comment on attachment 396026 [details] [diff] [review]
HTC Zoom - the diff from mobile folder. -working-

>diff --git a/app/mobile.js b/app/mobile.js

>+pref("javascript.options.showInConsole", true);
>+pref("nglayout.debug.disable_xul_cache", true);
>+pref("browser.dom.window.dump.enabled", true);
>+pref("javascript.options.strict", true);
>+pref("extensions.logging.enabled", true);

Remove these prefs before making your next patch

>diff --git a/chrome/content/InputHandler.js b/chrome/content/InputHandler.js

>+  this.listenFor(browserViewContainer, "MozMousePixelScroll");
>+  
>+  /* HTC Nav Wheel module; it gets DOMMouseScroll and MozMousePixelScroll */
>+  this.listenFor(window, "DOMMouseScroll");
>+  this.listenFor(window, "MozMousePixelScroll");

Do we need "MozMousePixelScroll" in both handlers? Do we need "DOMMouseScroll" in both?

We should probably wrap this Win CE only code in #ifdef WINCE

>   this.addModule(new MouseModule(this));
>   this.addModule(new ScrollwheelModule(this, browserViewContainer));
>+  this.addModule(new HTCScrollModule(this));

We should probably wrap this Win CE only code in #ifdef WINCE

> ScrollwheelModule.prototype = {
>+	pendingEvent : 0,

Use 2 spaces for indents, no tabs

>   handleEvent: function handleEvent(evInfo) {
>-    if (evInfo.event.type == "DOMMouseScroll") {
>-      Browser.zoom(evInfo.event.detail);
>+    if (evInfo.event.type == "DOMMouseScroll" || evInfo.event.type == "MozMousePixelScroll") {
>+		dump("We got a Scroll event!!!Scroll Module:");
>+		dump(evInfo.event.type);
>+		if( this.pendingEvent )
>+			window.clearTimeout(this.pendingEvent);
>+		this.pendingEvent = window.setTimeout(this.handleEventImpl, 10, evInfo.event.detail);

Use 2 spaces for indents, no tabs

>       evInfo.event.stopPropagation();
>       evInfo.event.preventDefault();
>     }
>+  },
>+  handleEventImpl: function handleEventImpl(zoomlevel){
>+		this.pendingEvent = 0;
>+		Browser.zoom(zoomlevel);
>+		//Browser.zoom(evInfo.event.detail);

Remove the dumps before your next patch
Why do we need a timeout?
Use 2 spaces for indents, no tabs

>+HTCScrollModule.prototype = {
>+	pendingEvent : 0,
>+  handleEvent: function handleEvent(evInfo) {
>+    if (evInfo.event.type == "DOMMouseScroll" || evInfo.event.type == "MozMousePixelScroll") {
>+		dump("We got a Scroll event!!!HTC Module:");
>+		dump(evInfo.event.type);
>+		if( this.pendingEvent )
>+			window.clearTimeout(this.pendingEvent);
>+		this.pendingEvent = window.setTimeout(this.handleEventImpl, 10, evInfo.event.detail);
>+      evInfo.event.stopPropagation();
>+      evInfo.event.preventDefault();
>+    }
>+  },
>+  handleEventImpl: function handleEventImpl(zoomlevel){
>+		this.pendingEvent = 0;
>+		Browser.zoom(zoomlevel);
>+  },
>+
>+  /* We don't have much state to reset if we lose event focus */
>+  cancelPending: function cancelPending() {}
>+};

Same issues here

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>+	//window.addEventListener("DOMMouseScroll", function (e) {dump("yeeeeeeeeees! DMS");}, true);
>+	//window.addEventListener("MozMousePixelScroll", function (e) {dump("yeeeeeeeeees! MMPS");}, true);

Remove these
Attachment #396026 - Flags: review-
Attachment #396025 - Attachment is obsolete: true
Attachment #396026 - Attachment is obsolete: true
Attached patch mobile folder patch working II (obsolete) — Splinter Review
Attached patch mobile folder patch working III (obsolete) — Splinter Review
Attachment #396087 - Attachment is obsolete: true
Attachment #396101 - Attachment is obsolete: true
Attachment #396103 - Flags: review?(mark.finkle)
Attachment #396086 - Flags: review?(doug.turner)
Comment on attachment 396103 [details] [diff] [review]
mobile folder patch working IV - tab problem solved

>diff --git a/chrome/content/InputHandler.js b/chrome/content/InputHandler.js

>   this.listenFor(browserViewContainer, "keydown");
>   this.listenFor(browserViewContainer, "keyup");
>   this.listenFor(browserViewContainer, "DOMMouseScroll");
>-
>-  //let useEarlyMouseMoves = gPrefService.getBoolPref("browser.ui.panning.fixup.mousemove");
>+  this.listenFor(browserViewContainer, "MozMousePixelScroll");
>+  
>+#ifdef WINCE
>+  /* This ensures we get the DOMMouseScroll and MozMousePixelScroll events from the HTC Nav Wheel*/
>+  this.listenFor(window, "DOMMouseScroll");
>+  this.listenFor(window, "MozMousePixelScroll");
>+#endif


The only thing that I'm wondering about now is the fact we add listeners for "DOMMouseScroll" and "MozMousePixelScroll" to the browserViewContainer _and_ to the window (if WINCE). I am wondering if we can remove the browserViewContainer listeners and always use the window listeners (not just for WINCE).

Can you change the browserViewContainer listeners to window listeners and completely remove the #ifdef WINCE block? Will everything still work?

I would like to only have one listener for any event.
Attachment #396103 - Flags: review?(mark.finkle) → review-
I was very surprised to find out the events were coming from browserViewContainer after all.
Attachment #396103 - Attachment is obsolete: true
Attachment #396145 - Flags: review?(mark.finkle)
Attachment #396145 - Flags: review?(mark.finkle) → review+
Comment on attachment 396145 [details] [diff] [review]
mobile folder patch working V

>diff --git a/chrome/content/InputHandler.js b/chrome/content/InputHandler.js

> ScrollwheelModule.prototype = {

>+      if( this.pendingEvent )
>+        window.clearTimeout(this.pendingEvent);

Add space after "if" and remove spaces in the "(" ")"

>     }
>+  },
>+  handleEventImpl: function handleEventImpl(zoomlevel){

Add a blank line between methods and a space before the "{"

r+, nice job

Whoever checks in this patch can make these small nit changes.
Attachment #396086 - Flags: review?(doug.turner) → review-
Comment on attachment 396086 [details] [diff] [review]
mozilla-central folder patch working II

good start, but too many issues to comment on.  I will attach a cleaned up patch and you can compare for your own edification.
Attached patch patch v.3 (obsolete) — Splinter Review
this isn't ready for review.

* the HTCsndr thing is a hack, and I am not really sure for what.  Why is it needed?  I do not understand the comment above this code

* why do you need to set the time on the event?

Give this patch a go, fix the above, and resubmit.  I would really like to see this working! ;-)
Attachment #396086 - Attachment is obsolete: true
Comment on attachment 396145 [details] [diff] [review]
mobile folder patch working V


>+      if( this.pendingEvent )
>+        window.clearTimeout(this.pendingEvent);
>+      this.pendingEvent = window.setTimeout(this.handleEventImpl, 1, evInfo.event.detail);

When landing this, let's try to remember to drop the "window." part form clearTimeout and setTimeout. Also, instead of a 1ms timeout, let's just use 0, which is more conventional.
Attached patch patch v.4 (obsolete) — Splinter Review
This patch moves the initialization code from the window creation to the window activation so that the most recently activated window is receiving the events as opposed to the most recently created window.

This patch also changes the WM_HTCNAV message handing code to just call OnMouseWheel so that the events are handled with the same code as regular mouse wheel events.  To enable this, the mouse wheel handling code was un-ifdef'd.

The one known hacky thing about this patch is I'm using the center of the screen for the mouse wheel event's point.  Alternatively we could keep track of the last mouse up and use that, but this seems to work just as well without all that overhead.
Attachment #396231 - Attachment is obsolete: true
Attachment #396677 - Flags: review?(doug.turner)
Comment on attachment 396677 [details] [diff] [review]
patch v.4

Drop:

+
+unsigned int HTCsndr = 1;


Wrong curly brace style (this was your change.  no more talking trash about my editor!)

+      if(gHTCApiNavOpen == nsnull)
+        {

In both calls to the HTC Api, we are passing magic numbers.  Can we get some sort of documentation or URL in a comment? 

+      if (gHTCApiNavOpen != NULL)
+        gHTCApiNavOpen(mWnd, 1 /* 1 means xxx */);
+      
+      if ( gHTCApiNavSetMode != NULL)
+        gHTCApiNavSetMode ( mWnd, 4 /* Gesture Mode. wtf does that mean*/);


Using the center of the screen probably works.  Do you know if you can scroll iframes that are positioned such that they do not intersect with the center point of the screen?
Attachment #396677 - Flags: review?(doug.turner) → review-
Comment on attachment 396677 [details] [diff] [review]
patch v.4

r+ if you fix those things up and give me a reasonable answer for the iframe question.
Attached patch patch v.5Splinter Review
(In reply to comment #25)
> (From update of attachment 396677 [details] [diff] [review])
> Drop:
> 
> +
> +unsigned int HTCsndr = 1;
 
done

> Wrong curly brace style (this was your change.  no more talking trash about my
> editor!)
> 
> +      if(gHTCApiNavOpen == nsnull)
> +        {
done, but I'm skeptical you didn't cause this somehow
 
> In both calls to the HTC Api, we are passing magic numbers.  Can we get some
> sort of documentation or URL in a comment? 
> 
> +      if (gHTCApiNavOpen != NULL)
> +        gHTCApiNavOpen(mWnd, 1 /* 1 means xxx */);
> +      
> +      if ( gHTCApiNavSetMode != NULL)
> +        gHTCApiNavSetMode ( mWnd, 4 /* Gesture Mode. wtf does that mean*/);
 
I've got nothing in terms of documentation, but there  is discussion of these methods and their various uses on xda-developers.com's forums
 
> Using the center of the screen probably works.  Do you know if you can scroll
> iframes that are positioned such that they do not intersect with the center
> point of the screen?

No, but can we take that as a follow up bug?
Attachment #396677 - Attachment is obsolete: true
Attachment #396683 - Flags: review?(doug.turner)
Comment on attachment 396683 [details] [diff] [review]
patch v.5

/* 1 means xxx */

to

/* undocumented value */


/* Gesture Mode. wtf does that mean*/

to

/* 4 is Gesture Mode.  This will generate the WM_HTCNAV event to our window */


Also, i would add above:

+const int WM_HTCNAV = 0x0400 + 200;


This is the defined value for Gesture Mode.


As for follow up bugs, sure.  But it would be nice to know in advance if this will work for such cases.
Attachment #396683 - Flags: review?(doug.turner) → review+
Component: Windows Mobile → Widget: Win32
Product: Fennec → Core
QA Contact: mobile-windows → win32
Attachment #396683 - Flags: approval1.9.2?
Flags: in-litmus?
Attachment #396683 - Flags: approval1.9.2? → approval1.9.2+
Flags: in-litmus? → in-litmus-
Assignee: nobody → alexandru.cristei
You need to log in before you can comment on or make changes to this bug.