Opened 8 years ago

Closed 7 years ago

#14828 closed defect (fixed)

dijit.form.TextBox _onInput needs better checking for destroyed state

Reported by: Karl Tiedt Owned by: Douglas Hays
Priority: undecided Milestone: 1.8
Component: Dijit - Form Version: 1.7.2
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

If you have a TextBox that is in a form that destroys itself onCancel, the TextBox will error if it was focused when ESC is hit (or the dialog is destroyed)

This happens in ValidationTextBox as well as regular TextBox

You can see a demo of this here: http://ktiedt.dojotoolkit.org/dojotoolkit/dijit/tests/test_Dialog.html

The first button opens a dialog that triggers the problem once you hit esc (any of the textbox's should demo the error)

There are 2 possible fixes that I can think of, depending on what is considered "best" for dijit

1) make TextBox widgets ignore ESC for _onInput
2) check for _beingDestroyed before entering _onInputs main code

Attachments (1)

14828.patch (35.4 KB) - added by Douglas Hays 7 years ago.
refreshed fix

Download all attachments as: .zip

Change History (14)

comment:1 Changed 8 years ago by Karl Tiedt

Actually due to potential setTimeout paths... the _beingDestroyed approach would need to be done in multiple locations: potentially, _FormValueMixin _TextBoxMin and TextBox?

comment:2 Changed 8 years ago by bill

Description: modified (diff)

You could check for this._destroyed instead of this._beingDestroyed; either one would work but the former seems more standard.

Alternately, keep track of all the running setTimeout() calls and cancel them in destroy().

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

comment:3 Changed 8 years ago by Douglas Hays

How about a widget version of setTimeout that just checks _destroyed before running the given function? Then there's no bookkeeping nor queues to maintain.

comment:4 Changed 8 years ago by Douglas Hays

Milestone: tbd1.8
Status: newassigned

comment:5 Changed 8 years ago by bill

How about a widget version of setTimeout that just checks _destroyed before running the given function?

Seems like a good idea.

It could alternately maintain a queue of in-progress setTimeout() calls, canceling them on widget destroy. That would be a bit more work, but would ensure that everything was gone by the time destroy() finished executing.

I count a scary 33 uses of setTimeout() in dijit, not just limited to dijit/form, so seems like it would be a good global change.

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

comment:6 Changed 7 years ago by Douglas Hays

bill, this is a big change with a lot of hard to test code paths. Can you review the latest patch please?

Changed 7 years ago by Douglas Hays

Attachment: 14828.patch added

refreshed fix

comment:7 Changed 7 years ago by Douglas Hays

Patch update to rollback typematic changes since _WidgetBase is not always available.

comment:8 Changed 7 years ago by bill

The new patch is looking better. I got failures in the Editor tests though (FullScreen, ViewSource, LinkDialog).

comment:9 Changed 7 years ago by Douglas Hays

Resolution: fixed
Status: assignedclosed

In [27988]:

Fixes #14828. Add defer method to _WidgetBase and change setTimeout to use defer(...) instead, and clearTimeout to use handle.remove() instead. The defer method will check if the widget has been destroyed before executing the supplied function. !strict

comment:10 Changed 7 years ago by bill

Resolution: fixed
Status: closedreopened

As I mentioned above, this is causing failures in the Editor tests.

comment:11 Changed 7 years ago by Douglas Hays

Those tests are passing for me. Please append more detailed failure information. The editor plugin changes were never committed so there shouldn't be any failures.

comment:12 Changed 7 years ago by bill

In LinkDialog on IE8, I get two failures about the doubleclick test (one for the LinkDialog and one for the ImgDialog). It's working in [27987] and fails in [27988]. Happens stand-alone or from runTests.html.

    _AssertFailure: assertTrue('0') failed with hint:  		tooltip dialog still visible
     ERROR IN: 		 function(){
							var d = new doh.Deferred();

							// Focus on the editor window
							editor.focus();
							doh.robot.mouseMoveAt(editor.iframe, 1000, 1);
							doh.robot.mouseClick({left:true}, 500);

							doh.robot.sequence(d.getTestErrback(function(){
								doh.t(ldPlugin && ldPlugin.dropDown && ldPlugin.dropDown.domNode, "found TooltipDialog");
								doh.t(isHidden(ldPlugin.dropDown.domNode),  "tooltip dialog is hidden");
							}));

							// Double click
							doh.robot.mouseMoveAt(node, 500, 1);
							doh.robot.mouseClick({left:true}, 100);
							doh.robot.mouseClick({left:true}, 100);

							var f;
							doh.robot.sequence(d.getTestErrback(function(){
								doh.t(isVisible(ldPlugin.dropDown.domNode),  "tooltip dialog is visible");

								f = dijit.getFocus();
								doh.t(f.node, "got focus");

								var w = dijit.getEnclosingWidget(f.node);
								doh.t(w, "found enclosing widget");

								var val = w.get("value");
								doh.t(new RegExp(".*/sample.jpg").test(val), "Verifying the contents contained image name");
							}), 2000);

							// Clicking the <input> shouldn't close TooltipDialog (#14395)
							doh.robot.mouseMoveAt(function(){ return f.node; }, 500, 1);
							doh.robot.mouseClick({left:true}, 100);
							doh.robot.sequence(d.getTestCallback(function(){
								doh.t(isVisible(ldPlugin.dropDown.domNode),  "tooltip dialog still visible");
							}), 1000);

							return d;
						} FAILED test: ../../dijit/tests/editor/robot/Editor_LinkDialog.html::ImgDialog_tests::Image Tag: Double-Click opens TooltipDialog. 5844 ms

I'm also getting other spurious failures but perhaps they are unrelated.

If that LinkDialog test failure doesn't reproduce for you I'll trace it on this end.

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

comment:13 Changed 7 years ago by Douglas Hays

Resolution: fixed
Status: reopenedclosed

In [28028]:

Fixes #14828. Update editor._updateTimer references in LinkDialog? plugin to reference remove() method instead. Optimized a couple of invocations to defer(...) in RichText?. !strict

Note: See TracTickets for help on using tickets.