Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#13372 closed defect (fixed)

MVC does not work with dijit.form.Select

Reported by: ben hockey Owned by: Ed Chatelain
Priority: high Milestone: 1.8
Component: DojoX MVC Version: 1.7.0b1
Keywords: Cc: rahul
Blocked By: Blocking:

Description

MVC just does not work at all with dijit.form.Select.

i've found 2 issues so far:

  • the initial value of the select is not set to the value represented by ref
  • when you try to open the drop down of a select there is an error that dijit.form._SelectMenu has not method '_dbstartup'. this seems like something is messed up with the inheritance chain.

you can see these both demonstrated in the file i'm attaching to this ticket.

Attachments (6)

13372.html (2.2 KB) - added by ben hockey 8 years ago.
13372-declare_debug.diff (1.4 KB) - added by ben hockey 8 years ago.
debugging patch for dojo.declare
13372-c3mro.html (1.4 KB) - added by ben hockey 8 years ago.
c3mro debugging
mvc-13372-kitchensink-test-patch.txt (82.2 KB) - added by Ed Chatelain 8 years ago.
This patch contains the mvc-form-kitchensink tests. There will be one failure if the _FormSelectWidget patch is not applied first.
_FormSelectWidget-patch.txt (1.1 KB) - added by Ed Chatelain 8 years ago.
Here is a patch to move the code to setup the store from startup to postCreate, I also removed the odd connect call for _loadChildren and moved that call into startup. It solves this problem, and all of the dijit form tests related to select passed.
mvc-13372-testcase-update.patch (853 bytes) - added by Ed Chatelain 8 years ago.
Updated testcase to go along with the change.

Download all attachments as: .zip

Change History (39)

Changed 8 years ago by ben hockey

Attachment: 13372.html added

comment:1 Changed 8 years ago by ben hockey

i meant to add that the 2nd problem can be seen even if the widget is not data-bound. just the inclusion of dojox/mvc anywhere in the app will cause this problem.

comment:2 Changed 8 years ago by Ed Chatelain

It looks like the 2nd problem is in _patches.js, it is calling this._dbstartup(), but it should be checking to be sure that this._dbstartup is not undefined before calling it. For some reason the _DataBindingMixin which has the _dbstartup function is not being mixed into dijit.form._SelectMenu. This seems to fix the 2nd problem:

if(this._dbstartup){

this._dbstartup();

}

I will try to figure out why the initial value is not getting set correctly tomorrow.

comment:3 in reply to:  2 Changed 8 years ago by ben hockey

Replying to edchat:

This seems to fix the 2nd problem:

if(this._dbstartup){

this._dbstartup();

}

that will stop the errors but i'm not sure it fixes the problem. all widgets derived from dijit._WidgetBase should have _dbstartup after the patches are applied. the problem is that in this case we have a widget that doesn't have _dbstartup which will lead to failures due to the widget not binding to its parent.

it smells like there's something strange with the inheritance chain. i'll let you know if i can find anything.

comment:4 Changed 8 years ago by rahul

Right, defensive code is one thing, but it'd be good to also note why the method isn't there -- that will point us in the right direction.

comment:5 Changed 8 years ago by Ed Chatelain

I agree, the check stops the error and allows the drop-down to display, but we do need to find the inheritance problem. I have not been able to figure it out yet, but interestingly FilteringSelect? does not have either problem. I suspect that if we fix the inheritance problem it will fix both problems.

comment:6 Changed 8 years ago by ben hockey

Cc: Eugene Lazutkin added

i've spent a few hours looking at it and i can't figure out why it's different but anything derived from dijit._MenuBase does not get affected by the patches. it's also possible that there are other classes besides dijit._MenuBase where this is happening.

i know that dojo.declare resolves inheritance chains in a specific way that means that cases can exist where updating the prototype of a superclass doesn't affect the subclasses as would normally happen with prototypical inheritance. afaik, only one class in the chain is truly a superclass and the rest are all copied/mixed in. anyone familiar with c3mro? i understand the concept but when it comes to analyzing why its causing this specific problem it's beyond my current understanding.

given how dojo.declare works, i had realized that it would be possible that classes might exist that wouldn't be affected by the patches. however, the only alternative to extending the dijit._WidgetBase prototype is to take a different approach where the user needs to explicitly mixin a class to every class that they want to use with mvc. i know that's not at all desirable.

i copied eugene since he wrote the current version of dojo.declare and maybe he can explain what it is about dijit._MenuBase that causes it (and its subclasses) to become "detached" from the dijit._WidgetBase prototype.

comment:7 Changed 8 years ago by bill

There are a couple weird things, which I've mentioned previously. Specifically, the way MVC adds functions to _WidgetBase is overcomplicated. It first does:

declare("dojox.mvc._DataBindingMixin", { ... } );

and then does:

lang.extend(wb, new dbm());

(where dbm == _DataBindingMixin). Both the declare() and the "new" are unnecessary, it can/should simply be:

lang.extend(wb, { ... } );

Not sure if that's the cause of the problem or not.

The dojo.declare() documentation also claims that there's an extend method on declared objects:

//	|	var B = dojo.declare(A, {
//	|		m1: function(){
//	|			this.inherited(arguments);
//	|			console.log("B.m1");
//	|		}
//	|	});
//	|	B.extend({
//	|		m2: function(){
//	|			this.inherited(arguments);
//	|			console.log("B.m2");
//	|		}
//	|	});

However, it doesn't seem to be true.

I also noticed that the "monkey patch dijit._WidgetBase.destroy" code can be done using aspect.before(), although that's likely unrelated.

comment:8 Changed 8 years ago by bill

PS: although the above problem should be fixed, it's probably unrelated to the MVC/Select issue. Because I've also noticed a failure in dijit/themes/themeTester-orig.html on IE6 where the root cause is that MenuBar doesn't inherit the layoutPriority flag created in a dojo.extend(). Mailed Eugene about it a while ago; it happens without MVC.

comment:9 Changed 8 years ago by rahul

Yes, the overcomplication has been mentioned, I didn't get around to reply to it. The basis for that pattern is to indicate that _DataBindingMixin is a potential unit of reuse. At this point the only thing we're extending is _WidgetBase, though any other widget or library (custom) that provides a dojo.Stateful contract as dijit does may, in theory, be extended similarly and thereby become a data-bound widget in the dojox.mvc sense.

comment:10 Changed 8 years ago by ben hockey

this very unintuitive patch fixes the inheritance problem

  • dijit/_KeyNavContainer.js

    ### Eclipse Workspace Patch 1.0
    #P dojo trunk
     
    1616        // summary:
    1717        //              A _Container with keyboard navigation of its children.
    1818
    19         dojo.declare("dijit._KeyNavContainer", [dijit._Container, dijit._FocusMixin], {
     19        dojo.declare("dijit._KeyNavContainer", [dijit._FocusMixin, dijit._Container], {
    2020
    2121                // summary:
    2222                //              A _Container with keyboard navigation of its children.

Changed 8 years ago by ben hockey

Attachment: 13372-declare_debug.diff added

debugging patch for dojo.declare

Changed 8 years ago by ben hockey

Attachment: 13372-c3mro.html added

c3mro debugging

comment:11 Changed 8 years ago by ben hockey

the 2 files i just attached were helpful for debugging the inheritance chain. they may be useful to someone else so i attached them for later reference.

comment:12 Changed 8 years ago by ben hockey

sorry for the noise... one more thing to note. the 13372-c3mro.html file represents what the classes would look like if the patch in comment:10 was applied. to see the state of the classes without that patch, swap the order of the G and H dependencies in the declaration of the D class.

comment:13 Changed 8 years ago by ben hockey

bill, does the patch above fix your MenuBar? issue in IE6? can it be committed?

comment:14 Changed 8 years ago by bill

I could try it, but the patch is a workaround to a dojo.declare() bug, right? Seems like we should fix the actual bug.

comment:15 Changed 8 years ago by ben hockey

i'm fairly certain its not a bug of dojo.declare - its the way it is supposed to work. eugene might be able to shed some more light but see http://www.python.org/download/releases/2.3/mro/ - it was a lot for me to understand in depth and is why i attached the debugging files that helped me. if you don't have the time to spend to get a deep understanding of it (it took me a day and a half of debugging to come up with this patch) then take my word that there is no bug with dojo.declare.

by luck more than by design changing the dijit._WidgetBase prototype will affect subclasses. however, using c3mro this is not a given. before eugene refactored dojo.declare to use c3mro, it was my understanding that the prototype chain was kept in tact (multiple inheritance) but with c3mro the prototype chain is not guaranteed to be preserved (single inheritance). i opened #10788 at the time when i started to observe some unexpected changes in behavior but based on the discussion in that ticket i became aware that c3mro uses single inheritance and mixes in the other classes (see http://bugs.dojotoolkit.org/ticket/10788#comment:13) such that a situation could be possible where dijit._WidgetBase may be mixed in rather than part of the prototype chain.

the declaration order in _KeyNavContainer dictates that properties from _Container should be overriden by _FocusMixin properties. _Widget is declared in such a way that properties from _FocusMixin should override _WidgetBase. in _MenuBase this leads to the following linearization:

dijit._MenuBase -> dijit._KeyNavContainer -> dijit._TemplatedMixin -> dijit._Widget -> dijit._FocusMixin -> dijit._Container -> dijit._OnDijitClickMixin -> dijit._WidgetBase -> dojo.Stateful

and everything is mixed into dojo.Stateful.

when you apply the patch, now properties from _Container will override _FocusMixin and the linearization of dijit._MenuBase is:

dijit._MenuBase -> dijit._KeyNavContainer -> dijit._Container -> dijit._TemplatedMixin -> dijit._Widget -> dijit._FocusMixin -> dijit._OnDijitClickMixin -> dijit._WidgetBase -> dojo.Stateful

this makes dijit._Widget the superclass and only _TemplatedMixin, _Container, _KeyNavContainer and _MenuBase are mixed in because the rest of the linearization after dijit._Widget matches the linearization for dijit._Widget.

i expect that this may not be easy to understand (a week ago i would have not seen the problem) but the bottom line is that dojo.declare does not have a bug and if you want subclasses of dijit._MenuBase to reflect changes to the prototype of dijit._WidgetBase then apply the patch. its a harmless change because _FocusMixin and _Container do not have any overlapping properties so apart from this inheritance chain, there is no difference in which order they are applied. given how the c3mro linearization works my real surprise is that we never saw something like this earlier.

comment:16 Changed 8 years ago by bill

Cc: rahul added
Milestone: tbd1.7
Owner: changed from rahul to bill
Status: newassigned

Wow, so _WidgetBase was getting mixed in rather than being inherited. Thanks for tracking that down. Yes, the change you suggested fixes the Menu problem too. I'll check it in with a test case for that Menu problem. Someone should add a test to the MVC code for dijit.form.Select.

comment:17 Changed 8 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [25769]) Fix inheritance chain so that _WidgetBase is (prototypically) inherited by widgets, rather than being a mixin. Thanks to Ben for tracking all this down and the patch. Fixes #13372 !strict.

comment:18 Changed 8 years ago by Ed Chatelain

Ben, are you getting the initial value set correctly now? I am still seeing the first problem "the initial value of the select is not set to the value represented by ref". But the update does fix the second problem. I will add an MVC test for dijit.form.Select.

comment:19 Changed 8 years ago by ben hockey

Resolution: fixed
Status: closedreopened

ed, you beat me to it... the initial problem still exists so i'll reopen this ticket since that was the main problem. in addition to the test for dijit.form.Select it would probably be good to add tests for all widgets with values. this would be all of dijit.form as well as dijit.Calendar, dijit.Editor, ...? i think most of the existing tests only use some form of TextBox?.

comment:20 Changed 8 years ago by ben hockey

Owner: changed from bill to rahul
Status: reopenednew

comment:21 Changed 8 years ago by rahul

Ed, thanks for offering to write up the test. It'd be great if you could make that test a kitchen sink test for all form dijits, that'll give us full test coverage and identify all that need working through.

comment:22 Changed 8 years ago by Ed Chatelain

I am finally able to get back to working on this, and I am looking for suggestions on how to handle this problem. The problem with the setting of the initial value is that there is a race condition where the value gets set to 1 by the _DataBindingMixin (line 281) for the ref, but that value is overwritten to 8 in _FormSelectWidget line 271 in the onComplete: function.

comment:23 Changed 8 years ago by Ed Chatelain

I guess I could just set the initial value on the Select in addition to setting the ref for the Select test. Like this: var sel = new dijit.form.Select({

store: store, value: 1, ref: model.number bind to model.number

}, document.getElementById('sel'));

By the way, the problem setting the initial value from the ref does not happen with a FilteringSelect? or ComboBox?.

comment:24 Changed 8 years ago by ben hockey

yeah - i think the problem is unrelated to what you've mentioned. the problem happens earlier. in my brief look at it, when the binding tries to set the value, it fails because the call to getOptions (http://bugs.dojotoolkit.org/browser/dojo/dijit/trunk/form/_FormSelectWidget.js?rev=25900#L306) returns no options which causes the value to be ignored due to (http://bugs.dojotoolkit.org/browser/dojo/dijit/trunk/form/_FormSelectWidget.js?rev=25900#L322)

i believe the problem would be solved if the options existed at that time.

comment:25 Changed 8 years ago by ben hockey

...and the reason there are no options is because the store is being set way too late. http://bugs.dojotoolkit.org/browser/dojo/dijit/trunk/form/_FormSelectWidget.js?rev=25900#L525

this is starting to smell like a dijit bug to me.

comment:26 Changed 8 years ago by bill

That does look dodgy, I don't know why it's setting the store so late, That setStore() function is tricky since it takes a number of arguments, but sure seems like the setStore() code in startup() could be moved to postCreate().

Changed 8 years ago by Ed Chatelain

This patch contains the mvc-form-kitchensink tests. There will be one failure if the _FormSelectWidget patch is not applied first.

comment:27 Changed 8 years ago by Ed Chatelain

After talking to Doug about this, we decided to remove my patch which had this comment: ("I tried Bill's suggestion to move the store setup from startup to postCreate, and it did fix the problem. It also passed all of the dijit form tests. Here is the patch. ") pending a little more investigation.

Doug was concerned about an async load case for the store, but it turns out that the async load case is not a problem, since this._loadingStore gets set true before the mvc code tries to set the value, so code in _setValueAttr will save the value away in this._pendingValue and use it when the store load is complete if this._loadingStore is true.

Another fix for this problem would be to change _FormSelectWidget's startup function to call this.inherited(arguments); at the end instead of the beginning, this would have the mvc code run after this._loadingStore is set true, and it may be a safer change. I was able to successfully run all of the dijit.form tests with this change, and I ran some of the related dojox.form tests.

Doug also had a question about why postCreate was calling: this.connect(this, "startup", "_loadChildren");

I did not see a difference with or without this connection setup in the tests, but I did see that the _loadChildren being called was the one in dijit.form.Select, and that the one in _FormSelectWidget was not called unless the drop-down is displayed, because _loadChildren in Select checks if(loadMenuItems === true)before it calls this.inherited(arguments);

Doug and I also talked about waiting on this fix until after 1.7, if we are close to shipping dojo 1.7, we may want to wait on making this fix since it is not a major problem. We can fix it in 1.8.

Last edited 8 years ago by Ed Chatelain (previous) (diff)

comment:28 Changed 8 years ago by Ed Chatelain

I added the kitchen sink tests along with other updates via this changeset: In [26567] which was made for ticket #13773. But this ticket should remain open until the initialization of the dijit.form.Select is fixed. There is a test in the kitchensink test dealing with the initial setting of the dijit.form.Select which I have commented out for now. Once this is fixed that test should be uncommented.

comment:29 Changed 8 years ago by Adam Peller

Cc: Eugene Lazutkin removed
Owner: changed from rahul to Ed Chatelain

Changed 8 years ago by Ed Chatelain

Attachment: _FormSelectWidget-patch.txt added

Here is a patch to move the code to setup the store from startup to postCreate, I also removed the odd connect call for _loadChildren and moved that call into startup. It solves this problem, and all of the dijit form tests related to select passed.

comment:30 Changed 8 years ago by Douglas Hays

The latest patch looks good to me.

Changed 8 years ago by Ed Chatelain

Updated testcase to go along with the change.

comment:31 Changed 8 years ago by cjolif

Resolution: fixed
Status: newclosed

In [27741]:

fixes #13372. MVC does not work with dijit.form.Select. Thanks edchat (IBM, CCLA) for the patch and doughays for the review. !strict.

comment:32 Changed 8 years ago by cjolif

In [27742]:

refs #13372. Test case. Thanks edchat.

comment:33 Changed 8 years ago by Ed Chatelain

#14448 is a duplicate of this ticket.

Note: See TracTickets for help on using tickets.