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(); });
Change History (26)
comment:1 Changed 5 years ago by
Milestone: | tbd → 1.10.3 |
---|---|
Owner: | set to Patrick Ruzand |
Priority: | undecided → blocker |
Status: | new → assigned |
comment:2 Changed 5 years ago by
Owner: | changed from Patrick Ruzand to Adrian Vasiliu |
---|
comment:3 Changed 5 years ago by
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.
comment:4 Changed 5 years ago by
The issue is coming from this preventDefault() call: https://github.com/dojo/dojo/commit/33be784ac33a090cc9091ef449ef930064802f7d#diff-4d94e8ca70a3b3b428f55af8bcf13a34R122
comment:5 Changed 5 years ago by
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
Cc: | bill Eric Durocher added |
---|
comment:7 Changed 5 years ago by
Component: | DojoX Mobile → Events |
---|---|
Summary: | html checkbox problem on mobile → dojo/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 follow-up: 11 Changed 5 years ago by
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
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
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 Changed 5 years ago by
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. callpreventDefault()
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
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
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
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
Milestone: | 1.10.3 → 1.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
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:19 Changed 4 years ago by
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
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
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
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
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
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
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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.
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.