Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#9772 closed defect (fixed)

ContentPane's checkIfSingleChild is thrown off by <script> elements

Reported by: Sam Foster Owned by: bill
Priority: high Milestone: 1.4
Component: Dijit Version: 1.3.2
Keywords: Cc:
Blocked By: Blocking:

Description

If you have a <script> element alongside your single widget in the containerNode, _singleChild ends up not being set.

This is naively fixed by just adding a filter to the childNodes..

var childNodes = dojo.query(">", this.containerNode).filter(function(node) {
  return node.tagName !== "SCRIPT"; // or a regexp for hidden elements like script|area|map|etc..
})

.. however its conceivable that you could have a script element with a dojotype that ended up getting rendered. I'm not sure there is a practical solution that covers every possibility, but there should be some accomodation for non-rendered elements here, as this drives ContentPane?'s resize behavior

Change History (12)

comment:1 Changed 10 years ago by bill

Sure, feel free to make the filtering more robust. Like you said it may not be possible to make it perfect but we could definitely make it better. Should add some unit tests too.

comment:2 Changed 10 years ago by Les

This also should work:

var childNodes = dojo.query(">:not(script)", this.containerNode)

comment:3 Changed 10 years ago by bill

Milestone: tbd1.4
Owner: set to bill
Status: newassigned

comment:4 Changed 10 years ago by bill

Hmm, maybe dojo.query(">:not(script)", this.containerNode) should work, but it doesn't, according to my tests. Not sure why. I will check in the first solution with the filter.

comment:5 Changed 10 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [20372]) Add code to ignore script tags when determining if a ContentPane has a single child or not, fixes #9772 !strict.

comment:6 in reply to:  4 Changed 10 years ago by Les

Replying to bill:

Hmm, maybe dojo.query(">:not(script)", this.containerNode) should work, but it doesn't, according to my tests. Not sure why.

It should work. If it doesn't work, it's a bug in dojo.query and I'd suggest creating a ticket.

comment:7 Changed 10 years ago by Les

Resolution: fixed
Status: closedreopened

I suggest changing the query below to dojo.query(">*", ...

dojo.query accepts the current form, but Sizzle for example doesn't. I believe you can use Sizzle with Dojo(?)

Also, document.querySelectorAll("form>") will throw an exception "An invalid or illegal string was specified", but document.querySelectorAll("form>*") works fine.

_checkIfSingleChild: function(){
	...
	var childNodes = dojo.query(">", ...

Also, would this work?

dojo.query(">*:not(script)", this.containerNode)

comment:8 Changed 10 years ago by bill

Resolution: fixed
Status: reopenedclosed

OK, but please don't reopen a (bug) ticket unless something is actually broken.

dojo.query accepts the current form, but Sizzle for example doesn't. I believe you can use Sizzle with Dojo(?)

No not really, there some experimental code for it but it's not fully working. See #8547. But I'll change from ">" to "> *".

Also, document.querySelectorAll("form>") will throw an exception "An invalid or illegal string was specified", but document.querySelectorAll("form>*") works fine.

I'm not sure that's a valid argument as neither document.querySelectorAll(">") nor document.querySelectorAll("> *") works.

Also, would this work? dojo.query(">*:not(script)", this.containerNode)

As I mentioned above, the not() syntax isn't working for me. You should try it and start writing tickets with test cases (ideally extensions of the automated dojo.query() tests) against dojo.query().

comment:9 Changed 10 years ago by bill

(In [20383]) For dojo.query() call, use "> *" rather than ">" so it works on sizzle. Refs #9772 !strict.

comment:10 Changed 10 years ago by Les

I tried ":not(script)" and it works for me. How come you din't create a ticket for it if it doesn't work? It's a bug, and a major one.

Also, why there always white space around > in dijit queries? The white space is optional according to W3C.

comment:11 Changed 10 years ago by bill

It's mysterious how :not(script) worked for you, all I did was try the code you wrote above. Are you running against trunk? Did you run the unit tests, specifically ContentPaneLayout.html nestedStack.html, and ContetnPane.html?

Anyway, I filed #10066 to track the problem.

comment:12 in reply to:  11 Changed 10 years ago by Les

Replying to bill:

It's mysterious how :not(script) worked for you, all I did was try the code you wrote above.

I prefer to use the compact query notation, so this might explain the mystery. See #10066

Note: See TracTickets for help on using tickets.