Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#8358 closed defect (fixed)

[patch][ccla]selectors without spaces incorrectly parsed by dojo.query()

Reported by: alex Owned by: alex
Priority: high Milestone: 1.4
Component: Query Version: 1.2.3
Keywords: query, tokenizer, parsing Cc: James Burke
Blocked By: Blocking:

Description

dojo.query() doesn't currently handle queries like:

dojo.query(".foo>.bar");

Instead, it currently requires spaces around both sides of the ">" char. This is in conflict with the CSS 3 selectors spec which does allow tight packing around the infix operators ">", "~", and "+".

Attachments (2)

8358.patch (4.8 KB) - added by Douglas Hays 10 years ago.
possible patch to be reviewed
8358-james.patch (3.4 KB) - added by James Burke 10 years ago.
Another type of patch, normalizing the string before looking for caches and deciding if qsa is safe

Download all attachments as: .zip

Change History (11)

comment:1 Changed 10 years ago by Douglas Hays

Summary: selectors without spaces incorrectly parsed by dojo.query()[patch][ccla]selectors without spaces incorrectly parsed by dojo.query()

Changed 10 years ago by Douglas Hays

Attachment: 8358.patch added

possible patch to be reviewed

comment:2 Changed 10 years ago by bill

Milestone: 1.31.4

1.3rc1 has been release; bumping remaining tickets to 1.4 (except for documentation/testing tickets)

comment:3 Changed 10 years ago by bill

Component: CoreQuery

comment:4 Changed 10 years ago by dante

Priority: normalhigh

@alex, could you find a moment to review this? been sitting for 8 months w/ ccla/patch

comment:5 Changed 10 years ago by dante

@jburke, actually looking at this more, I think it fixes your issue with dojox.jq and spaces around >'s ... worth applying and testing.

comment:6 Changed 10 years ago by Les

See also #10066. This is a reversed example. The query returns results if the spaces around > are removed, but it fails when the spaces are added.

Changed 10 years ago by James Burke

Attachment: 8358-james.patch added

Another type of patch, normalizing the string before looking for caches and deciding if qsa is safe

comment:7 Changed 10 years ago by James Burke

I have concerns about the original 8358.patch. It modifies the character state machine area, but the decision to use QSA is made before that point, and seeing a space in the selector is used to branch to non-QSA, so I think it is better to normalize the string before the useQSA check, and also allows for catching a cached query if we normalize on spaces around the infix operators.

I just uploaded a different patch that does this. Unfortunately a regexp is involved for normalization.

In the patch I commented out an if() test that did indexOf tests for the infix operators, and to skip the string replace/regexp call in that case. In my testing using the normal slickspeed tests, I cannot see much of a difference between query before this patch or between the patch without that if() and with the if().

To get a better idea on the performance aspect though it would be good to get the ops/sec test system that Alex used. For many of the browsers, the slickspeed tests now register as running in 0 ms, so it is hard to get a read on the real cost.

comment:8 Changed 10 years ago by James Burke

Resolution: fixed
Status: newclosed

(In [20851]) Fixes #8358, some infix operators do not require spaces around them. Tested with Alex's queries/mss tool and normal slickspeed and do not see appreciable differences with this change. \!strict

comment:9 Changed 10 years ago by James Burke

(In [20859]) Refs #8358, patch to my patch from doughayes that is more compact and prettier. Tests still pass, and no noticeable drag on perf over previous patch. \!strict

Note: See TracTickets for help on using tickets.