Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#16622 closed defect (fixed)

_FormWidgetMixin needs to use touch.release instead of "mouseup"

Reported by: Mark DeMichele Owned by: bill
Priority: undecided Milestone: 1.9
Component: Dijit - Form Version: 1.8.3
Keywords: Cc:
Blocked By: Blocking:

Description

I'm create a custom control that has a hidden focusNode as an input element so I can make the keyboard show on mobile devices. The input needs to get the focus on a "mouseup" which works on Android, but on IOS, it needs to be a "touchend". To fix this, I needed to change "onmouseup" in the _onFocus method to touch.release using the "dojo/touch" module.

I monkey patched it with the following code.

_FormWidgetMixin.extend( { _onFocus: function (/*String*/by) {

If user clicks on the widget, even if the mouse is released outside of it, this widget's focusNode should get focus (to mimic native browser hehavior). Browsers often need help to make sure the focus via mouse actually gets to the focusNode. if (by == "mouse" && this.isFocusable()) {

IE exhibits strange scrolling behavior when refocusing a node so only do it when !focused. var focusConnector = this.connect(this.focusNode, "onfocus", function () {

this.disconnect(mouseUpConnector); this.disconnect(focusConnector);

}); Set a global event to handle mouseup, so it fires properly even if the cursor leaves this.domNode before the mouse up event. var mouseUpConnector = this.connect(this.ownerDocumentBody, "onmouseup", function(){ var mouseUpConnector = this.connect(this.ownerDocumentBody, touch.release, function () {

this.disconnect(mouseUpConnector); this.disconnect(focusConnector); if here, then the mousedown did not focus the focusNode as the default action if (this.focused) {

this.focus();

}

});

} if (this.scrollOnFocus) {

this.defer(function () { winUtils.scrollIntoView(this.domNode); }); without defer, the input caret position can change on mouse click

} this.inherited(arguments);

} });

Attachments (1)

touchendTest.patch (2.8 KB) - added by bill 6 years ago.
test for android and iOS, if you touch the button, slide your finger off it, and then release, the mouseup handler never gets called

Download all attachments as: .zip

Change History (13)

comment:1 Changed 6 years ago by Mark DeMichele

Turns out my fix broke on my android tablet. Not sure why. So I needed to specify the event this way

var mouseUpConnector = this.connect(this.ownerDocumentBody, has("ios") ? touch.release : "onmouseup", function () {...}

This works in both.

comment:2 Changed 6 years ago by bill

This is a scary ticket because if this problem does occur, it will leave a dangling mouseup listener on <body>... and then theoretically after focusing a number of form widgets you could get a buildup of those listeners. Not sure what ill effects that would cause but it doesn't seem good.

I'd like to see a test case, but I think it would be safe regardless to change the existing code from:

var mouseUpConnector = this.connect(this.ownerDocumentBody, "onmouseup", function(){
	this.disconnect(mouseUpConnector);
	this.disconnect(focusConnector);
	// if here, then the mousedown did not focus the focusNode as the default action
	if(this.focused){
		this.focus();
	}
});

to be:

var mouseUpConnector = this.own(on(this.ownerDocumentBody, "mouseup, touchend", function(){
	mouseUpConnect.remove()
	focusConnector.remove();
	// if here, then the mousedown did not focus the focusNode as the default action
	if(this.focused){
		this.focus();
	}
}));

Basically what you suggested, but there's no need to pull in dojo/touch.

On a larger note, I wonder about getting rid of that mouseup listening code altogether. I really don't care what happens if the user mousedown's on a button but then moves the cursor away before mouseup. And, the code in dijit/focus.js to listen for touch events doesn't work well anyway because on IE it's using attachEvent() on <body>, which means that any evt.stopPropagation() call on a subnode will break it.

Changed 6 years ago by bill

Attachment: touchendTest.patch added

test for android and iOS, if you touch the button, slide your finger off it, and then release, the mouseup handler never gets called

comment:3 Changed 6 years ago by bill

OK, turns out the test case is pretty simple. I attached a test case above where the mouseup code never gets called on android or iOS if you touch the button, slide your finger off it, and then release.

Catching the touchend on old android devices though is not so simple, due to http://code.google.com/p/android/issues/detail?id=19827. I'll check in the code I suggested above to listen to touchend in addition to mouseout, but that's only a partial solution because it doesn't make things better on old android. (Theoretically it will fix things on new android.)

So, I still wonder about removing that code completely. Doug?

comment:4 Changed 6 years ago by bill

PS: Hmm seems if that code is not there then (at least) the first dialog button in test_Dialog.html doesn't get focused when clicked (by the mouse, in IE8). Not sure why.

comment:5 Changed 6 years ago by bill

In [30461]:

Monitor touchend in addition to mouseup, for mobile. Also, use this.own() with dojo/on instead of deprecated this.connect() . Refs #16622, #16585 !strict.

comment:20 Changed 6 years ago by bill

In [30463]:

For _TextBoxMixin too, monitor touchend in addition to mouseup. Also, use this.own() with dojo/on instead of deprecated this.connect() . Refs #16622, #16585 !strict.

comment:21 Changed 6 years ago by Douglas Hays

Milestone: tbd1.9
Owner: changed from Douglas Hays to bill
Status: newassigned

Without the mouseup focus code, it's possible to click a button and it never receives focus. Dialog_mouse::cancel Dialog shows this.

comment:22 Changed 6 years ago by bill

Resolution: fixed
Status: assignedclosed

OK, let's leave that code there for now, and mark this as fixed, since it's now monitoring for both mouseup and touchrelease.

comment:23 Changed 6 years ago by Mark DeMichele

Bill,

Thanks for this fix. I patched your fix in my local copy and there are still issues with the IPAD (and probably Android). However, the problems only occur when the widget is on a page that's in an IFrame (which happens to be my primary use case). It's really odd. You can test this by making a simple test page with a dijit TextBox?. Then make a simple html page (no dojo) with and IFrame that points to the test page.

If you bring that up on an ipad, the textbox doesn't work.

While trying to narrow down a cause for this, I ran some other tests with my ipad and found some interesting things that may mean that this is more involved than just an issue with FormWidgetMixin?. Once again, these issues only happen when your app is in an IFrame.

Doing this same iframe test. I added a basic non-dojo input tag to my page. The tag happens to be inside and ContentPane? which is inside a BorderContainer?, but I'm not sure if that matters. On the IPAD, this input behaves oddly after you type in it and then try to click again in another spot to edit your text you just typed in without losing your focus on the input. Sometimes it doesn't work, other times the keyboard will go away and come back and then you can type. I'm just bringing it up since it could be a clue. It's all very strange.

If you would like to discuss all this in detail maybe we can get together on IRC or something. Just let me know.

comment:24 Changed 6 years ago by bill

I'm not sure if there's much to discuss. You could file another ticket with the iframe test case (still need a test case although you say it's trivial), but it's low priority; not sure if it's even fixable.

comment:25 Changed 6 years ago by Mark DeMichele

Ok, I'm working on a fix. When I get it all figured out, I'll create another ticket. How do you suggest I supply a test case? I'll basically need to supply two html pages. Do I hook them up with the CDN or maybe do the url that hosts the Dojo source? Or do I put them dijit/tests/form folder in the trunk and include them in a patch file with my fixes?

comment:26 Changed 6 years ago by bill

Oh including them in the patch file would be great, or you could just attach both files to the ticket.

Note: See TracTickets for help on using tickets.