Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#13815 closed defect (fixed)

[patch][cla] dijit.form.HorizontalRuleLabels throws error on creation without srcNodeRef

Reported by: Kenneth G. Franqueiro Owned by: Kenneth G. Franqueiro
Priority: high Milestone: 1.8
Component: Dijit - Form Version: 1.6.1
Keywords: Cc: Douglas Hays
Blocked By: Blocking:

Description

dijit.form.HorizontalRuleLabels contains two lines of code which assume that srcNodeRef exists; this causes errors to be thrown when it doesn't. (dijit.form.VerticalRuleLabels is also affected since it inherits the offending code.)

I'm attaching a patch which fixes the two lines of code (one of which is completely pointless in a templated widget), as well as augments the test_Sliders page to demonstrate the issue. I'll attach the html standalone as well.

(FWIW, the code involved in this problem actually dates back at least to 1.3, if not farther, but it still applies today. I'm not marking it 1.7.0b1 because it should not raise any alarm against the 1.7 release; the fix should probably be committed post-1.7.)

Attachments (2)

test_Slider.html (10.0 KB) - added by Kenneth G. Franqueiro 8 years ago.
updated test page to reproduce issue (check console)
13815.diff (4.0 KB) - added by Kenneth G. Franqueiro 8 years ago.
Updated patch with test worked into robot/Slider_mouse.html

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by Kenneth G. Franqueiro

Attachment: test_Slider.html added

updated test page to reproduce issue (check console)

comment:1 Changed 8 years ago by bill

Cc: Douglas Hays added
Component: DijitDijit - Form

I guess you meant for Doug to look at this [and check it in].

Would be better to put the programmatic creation somewhere inside a DOH test.

comment:2 Changed 8 years ago by Kenneth G. Franqueiro

I didn't expect this to get checked in before 1.7, but if you think it can be, I'm fine checking it in as long as someone +1s it. Or Doug could do the honors if he wants.

I wasn't sure if we even had any DOH (non-robot, at least) tests for sliders...

comment:3 Changed 8 years ago by bill

I guess we don't have any non-robot tests for Slider, perhaps we should. See for example the Select test file, which runs automated tests when passed a flag: tests/form/test_Select.html?mode=test

Alternately sometimes we piggyback plain (non-robot) tests into the robot test files. In this case just making sure that the widget was properly created.

comment:4 Changed 8 years ago by Kenneth G. Franqueiro

It looks like there's already a single sanity test before the robot tests really kick in on dijit/tests/form/robot/Slider_mouse.html, and that already uses test_Slider.html which I patched above, as a base. Maybe I can just add another sanity test to that.

Changed 8 years ago by Kenneth G. Franqueiro

Attachment: 13815.diff added

Updated patch with test worked into robot/Slider_mouse.html

comment:5 Changed 8 years ago by Douglas Hays

bill, can you review/commit this patch? It is changing the code added in [9692] that you committed and you may better understand srcNodeRef's role there and why you added the innerHTML= that this patch is removing.

comment:6 Changed 8 years ago by bill

@kgf - I guess you are right that the

this.srcNodeRef.innerHTML = '';

is unneeded. Like Doug said I added that in [9692] but I can't see a reason now.

So that patch looks good for me, go for it.

comment:7 Changed 8 years ago by Kenneth G. Franqueiro

Resolution: fixed
Status: newclosed

In [26948]:

Allow RuleLabels? to be created without srcNodeRef; fixes #13815

comment:8 Changed 8 years ago by bill

Milestone: tbd1.8
Note: See TracTickets for help on using tickets.