Opened 5 years ago

Closed 4 years ago

#18352 closed defect (fixed)

dojo/touch with dojoClick=true breaks interaction with "native" HTML input of type "checkbox" or "radio" on iOS 8 and Chrome / Android 4.4

Reported by: ysazak Owned by: Adrian Vasiliu
Priority: blocker Milestone: 1.11
Component: Events Version: 1.10.2
Keywords: Cc: bill, Eric Durocher
Blocked By: Blocking:

Description

I use a regular html checkbox elemnt in my mobile application.

after call below code block, checkbox doesnt respond on mobile devices(Ipad Ios8, Android 4.4). Please check jsfiddle test case aswell.

        require(["dojox/mobile/parser", "dojox/mobile"],
			function (parser) {
			    parser.parse();
			});

http://jsfiddle.net/ysazak/wpL0L5p4/6/

Change History (26)

comment:1 Changed 5 years ago by dylan

Milestone: tbd1.10.3
Owner: set to Patrick Ruzand
Priority: undecidedblocker
Status: newassigned

I fixed your fiddle to load async: http://jsfiddle.net/dylan/wpL0L5p4/7/

I notice that this example works on iOS7, but breaks on iOS8.

If this is a bug and not a misuse of dojox/mobile, it pretty much breaks things badly.

comment:2 Changed 5 years ago by Patrick Ruzand

Owner: changed from Patrick Ruzand to Adrian Vasiliu

comment:3 Changed 5 years ago by Adrian Vasiliu

Here is a more minimal version of the fiddle: http://jsfiddle.net/adrian_vasiliu/fwhg8dae/ Requiring any dojox/mobile module isn't necessary for reproducing the issue. In this variant of the fiddle, the only required module is "dojo/touch", plus setting document.dojoTouch at true (as dojox/mobile/common does - this feature of dojo/touch has been introduced for dojox/mobile).

There will be further investigation. For now, two solutions/workarounds: 1/ Use dojox/mobile/ComboBox instead of <input type="checkbox">. Besides avoiding this issue, it also provides a nicer look and feel. I've put it in http://jsfiddle.net/adrian_vasiliu/wxuyyrbb/ (notice that it also imports the CSS of dojox/mobile/ComboBox by importing dojox/mobile/deviceTheme.js as external resource).

2/ If you don't want to use dojox/mobile/ComboBox, just set document.dojoTouch at false after dojox/mobile/common loads. I've put it in http://jsfiddle.net/adrian_vasiliu/eqpr53ap/. The drawback is that there is a 300 ms delay between touching the checkbox and its state change. The use of dojox/mobile/ComboBox seems preferable to me.

Version 1, edited 5 years ago by Adrian Vasiliu (previous) (next) (diff)

comment:4 Changed 5 years ago by Adrian Vasiliu

Last edited 5 years ago by Adrian Vasiliu (previous) (diff)

comment:5 Changed 5 years ago by Adrian Vasiliu

I've put a candidate solution in https://github.com/AdrianVasiliu/dojo/commit/34bbe042d1fb4a31659a78f0c4aaecd4a91ae620 (change in dojo/touch). A manual test case: https://github.com/AdrianVasiliu/dojox/commit/5fe1b982317673703369f2270ff741108e72a81c .

@bill / @edurocher, what would you think about it? Of course, it's not an ideal solution, but given the current dojo/touch I'm not sure we can do better.

If there's no disagreement on it, I'll further test and transform it in a pull request.

comment:6 Changed 5 years ago by Adrian Vasiliu

Cc: bill Eric Durocher added

comment:7 Changed 5 years ago by Adrian Vasiliu

Component: DojoX MobileEvents
Summary: html checkbox problem on mobiledojo/touch with dojoClick=true breaks interaction with "native" HTML input of type "checkbox" or "radio" on iOS 8 and Chrome / Android 4.4

comment:8 Changed 5 years ago by bill

I looked at this a long time. I guess it's OK. It's ugly, but solves the problem, and the code was already ugly before your change.

One alternate approach is for dojo/touch to never call preventDefault() for any <input> nodes, but to make dijit/form/Checkbox etc. call preventDefault() manually. Not sure if that is better though. The widget code would need to setup a listener in the capturing phase (addEventListener(..., true)) to get to the event even though dojo/touch calls stopImmediatePropagation().

comment:9 Changed 5 years ago by Eric Durocher

The fix looks OK to me as it should be harmless for widgets. Maybe the condition could be made clearer, as it is becoming more and more difficult to read...

Also, if the behavior really changed with iOS8 and Android 4.4, is it still OK with older versions and the fix (which does not test OS versions)?

comment:10 Changed 5 years ago by Adrian Vasiliu

I've also tested with iOS 7 and it looked... even better with the fix than without. I mean, strangely, without the fix on iOS 7.1.2 (and with dojo/touch and dojoClick=true) the native checkbox "sometimes" works, while with the fix it always works... Hence it doesn't seem so clear that the fix is only useful for iOS >= 8. Anyway, so from this test the fix seems rather benefical for both iOS 7 and 8.

I'll also test with iOS 6 (although low priority) and with older Android with stock browser and Chrome.

comment:11 in reply to:  8 Changed 5 years ago by ben hockey

Replying to bill:

One alternate approach is for dojo/touch to never call preventDefault() for any <input> nodes, but to make dijit/form/Checkbox etc. call preventDefault() manually.

touch events are complicated and i'm not an expert with them but this option sounds better to me since code in the dojo namespace doesn't need to add special knowledge of something related to the dijit namespace - it puts the responsibility of the preventDefault() call with the code that depends on the call. i think it is worth pursuing this option and only considering adrian's suggestion if this option isn't possible.

comment:12 Changed 5 years ago by Adrian Vasiliu

doesn't need to add special knowledge of something related to the dijit namespace

Just to mention that dojo/touch already had a similar solution (checking a dijit CSS class): https://github.com/dojo/dojo/blob/1.10/touch.js#L182

Of course, we may prefer not adding another similar check. But this proposed change is similar to the existing one, and reengineering dojo/touch entirely (plus readapting dijit and dojox/mobile to possible changes, and testing everything on all supported mobile browsers) to really clean it up is probably not the best thing we can afford doing now.

comment:13 Changed 5 years ago by dylan

Given that Dijit depends on Dojo, but Dojo does not depend on Dijit, that's really not a good thing.

Just because it's there doesn't mean we should do it again. That should really be fixed/changed, not replicated. :)

comment:14 Changed 5 years ago by bill

The advantage of Adrian's approach is that it's the least invasive, so the least likely to cause problems (side effects). This is an important consideration, especially for updates to patch releases (i.e. 1.10.3 and possible further backports).

I did think of another approach though, which is probably better than my original suggestion. When dojo/touch emits the dojoclick event, it could also programatically change the state of the <input type=checkbox>. Not sure if you can call node.click(), or you would have to do something manual like node.focus(); node.checked=true; on.emit(node, "change", ...) (since IIRC change events are only emitted for user instantiated changes. In any case, the point is that you could keep calling preventDefault() on checkboxes, and you would get rid of the 300ms delay "problem" for native checkboxes too.

comment:15 Changed 4 years ago by dylan

Milestone: 1.10.31.11

I guess we should just land the proposed fix for 1.11 then.

Adrian, do you have time to create a pull request?

comment:16 Changed 4 years ago by Adrian Vasiliu

Yes, I can create the PR, if bill is still okay with it. FYI I checked whether the issue is still there with iOS 9: yes, still there, and the fix still fixes it. Tested on iOS 9.0.2 on iPad Air 2. I'll also check Android 5 and 6.

comment:17 Changed 4 years ago by bill

Sure, sounds fine.

comment:18 Changed 4 years ago by Adrian Vasiliu

Okay, hope to find a bit of time for it soon.

comment:19 Changed 4 years ago by beglee

We've been affected by this issue in our product, which currently uses Dojo v1.9.7. Will the fix be backported to older versions?

comment:20 Changed 4 years ago by Adrian Vasiliu

Yes, at least in my eyes the fix would be backported to 1.10 and 1.9.

By the way, I did spend some time working on preparing the PR, mostly fighting to find a way to get the functional tests running such that I can add one for this issue. So far wasn't successful (on my Windows machine), either locally or remotely on saucelabs, but I keep working on it.

comment:21 Changed 4 years ago by Adrian Vasiliu

And I said I'll check Android. This is done. The issue hurts on Android 4.4.4, 5.1 et 6.0 (all tests on Nexus 5 physical device) just as on iOS 8 and 9, and the proposed fix does fix it for all these Android versions too.

comment:22 Changed 4 years ago by Adrian Vasiliu

Just an update, running the functional tests works for me on a Mac (while I wasn't able to get them running on my PC).

comment:23 Changed 4 years ago by beglee

I've tried out the proposed fix in 1.9.7 in our test environment, and it looks good for the most part. However, when I click on the LABEL associated with a checkbox/radio, the preventDefault still kicks in. Would there be a downside to adding [e.target.tagName != "LABEL"] to the if statement?

comment:24 Changed 4 years ago by Adrian Vasiliu

I don't reproduce with Safari / iOS 9.0.2 / iPad Air 2. With an HTML such as

<input type="checkbox" id="test"><label for="test">Test</label>

I do get the state of the checkbox changing when touching the "Test" label, as long as I use the proposed fix.

If you use id=test instead id="test", please add the missing double-quotes. Also, on my iPad with this iOS version, I need to clear Safari's cache and even to reload the page a number of times before seeing the effect of changes in the JS code... This might be a source of confusion.

If you still reproduce despite the change mentioned above, please tell the exact device and OS version.

comment:26 Changed 4 years ago by Adrian Vasiliu

Resolution: fixed
Status: assignedclosed

Fix merged into master via https://github.com/dojo/dojo/commit/961860c16a70204097718f0d65d0fc1afafcfb40. Backport to 1.9 and 1.10 will be done once we get enough confidence from users' feedback for master/1.11.

Note: See TracTickets for help on using tickets.