Opened 8 years ago

Closed 7 years ago

Last modified 6 years ago

#12934 closed enhancement (fixed)

Apply new dojo.touch to dojo.dnd Movable & dijit

Reported by: evan Owned by: evan
Priority: high Milestone: 1.7
Component: Core Version: 1.6.1
Keywords: touch, dnd, dijit Cc: bill, Eugene Lazutkin, Kris Zyp, Evan
Blocked By: Blocking:

Description (last modified by bill)

We will use this ticket to track patch/changes of applying new dojo.touch to dojo.dnd and dijit.

The changes to dojo.dnd are only for dojo.dnd.Movable only, since the drag-and-drop code in dojo.dnd has an architecture that's incompatible with mobile: the drag-and-drop code works based on mouseenter/mouseleave events, which aren't present on mobile and have no touch equivalents.

Attachments (2)

12934-dijit.patch (1.9 KB) - added by evan 8 years ago.
Remove e.touches[] dependecies in dijit, also mapping mousexxx to dojo.touch.* for dijit widgets
12934-dojo-dnd.patch (5.7 KB) - added by evan 8 years ago.
Remove e.touches[] dependecies in dojo.dnd, also add a new dojo.touch.getHandle() to get an appropriate touch handle for the given event name

Download all attachments as: .zip

Change History (23)

comment:1 Changed 8 years ago by Chris Mitchell

Milestone: tbd1.8
Type: taskenhancement

Border container splitters need touch drag

Changed 8 years ago by evan

Attachment: 12934-dijit.patch added

Remove e.touches[] dependecies in dijit, also mapping mousexxx to dojo.touch.* for dijit widgets

Changed 8 years ago by evan

Attachment: 12934-dojo-dnd.patch added

Remove e.touches[] dependecies in dojo.dnd, also add a new dojo.touch.getHandle() to get an appropriate touch handle for the given event name

comment:2 Changed 8 years ago by Eugene Lazutkin

Cc: Eugene Lazutkin added

comment:3 Changed 8 years ago by Eugene Lazutkin

Do you plan to add a patch for the main DnD functionality (Container, Selector, Source)?

comment:4 in reply to:  3 ; Changed 8 years ago by evan

Yep, but that might depend on some new touch & gesture stuff for 1.8, e.g. for "ondragstart" we may need a dojo.gesture.drag.start to be run well on pc and mobile. Another issue might be missed correspondence for onmouseover/out on mobiles etc.

Replying to elazutkin:

Do you plan to add a patch for the main DnD functionality (Container, Selector, Source)?

comment:5 in reply to:  4 Changed 8 years ago by Eugene Lazutkin

Replying to evan:

Yep, but that might depend on some new touch & gesture stuff for 1.8, e.g. for "ondragstart" we may need a dojo.gesture.drag.start to be run well on pc and mobile.

I think it is solvable.

Another issue might be missed correspondence for onmouseover/out on mobiles etc.

Yep, we need to think this part through. I hate to resort to geometry calculations, which proved to be a source of many problems in the past. Maybe we need a special gesture for moving/copying items from container to container.

comment:6 Changed 8 years ago by bill

This isn't quite the direction I expected/intended for dijit. I understand how templates need to use strings for event names (as opposed to objects like dojo.touch.press), and how you are trying to map those strings to dojo.touch.* events via the getHandle() function.

However, I don't want to overload existing event names like "onmousedown" to become dojo.touch.press. There should be new names used in templates: "touch", "move", and "release". (This is the same discussion we went through originally with the dojo.touch code.)

Note that this pattern already exists in dijit: "ondijitclick" maps to the synthetic event defined in OnDijitClick.js. So, I think it's just a question of setting up a registry (ie: a hash) to hold those mappings and then making _WidgetBase.on() check that registry.

Is there anything I'm missing/misunderstanding?

comment:7 in reply to:  6 Changed 8 years ago by evan

Replying to bill:

This isn't quite the direction I expected/intended for dijit. I understand how templates need to use strings for event names (as opposed to objects like dojo.touch.press), and how you are trying to map those strings to dojo.touch.* events via the getHandle() function.

However, I don't want to overload existing event names like "onmousedown" to become dojo.touch.press. There should be new names used in templates: "touch", "move", and "release". (This is the same discussion we went through originally with the dojo.touch code.)

Yep, exactly, we discussed this previously but finally chose to use the new listen way - dojo.touch.press or dojo.gesture.swipe etc.

The major reason not using string events was we want to make gestures loaded on demand, but string gesture events will cause events/connect capability varies depending on widgets' loading sequence

Note that this pattern already exists in dijit: "ondijitclick" maps to the synthetic event defined in OnDijitClick.js. So, I think it's just a question of setting up a registry (ie: a hash) to hold those mappings and then making _WidgetBase.on() check that registry.

We can surely do that by making touch loaded by default for dijit, but I think it's better to only provide one kind of usage(string event vs. func event) to avoid confusions for end users?

Is there anything I'm missing/misunderstanding?

comment:8 Changed 8 years ago by evan

Cc: Kris Zyp added

comment:9 Changed 8 years ago by bill

Seems like you misunderstood my comments.

The major reason not using string events...

Your patch is already using string events. It is converting:

dojoAttachEvent="onmousepress: myfunc"

into

this.connect(node, dojo.touch.press, myfunc);

The problem is that onmousepress's meaning should not be altered. There should be a new word to map to dojo.touch.press.

We can surely do that by making touch loaded by default for dijit,

Your patch is loading touch as part of _TemplatedMixin. I'm not suggesting to change that.

comment:10 in reply to:  9 Changed 8 years ago by evan

Replying to bill:

Seems like you misunderstood my comments.

The major reason not using string events...

Your patch is already using string events. It is converting:

dojoAttachEvent="onmousepress: myfunc"

into

this.connect(node, dojo.touch.press, myfunc);

The problem is that onmousepress's meaning should not be altered. There should be a new word to map to dojo.touch.press.

Guess you mean the existing dojoAttachEvent="onmousedown: myfunc" ?

By string event I mean something like

dojo.connect(node, "press", func);
dojo.connect(node, "swipe", func);

Which will be used by end users, or we just keep the string events internally in dijit?

We can surely do that by making touch loaded by default for dijit,

Your patch is loading touch as part of _TemplatedMixin. I'm not suggesting to change that.

comment:11 Changed 8 years ago by bill

Milestone: 1.81.7

Sorry for the misunderstanding. Anyway I'll check in the part for dijit, but without changing the existing meaning of onmousedown/onmouseup.

comment:12 Changed 8 years ago by bill

(In [25151]) Fixes for dijit widgets on mobile:

  • clicking blank area of screen to close dropdown
  • clicking arrow icon to open dropdown for DateTextBox/ComboBox/etc.
  • dragging BorderContainer splitter
  • clicking slider bar to adjust slider value
  • CSS active effect when pressing button

Unresolved mobile issues:

  • have to click twice to close menu (first click doesn't even generate an onclick event, not sure why)
  • opening TooltipDialog/Dialog/InlineEditBox doesn't focus [first] field
  • Dialog position doesn't readjust when keyboard opened or phone orientation changed (portrait to landscape)

Changes:

  • use dojo.touch.* rather than mousexxx events
  • added press, move, release options for dojoAttachEvent, ex: dojoAttachEvent="press: _onMouseDown", that correspond to dojo.touch events
  • remove e.touches[] normalization code since that's now handled by dojo/on.

Not adjusting:

  • Editor since that has many issues on mobile.
  • deprecated SplitContainer widget

Refs #12934, fixes #12250 !strict

comment:13 Changed 8 years ago by bill

(In [25196]) Use async AMD loader, refs #13056, #12934.

comment:14 Changed 8 years ago by bill

Resolution: fixed
Status: newclosed

(In [25666]) Use dojo.touch code in dojo.dnd, fixes #12934, #13135. (I checked w/Eugene, he said Evan's patch looked good.)

comment:15 Changed 8 years ago by Evan

Cc: Evan added

comment:16 Changed 8 years ago by bill

In [26289]:

missing dependency, refs #12934

comment:17 Changed 7 years ago by nickmaynard

Resolution: fixed
Status: closedreopened

dojo.touch has not been retrofitted to all of dojo.dnd - touch only works in dojo.dnd.Mover and dojo.dnd.Moveable. Not surprising as patch [25666] only changes these files.

It appears that this effort is not complete - all other dojo.dnd components still need work.

Last edited 7 years ago by nickmaynard (previous) (diff)

comment:18 Changed 7 years ago by bill

@nickmaynard - is there any point to use dojo.touch in DnD besides for for drag-move?

Using dojo.dnd for actual DnD (as opposed to drag-move) doesn't work on mobile anyway, and switching to dojo.touch won't help. The problem is that mobile won't generate mouseover/mouseout events as you drag an element. And the solution is to use dojox/mdnd.

comment:19 Changed 7 years ago by nickmaynard

Sadly dojox/mdnd doesn't work with touch devices either (tested on my iPad). Looking at the code, it connects directly with mouse events; no use of dojo/on's event normalization, or dojo/touch.

Beyond Moveable, which is way too raw to avoid wheel reinvention, I'm having trouble finding any real support for DnD on touch in Dojo right now.

comment:20 Changed 7 years ago by bill

Description: modified (diff)
Resolution: fixed
Status: reopenedclosed
Summary: Apply new dojo.touch to dojo.dnd & dijitApply new dojo.touch to dojo.dnd Movable & dijit

That is too bad. You should file a separate ticket for that, or even better yet supply a patch (file a cla first).

I'll update this ticket's summary to be accurate about the work that was already done. If for some reason you think dojo.dnd's drag-and-drop code should be updated too, you can file a separate ticket, but I don't see any point to updating the other dojo.dnd modules.

comment:21 Changed 6 years ago by bill

In [30857]:

fix radio buttons in test so clicking one deselects the other, refs #12934

Note: See TracTickets for help on using tickets.