Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#14410 closed defect (fixed)

[regression] TimeTextBox: arrow keys don't always work

Reported by: Douglas Hays Owned by: bill
Priority: high Milestone: 1.7.2
Component: Dijit - Form Version: 1.7.0
Keywords: Cc:
Blocked By: Blocking:

Description

Run test_TimeTextBox.html. Click the down arrow and try to use the keyboard down arrow to navigate the dropdown. They don't work. Started with [25997].

Change History (12)

comment:1 Changed 8 years ago by Douglas Hays

Owner: changed from Douglas Hays to bill

The referenced changeset just unmasked the problem. _HasDropDown:_onDropDownMouseDown is calling event.stop(e) and preventing the TimeTextBox? widget from receiving mousedown events. It should be e.preventDefault() instead.

comment:2 Changed 8 years ago by Douglas Hays

This worked prior to 1.7.0.

comment:3 Changed 8 years ago by bill

Summary: TimeTextBox arrow keys don't always work[regression] TimeTextBox: arrow keys don't always work

OK. Apparently the stopEvent() prevents the mousedown from focusing the <input>. (I guess that's what you meant.) I'll try changing it to evt.preventDefault() and see if it breaks anything.

This is an unusual case where the user uses the mouse to open the drop down, but then uses the arrow keys to navigate the drop down list. That's why the automated regression test didn't catch this.

Last edited 8 years ago by bill (previous) (diff)

comment:4 Changed 8 years ago by bill

OTOH, maybe calling stopEvent() is appropriate. Whenever an event is handled, we want to stop propagation so that an outer node doesn't also handle the event. For example, if the user clicks the [x] icon to close a tab, the tab isn't selected (as it's being deleted) because StackContainer.onClickCloseButton() calls evt.stopPropagation().

I'm not sure what [25997] has to do with this problem. I see it is referencing the admittedly confusing _Widget.focused attribute, set to true by the focus manager not only when a widget [or descendant widget] gets focus, but also when the user clicks a non-focusable part of the widget.

In 1.6 this problem doesn't occur because _FormWidget._onMouseDown() is called before _HasDropDown._onDropDownMouseDown(). That happens because _HasDropDown is listening to onmousedown on this.domNode rather than just on the arrow icon, and _HasDropDown happens to register it's onmousedown listener before _FormWidget does. In 1.7 it's listening on the arrow icon.

_HasDropDown.js does have a _stopClickEvents parameter which defaults to true, but can be overridden to false. Perhaps that should also control propagation of mousedown events, in addition to click events, although it wouldn't matter in this case since TimeTextBox leaves that flag at it's default value of true.

comment:5 Changed 8 years ago by Douglas Hays

I think this should invoke the user's onmousedown handler for both the textbox AND the arrow since it's part of the widget:

<input data-dojo-type="dijit.form.TimeTextBox"
        data-dojo-props='value:"T17:45:00",
                onMouseDown: function(){ console.log("onmousedown"); }'>

but it doesn't as long as the stopEvent call is present.

comment:6 Changed 8 years ago by bill

Hmm. Not sure that's the same issue, but do you think that ComboButton.onClick() should be invoked when the user clicks the ComboButton arrow?

Last edited 8 years ago by bill (previous) (diff)

comment:7 Changed 8 years ago by Douglas Hays

onClick seems like an action defined by the widget, and onMouseDown should work just like onFocus (which fires whether you click on the textbox or the arrow) since they represent transient states. onMouseDown gets an event parameter and, if necessary, it's easy enough to know if it's over the arrow or not using the target attribute of the event.

comment:8 Changed 8 years ago by bill

Milestone: 1.7.11.7.2

These didn't make it into the 1.7.1RC, so bumping them to 1.7.2 (as stated in the email I sent yesterday).

comment:9 Changed 8 years ago by bill

OK, I'll check in the change to use evt.preventDefault(), hopefully it won't have any repercussions.

comment:10 Changed 8 years ago by bill

In [27394]:

Let mousedown event on _HasDropDown button node propagate, to fix a number of errors. Fix on trunk/, refs #14408, #14410, !strict.

comment:3 Changed 8 years ago by bill

Resolution: fixed
Status: newclosed

In [27410]:

Let mousedown event on _HasDropDown button node propagate, to fix a number of errors. Fixes #14408, #14410 on 1.7/ branch, !strict.

comment:4 Changed 7 years ago by bill

In [28814]:

Backport [27394] to 1.6 branch, using preventDefault() rather than stopEvent(), fixes #13790, refs #12517, #14408, #14410 !strict.

Note: See TracTickets for help on using tickets.