Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#17519 closed defect (fixed)

A customized menu widget with RadioMenuItem, once destroyed, cannot be displayed again

Reported by: sunxcint Owned by: bill
Priority: undecided Milestone: 1.10
Component: Dijit Version: 1.9.1
Keywords: Cc:
Blocked By: Blocking:

Description

The following step will fail with current RadioMenuItem? code:

  1. Declare a widget which is a menu with RadioMenuItem?;
  2. Instantiate it and add it to the page DOM;
  3. Destroy it;
  4. Create a new instance of the widget declared in step 1 and add it to the DOM.

The error is

Uncaught Error: dijit._WidgetsInTemplateMixin: parser returned unfilled promise (probably waiting for module auto-load), unsupported by _WidgetsInTemplateMixin. Must pre-load all supporting widgets before instantiation.

The test case is attached below.

Attachments (3)

test_RadioMenuItem.html (2.7 KB) - added by sunxcint 6 years ago.
patch.txt (438 bytes) - added by sunxcint 6 years ago.
patch_alternative.txt (2.5 KB) - added by sunxcint 6 years ago.

Download all attachments as: .zip

Change History (22)

Changed 6 years ago by sunxcint

Attachment: test_RadioMenuItem.html added

comment:1 Changed 6 years ago by sunxcint

RadioMenuItem? class has a property named '_currentlyChecked' in its prototype which logs checked item in each group created with this class. Because it is a property in prototype, it is shared and might be modified by all its instances. However, when a group is destroyed, it will not delete its corresponding value in this property. When another group with same group id is added to the DOM(e.g. through template), the old group is thought to be still existing and its undefined domNode will be visited and cause error.

I will attach a patch to fix this although I think the method a group of RadioMenuItems? use to share checked information(i.e. through prototype) is not a good idea.

Changed 6 years ago by sunxcint

Attachment: patch.txt added

comment:2 Changed 6 years ago by bill

Component: GeneralDijit
Owner: set to bill

RadioMenuItem? class has a property named '_currentlyChecked' when a group is destroyed, it will not delete its corresponding value in this property.

Ah, right, makes sense.

I think the method a group of RadioMenuItems? use to share checked information(i.e. through prototype) is not a good idea

When you select one RadioMenuItem? it needs to deselect the previously selected one. But I'm open to suggestions of another way to do that.

Changed 6 years ago by sunxcint

Attachment: patch_alternative.txt added

comment:3 Changed 6 years ago by sunxcint

I added another patch named "patch_alternative.txt" which uses the method similar to that in RadioButton? to deselect previously selected menu items.

comment:4 Changed 6 years ago by bill

Milestone: tbd1.10
Status: newassigned

OK thanks... well that will work in this case but you'll get problems if the template is changed to not be a TR, for example if there was a RadioMenuBarItem? widget. Performance is the other concern; hopefully the group=... in the query makes it fast though.

Also I can't really take the patch if you haven't signed a http://dojofoundation.org/about/cla.

I'll check in the simple fix for now.

PS: Another approach is for _setCheckedAttr() to iterate this.getParent().getChildren(), assuming that all the RadioMenuItem? are in the same Menu.

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

comment:5 Changed 6 years ago by Bill Keese <bill@…>

Resolution: fixed
Status: assignedclosed

In 57966f299a6c0b58933bad5566525cb268eccbaf/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:6 in reply to:  4 Changed 6 years ago by sunxcint

Replying to bill:

OK thanks... well that will work in this case but you'll get problems if the template is changed to not be a TR, for example if there was a RadioMenuBarItem? widget. Performance is the other concern; hopefully the group=... in the query makes it fast though.

I was also considering TR issue. Currently I prefer the CSS selector to be

"[group=" + this.group + "][role=" + this.role + "]"

I had two experiments on the performance. I logs the time of 10000 queries for

"[group=" + this.group + "][role=" + this.role + "]", "tr[group=" + this.group + "][role=" + this.role + "]", "[group=" + this.group + "][role=" + this.role + "]" and "tr"

respectively. The results are 1000+ms, 220+ms, 220+ms and 180+ms. Will you please let me know whether 220+ms is acceptable?

comment:7 in reply to:  4 Changed 6 years ago by sunxcint

Also I can't really take the patch if you haven't signed a ​http://dojofoundation.org/about/cla.

I have signed it with email sunxcint@….

comment:8 Changed 6 years ago by bill

OK thanks, I see your CLA now.

If the performance is <1ms to find the currently selected RadioMenuItem? that's definitely fine, although you didn't tell me how many DOM nodes are in your benchmark; that will affect that result. Another thing that affects the result is how many nodes there are with group=this.gropu, but I don't expect there to be many. I'm also unclear why you are filtering by role?

comment:9 Changed 6 years ago by sunxcint

I assume that the role value "menuitemradio" indicates that a filtered item is a RadioMenuItem? and the group id indicates that an item is subject to a certain group of RadioMenuItem?. So the combination of the two values can ensure logically that a filtered item is what we need.

The queries time came from the test case file I attached.

From another perspective, the difference of the query time between

"[group=" + this.group + "][role=" + this.role + "]"

and "tr" is not so big. I also verified that the query time for

"tr[group=" + this.group + "]"

is always bigger than "tr". Similarly, the query time for "input[type=radio]" should always be bigger than "input". So with the assumption that the query time of "tr" and "input" are about the same, now that "input[type=radio]" is used in RadioButton? I think the query time for

"[group=" + this.group + "][role=" + this.role + "]"

should be acceptable as well. This is just a coarse evaluation. Will you please let me know how dojo tests its performance for this kind of issue (or which kind of value is acceptable)?

comment:10 Changed 6 years ago by bill

The queries time came from the test case file I attached.

OK, you should try a more realistic test page like dijit/tests/test_Menu.html itself, but it sounds like the time will be fine. As long as its less than 10ms (for processing a single click on a RadioMenuItem?) I wouldn't worry at all. 100ms is borderline. Remember though something like an iPad runs about 5x slower than Chrome on a desktop.

Will you please let me know how dojo tests its performance for this kind of issue (or which kind of value is acceptable)?

We don't have any official performance tests for this type of thing; the performance concerns are almost always about page load, not the responsiveness of clicking a button etc.

comment:11 Changed 6 years ago by sunxcint

If performance and "tr" issue you mentioned are no longer issues, I suggests applying the alternative patch. After all it is not a best practice to let a class(prototype here) to log the information about the instances information it created.

comment:12 Changed 6 years ago by bill

Out of curiosity, what make you think that storing shared info in the prototype is a bad idea?

comment:13 Changed 6 years ago by sunxcint

"Prototypes having data properties is generally considered an anti-pattern". This is even reflected in classes definition of ECMAScript 6. And this ticket itself also indicates that "mutable prototype properties are difficult to manage".

Several articles talked about this topic, e.g.

http://www.2ality.com/2013/09/data-in-prototypes.html http://css.dzone.com/articles/ecmascriptnext-classes

comment:14 Changed 6 years ago by bill

OK, well dijit certainly isn't going to stop having "data properties in the prototype", because it's much better for performance than reinitializing all the properties in every instance. Also note that those articles are talking about default values for properties, which is unrelated to what RadioMenuItem? is doing.

This is really about the pros/cons of caching data vs. computing it on the fly.

comment:15 Changed 6 years ago by sunxcint

I might have not made it so clear in my last comment.

Actually "data properties in the prototype" is not forbidden strictly. To avoid unnecessary reinitializing, "it generally makes sense to use them to define only properties that are the same for all objects within the class" and "properties with constant values(such as mathematical constants) are also suitable with definition with prototype properties"(Javascript, The Definitive Guide).

"_currentlyChecked" is not a constant value. Although all objects of RadioMenuItem? share the same value and only one prototype object will be kept in memory for all the instances, it might contain information that is unnecessary for one certain object(i.e. the checked value for other groups) and so mutable and difficult to manage.

comment:16 Changed 6 years ago by sunxcint

Since I am relatively new to Dojo, please feel free to correct me which will benefit me to contribute more high quality code back to Dojo. Thanks.

comment:17 Changed 6 years ago by bill

Oh I understand what the article is saying, I just don't agree with it.

Anyway, as far as finding the checked item on the fly rather than caching the value, I don't really care, but I'll check in a modified version of your code since (as you said) it matches RadioButton?. A few (IMO) improvements I will make are:

  • array.forEach() (like Array.forEach in newer browsers) takes a "this" parameter so you don't need lang.hitch()
  • I can use _setGroupAttr: "domNode" to map the widget's group property to an attribute on the DOMNode, so I don't need a postCreate() method call domAttr.set().
  • _setCheckedAttr() shouldn't search for other checked RadioMenuItem? widgets if group is not set

comment:18 Changed 6 years ago by Bill Keese <bill@…>

In 7e98f10fd7b013a89929973df6aa293921de3af7/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:19 Changed 6 years ago by sunxcint

I like your method to use _setGroupAttr: "domNode". Thanks for your improvement.

Note: See TracTickets for help on using tickets.