Opened 7 years ago

Closed 6 years ago

#15786 closed defect (fixed)

Opening an dojox/mobile/Opener from a dojox/mobile/_ItemBase onClick does not work on iPad

Reported by: cjolif Owned by: Eric Durocher
Priority: high Milestone: 1.9
Component: DojoX Mobile Version: 1.8.0rc1
Keywords: todoapp Cc: avasiliu
Blocked By: Blocking:

Description (last modified by cjolif)

If you run the attached test case and touch "Click Me!" on an iPad, this will briefly open the dojox/mobile/Opener which will then get immediately closed.

This is working correctly on iPhone or Chrome.

Attachments (3)

test_Opener-DateSpinWheel.html (2.0 KB) - added by cjolif 7 years ago.
patch15786.patch (590 bytes) - added by Adrian Vasiliu 7 years ago.
Avoid that opener closes immediately after opening on tablets (in tooltip mode) - Adrian Vasiliu, IBM, CCLA
15786.patch (470 bytes) - added by Douglas Hays 7 years ago.
stop TabBarButton? click from triggering actions downstream

Download all attachments as: .zip

Change History (17)

Changed 7 years ago by cjolif

comment:1 Changed 7 years ago by cjolif

Cc: avasiliu added
Keywords: todoapp added
Milestone: tbd1.8.1
Owner: changed from Eric Durocher to cjolif
Priority: undecidedhigh
Status: newassigned

comment:2 Changed 7 years ago by cjolif

Owner: changed from cjolif to Eric Durocher

comment:3 Changed 7 years ago by cjolif

Description: modified (diff)

comment:4 Changed 7 years ago by Adrian Vasiliu

I reproduce on both iPad 2 iOS 5.0.1 and iPhone 4S iOS 5.0.1.

It seems to go this way: the Opener registers an onclick listener to provide an easy way to close it. This listener is registered on the "cover" div created as a child of opener's domNode. Now, when using the onclick of a button for showing the Opening, clicking the button calls application's callback which shows the Opener. Then, after the end of Opener.show, on iPad/iPhone, the click is propagated to the cover/underlay element of the Opener which closes the freshly opened Opener...

As a workaround, the following modification in the test program does the trick:

require([
	"dojo/ready",
	"dojo/parser",
	"dojo/_base/event",
	"dijit/_base/manager"  // dijit.byId
	], function(ready, parser, event){
	ready(function(){
		parser.parse();
	});
		
	show = function(evt){
		// Avoid the propagation of the click event to the Opener
		event.stop(evt);
		dijit.byId('datePicker').show(this.domNode, ['above-centered','below-centered','after','before'])
	}
});

We will search for a better solution.

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

comment:5 Changed 7 years ago by Adrian Vasiliu

The mechanism used for sizing opener's "cover" div appears to be involved. In any case, in Opener.show, commenting out the first line below:

domStyle.set(this.cover, { top:'0px', left:'0px', width:'0px', height:'0px' }); // move cover temporarily to calculate domNode vertical position correctly
this._resizeCover(domGeometry.position(this.domNode, false)); // must be before this.inherited(arguments) for Tooltip sizing

solves the problem on iPad.

Changed 7 years ago by Adrian Vasiliu

Attachment: patch15786.patch added

Avoid that opener closes immediately after opening on tablets (in tooltip mode) - Adrian Vasiliu, IBM, CCLA

comment:6 Changed 7 years ago by Adrian Vasiliu

What I can say after a second round of investigation:

  • The bug hurts on both iOS and Android tablets (reproduced for instance on Samsung Galaxy Tab 10.1 Android 3.1). More exactly, on devices with screens such that the smallest dimension is larger than 500 ("dj_phone" class conditional addition by dojox/mobile/common), such the Opener acts as a Tooltip (versus Overlay).
  • I also reproduced on iPhone with the submitted test case as-is, simply because in this test case the phone vs. tab detection fails, thus the Opener acts as Tooltip, as if it would be a large tablet. This seems to be due to dojox/mobile modules not being explicitly required in the test file (side-effect of relying on auto-require, apparently; will be investigated separately).
  • The bug appears to be a side-effect of a small portion of changeset 27306 for dojox/trunk/mobile/Opener.js, which was intended as a fix for #14165.
  • More exactly, as I said in the previous comment, this is about the line of code with the comment "move cover temporarily to calculate domNode vertical position correctly".
  • The attached patch15786.patch is simply the removal of this line of code. I have tested on several iOS and Android phones and devices, and the trouble reported in this ticket goes away in the cases where it hurts without this patch, while I see no regression.
  • Since this patch reverts part of the fix for #14165, I have tried to make sure it does not reintroduce the trouble reported in this old ticket. For that, I tested the scenario reported for test_Opener-RoundSelectList-async.html, in particular on HTC Sensation Android 2.3.4 and HTC One X Android 4.0.4 (this ticket was reported for HTC EVO 4G, that I do not have). In all cases, the behavior of this test is similar with, and without my patch, that is HTC One X behaves just fine, while HTC Sensation misbehaves similarly with, and without the patch.
  • All in one, the attached patch is my best guess, while I do not fully understand what's going on here, and in all tests I could do the patch solves the problem without creating regressions.

comment:7 Changed 7 years ago by Adrian Vasiliu

Just to add the explanation about why the submitted test case fails to detect that the iPhone is a phone, not a tablet...

The phone/tablet detection is performed by dojox/mobile/common. This module is typically required very early, because often the apps require the dojox/mobile module (which requires it indirectly) and/or modules such as dojox/mobile/View and dojox/mobile/_base (which require dojox/mobile/common). Now, in the test case, none of dojox/mobile modules is required explicitly. Hence, during the auto-require, dojox/mobile/Opener is required before dojox/mobile/common. The later happens to be finally required because the test also contains a dojox/mobile/Heading in mark-up, which requires dojox/mobile/View, which finally requires dojox/mobile/common. But this is too late, since Opener has loaded already.

That said, there's nothing specific to the auto-require case here. It would be the same if the modules used in markup would be required explicitly in the same order as in mark-up. It seems we will need to add more dependencies on the base/common modules... (Update: registered as #15910).

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

Changed 7 years ago by Douglas Hays

Attachment: 15786.patch added

stop TabBarButton? click from triggering actions downstream

comment:8 Changed 7 years ago by Douglas Hays

My analysis is that the _onClick handler in TabBarButton is allowing it's touchend event to roam freely which eventually gets to the Overlay's cover object and closes it. A simple and safer fix would be to just call e.preventDefault() in the handler. However, I don't have a lot of devices to test this on for side-effects.

comment:9 Changed 7 years ago by Adrian Vasiliu

I just wonder whether it is a typical pattern for _onClick handlers to call e.preventDefault(). In any case, in dojox.mobile, this this *not* done by ListItem._onClick, nor ToolBarButton._onClick. Shouldn't all treat the events consistently?

On the other side, in dijit, dijit.form._ButtonMixin._onClick does call e.preventDefault() but only in some cases: if there's no user-defined action, plus other conditions depending on the type of the button. And its subclass dijit.form.Button conditionally calls both e.preventDefault() and e.stopPropagation(). Note that our TabBarButton can also get user-defined actions.

I can of course perform non-regression tests (tomorrow) for this proposed change in TabBarButton?, knowing that there are many situations to test (on many devices). All in one, if we need a quick fix for 1.8.1, maybe the change in Opener itself would have the advantage to be less risky because only affects the Opener itself? (while taking more time for a longer-run fix).

And a question: do you remember more about the line you added in Opener with the comment "move cover temporarily to calculate domNode vertical position correctly"? I mean, what and where something would break without it?

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

comment:10 Changed 7 years ago by cjolif

To answer the 1.8.1 question, there is no urgence for 1.8.1 on this. We do have a workaround (which is top listen to DOM event instead).

comment:11 Changed 7 years ago by Douglas Hays

I think I remember that setting the "cover" coordinates/size causes dojo.position to return the correct size (not artificially elongated in either direction) especially in regards to an orientation change. I don't think preventDefault will be risky. Most likely, other click events need the same change but we just don't have a testcase yet.

comment:12 Changed 7 years ago by Adrian Vasiliu

I found out from Christophe that the todoApp issue with the Opener originally holds with ListItems, not with TabBarButton as in the test case. So if we go for the e.preventDefault() solution we would need to add it in ListItem._onClick too (at minimum). But there are 14 implementations of _onClick in dojox.mobile, none of them currently doing e.preventDefault().

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

comment:13 Changed 7 years ago by cjolif

Milestone: 1.8.11.9

As we have an easy workaround, as there is not (yet) a consensus on the solution and as nobody but me is complaining/facing this, let's move that to 1.9 for now. We might put it back on 1.8 if other users complain.

comment:14 Changed 6 years ago by Eric Durocher

Resolution: fixed
Status: assignedclosed

Changeset [30784] also fixes the Opener problem, even without the "stop event" workaround, so I close this ticket.

Note: See TracTickets for help on using tickets.