Opened 6 years ago

Closed 6 years ago

#17040 closed defect (fixed)

dijit/form/Select not an instance of dijit/_WidgetBase

Reported by: Brian Arnold Owned by: Douglas Hays
Priority: undecided Milestone: 1.9
Component: Dijit - Form Version: 1.9.0b2
Keywords: Cc: cjolif
Blocked By: Blocking:

Description (last modified by Brian Arnold)

While I realize that instanceof is not reliable for mixins and such, it is typically reliable to do things like object instanceof _WidgetBase to determine that something is a widget, and in 1.8 and prior, this worked quite well pretty much across the board.

However, part of r30620 was a refactoring of _KeyNavMixin and such, and it changed the way that dijit/form/Select is declared to the following:

declare("dijit.form.Select" + (has("dojo-bidi") ? "_NoBidi" : ""), [_KeyNavMixin, _FormSelectWidget, _HasDropDown], {

You'll notice that now, the base class for dijit/form/Select is _KeyNavMixin, and not _FormSelectWidget as it had been previously.

I can use object.isInstanceOf and that works just fine, but this seems kind of broken. If I swap the order of _KeyNavMixin and _FormSelectWidget, the robot tests break, so it's not as trivial as a one line change, unfortunately.

Attachments (2)

17040.patch (1.2 KB) - added by Douglas Hays 6 years ago.
add _WidgetBase to inheritance chain so instanceof works
anyOrder.patch (1.9 KB) - added by bill 6 years ago.
don't depend on inheritance order

Download all attachments as: .zip

Change History (7)

comment:1 Changed 6 years ago by Brian Arnold

Description: modified (diff)

Changed 6 years ago by Douglas Hays

Attachment: 17040.patch added

add _WidgetBase to inheritance chain so instanceof works

comment:2 Changed 6 years ago by bill

I tried to do a cleaner (but slightly bigger) patch where the inheritance order doesn't matter, and also there's no issue with the order of operations in postCreate(), which as a lifecycle creation method should really called this.inherited() before doing it's own thing. The main part of the change was rolling back the [31124] change to _HasDropDown.

It led to a strange problem in test_Select.html where mouseup on the first Select automatically selected Tennessee. Turns out it was that the mouseup listener setup in _FormWidgetMixin::_onFocus() calls this.focus(), which (after my change) calls _KeyNavMixin::focus() rather than _FormWidgetMixin::focus(), which then calls focusFirstChidl(). This is because Select fragilely extends _KeyNavMixin even though SelectMenu is the container of the items being navigated.

So then I had to workaround that problem, and things are working. See attached patch, does it look good to you? Although the change is bigger is seems safer long term, and given that the combination of _KeyNavMixin and _HasDropDown is a new thing for this release, I think it should go into 1.9.

Version 0, edited 6 years ago by bill (next)

Changed 6 years ago by bill

Attachment: anyOrder.patch added

don't depend on inheritance order

comment:3 Changed 6 years ago by cjolif

Cc: cjolif added

comment:4 Changed 6 years ago by Douglas Hays

Bill, I tested this on several browsers and it's working well on each. Go ahead and commit this when you've finished testing.

comment:5 Changed 6 years ago by bill

Resolution: fixed
Status: newclosed

In [31291]:

Roll back the [31124] change to _HasDropDown so it can come before or after _KeyNavMixin in the inheritance chain, thus allowing Select to have a saner list of superclasses. Fixes #17040, refs #16589 !strict.

Note: See TracTickets for help on using tickets.