Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#18321 closed defect (fixed)

Presence of a prolog node breaks Query results

Reported by: Claude Guyomard Owned by: dylan
Priority: undecided Milestone: 1.7.9
Component: Query Version: 1.9.5
Keywords: Cc:
Blocked By: Blocking:

Description

Different but similar to https://bugzilla.mozilla.org/show_bug.cgi?id=901632#c5

The reason why I specified 1.9.5 as affected is that the code is so similar to 1.5. Version 1.5 is the version where I actually observed this issue that affects the test 'childNodes.length == childWidgetNodes.length' present in dijit.layout.ContentPane?._checkIfSingleChild()


In dojo/_selector/acme, implementation of '_simpleNodeTest' is determined like :

var _noNES = (typeof getDoc().firstChild.nextElementSibling == "undefined"); ... var _simpleNodeTest = (_noNES ? _isElement : yesman);

with

var yesman = function(){ return true; }; var _isElement = function(n){ return (1 == n.nodeType); };

Issue : When an XML prolog is present on top of the page, the value of 'getDoc().firstChild.nextElementSibling' is inconsistent between browsers :

FF33

  • getDoc().firstChild => Comment { data="?xml version="1.0" encoding="UTF-8"?", length=36, nextElementSibling=html, more...}
  • getDoc().firstChild.nextElementSibling => HtmlElement?
  • typeof getDoc().firstChild.nextElementSibling == "undefined" => false
  • _noNES => false
  • _simpleNodeTest => yesman;

IE10

  • getDoc().firstChild => DocType?
  • getDoc().firstChild.nextElementSibling => undefined
  • typeof getDoc().firstChild.nextElementSibling == "undefined" => true
  • _noNES = true
  • _simpleNodeTest => _isElement;

Hope this will help.

Change History (15)

comment:1 Changed 5 years ago by Claude Guyomard

Clarifications :

  • the problem happens when _childElements() calls _simpleNodeTest(textNode) because the result is true even for textNodes.

I want to correct myself about 1.9 : I could not reproduce the problem with 1.9. In _childElements(), "tret = root.children || root.childNodes" results in "tret == root.children" which contains elements only.

Last edited 4 years ago by dylan (previous) (diff)

comment:2 Changed 4 years ago by dylan

Milestone: tbd1.11
Owner: set to Claude Guyomard
Status: newpending

So if I'm understanding correctly, this is not an issue with modern Dojo, just very old Dojo (e.g. 1.5?).

comment:3 Changed 4 years ago by Claude Guyomard

Status: pendingnew

Hi Dylan,

I observed this issue a longtime ago. There are 2 aspects to see :

The particular use case I pointed

It disappeared in Dojo-1.9.

While dijit.layout.ContentPane?._checkIfSingleChild() might fail in Dojo-1.5 when (FF and XML prolog), the Dojo-1.9 implementation now relies on different (or complementary*) criteria to establish whether it is "single-child" or not.

  • In particular the FILTERING_FUNC placed in DOJO-1.9;_ContentPaneResizeMixin._checkIfSingleChild():

query("> *", this.containerNode).some( FILTERING_FUNC );

The error, that can potentially cause issues distinct than the past _checkIfSingleChild issue

It is still there !

In dojo-1.5, a reason why the previous test failed was that dojo/_selector/acme bound "_simpleNodeTest" to 'yesman' function, an ALWAYS-TRUE predicate instead of the '_isElement' function, the effectively node-type test function.

While the (DOJO-1.9; _ContentPaneResizeMixin._checkIfSingleChild()) results now rely lesser onto _simpleNodeTest(), the same erroneous binding procedure of "_simpleNodeTest" still exists in (DOJO-1.9; dojo/selector/acme.js)

Search for '_simpleNodeTest' in :

Hope that this will help you.

Cordialement,

comment:4 Changed 4 years ago by dylan

Owner: changed from Claude Guyomard to dylan
Status: newassigned

Thanks, I'll see if I can fix this for 1.11, and if not, then 1.12.

comment:5 Changed 4 years ago by dylan

Ok, so I think I get what is going on:

https://github.com/dojo/dojo/blob/master/selector/acme.js#L501

This used to return true for Firefox and Chrome. But due to a change in behavior of the spec ( see https://bugzilla.mozilla.org/show_bug.cgi?id=901632#c5 ), this condition now returns false instead of true when a DocumentType? is provided.

comment:6 Changed 4 years ago by dylan

So I think that _noNES needs to change to something like this, but I think this would also capture HTML5 documents which is wrong:

var _noNES = (typeof getDoc().firstChild.nextElementSibling == "undefined") &&
		(typeof getDoc().firstChild.nodeType != 10);

Any thoughts on what the secondary condition should be in this case?

comment:7 Changed 4 years ago by Claude Guyomard

Hi Dylan,

_noNES semantic

I have a problem with the semantic of _noNES.

What does _noNES detect or capture exactly ?

1- The browser support of the "nextSiblingElement" property on nodes. See https://developer.mozilla.org/en/docs/Web/API/NonDocumentTypeChildNode/nextElementSibling

In this case, my question is : Isn't typeof getDoc().firstChild.nextElementSibling == "undefined" a non-robust implementation of "nextElementSibling" in getDoc().documentElement ?

This kind of issue seems to be well known :

From https://developer.mozilla.org/en/docs/Web/API/Document/documentElement#Notes

"HTML documents typically contain a single child node, <html>, perhaps with a DOCTYPE declaration before it. XML documents often contain multiple child nodes: the root element, the DOCTYPE declaration, and processing instructions.

That's why you should use document.documentElement rather than document.firstChild to get the root element."

2- the identification of specific Firefox versions (25, 26 or 27)

Possible justifications : "[2] Firefox 25 also added the previousElementSibling and nextElementSibling properties, this was removed in Firefox 28 due to compatibility problems."

Found in https://developer.mozilla.org/en-US/docs/Web/API/DocumentType#Browser_Compatibility

3- something else...

comment:8 Changed 4 years ago by dylan

I prefer your option 1 as well Claude.

https://github.com/dojo/dojo/pull/195

I think this fix makes #2 not matter, as it avoids us ever checking for this property on the DocumentType? anyway.

I'm going to land this as it passes all of our tests. Will backport it as well.

comment:10 Changed 4 years ago by Claude Guyomard

Dylan,

comment 1

typeof getDoc().firstChild.nextElementSibling == "undefined" should not be replaced by :

"nextElementSibling" in getDoc().documentElement

but by

!("nextElementSibling" in getDoc().documentElement)

Because _noNES is for no NextElementSibling.

comment 2

Because querying a property name on an object is said to be slow (may be I am wrong), another option could be to use it as a confirmation like this:

var htmlElm = getDoc().documentElement;
var _noNES = !(htmlElm.nextElementSibling || "nextElementSibling" in htmlElm);

Because all modern browsers will evaluate htmlElm.nextElementSibling as truthy, they will never query "nextElementSibling" in htmlElm. On the contrary IE8 will compute both left and right expressions.

comment:11 Changed 4 years ago by dylan

Resolution: fixed
Status: closedreopened

Sigh... somehow I deleted a character before I committed the change.

comment:12 Changed 4 years ago by dylan

Resolution: fixed
Status: reopenedclosed

comment:13 Changed 4 years ago by dylan

Ok, clearly I'm having a bad day. Fixed for real now (hopefully):

And backported through to 1.7.

comment:14 Changed 4 years ago by bill

To confirm, there's nothing wrong with ContentPane? per se, right? The code in question seems to be

query("> *", this.containerNode).some(function(node){
        var widget = registry.byNode(node);
        if(widget && widget.resize){
                candidateWidgets.push(widget);
        }else if(!/script|link|style/i.test(node.nodeName) && node.offsetHeight){
                otherVisibleNodes = true;
        }
});

I know that the some() call should really be forEach(), or alternately the function should return true in certain cases, but I think that's a harmless error, except for a minimal performance impact.

comment:15 Changed 4 years ago by dylan

Bill, yes, your assessment of the ContentPane? fix from a while back is correct. That fixed the more common error. My recent fix was primarily for code where an xhtml or xml DocumentType? is specified.

Note: See TracTickets for help on using tickets.