639638 - Lookup expressions should allow you to use '/[0]/' to select from a list of nodes

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Andrew Halberstadt [:ahal]]

Currently when you use a non-unique attribute (for example "{flex:1}") you run the risk of more than one node matching your query.  Currently the lookup parser will return a list of all nodes that match that query.

You can also use the term "/[0]/" to select the first child element of the previous node.  For example: '/id("foo")/[0]' will select the first child of element 'foo'.

The problem is that there is no way (that I know of) to keep going if the lookup parser returns a list.  For example: '/{"flex":1}/[0]' will be an error if the {"flex":1} portion of the lookup string returns a list (since list.childNodes is obviously not a valid property).

Ideally when you use '/[0]/' in a lookup expression, it should be smart enough to know whether it is dealing with a single element (in which case it will select from childNodes) or a list of elements (in which case it will select from the list).

In our current implementation I don't think there is any way to get past a list of nodes.

Comment 1

[Andrew Halberstadt [:ahal]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=638876 for how a use case where this needed a work-around.

(Henrik had to use '/id("foo")/[0]' instead of '/id("foo")/{"flex":1}/[0]' as the latter doesn't work)

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - Simple fix

Before taking this for 2.0 I think we should decide what exactly we want to happen in this scenario.  Taking this patch would mean that the same lookup string could return different elements depending on the children of the parent node.

I would argue that there isn't anything that this will break that wouldn't break before and that we gain additional flexibility in our lookup strings.

Comment 3

[cmtalbert]

We should test this on calendar which is the XBL use case from hell.  IF it works there then we should take it.

Comment 4

[Andrew Halberstadt [:ahal]]

Clint, are there any tests already written for calendar?

Comment 5

[Merike Sell (:merike)]

Calendar tests live in comm-central/calendar/test/mozmill.

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 517576 [details] [diff] [review]
Patch 1.0 - Simple fix

I really don't think there is anyway this can break anything, so I'm going to flag for review.

My reasoning is as follows: if you look at the patch, there is only different behaviour when we pass an Array object into the function. Before this patch, we would have called Array.childNodes which would have thrown an exception anyway.

Comment 7

[Heather Arthur [:harth]]

(In reply to comment #0)
> Ideally when you use '/[0]/' in a lookup expression, it should be smart enough
> to know whether it is dealing with a single element (in which case it will
> select from childNodes) or a list of elements (in which case it will select
> from the list).

I don't think it being smart is a good idea, the syntax for lookup should be deterministic like CSS selectors or XPath. What about '/{"flex": 1}[0]' to get the nth element that matches?

Comment 8

[Andrew Halberstadt [:ahal]]

Yeah, that's why I wasn't sure if we should take it for 2.0 (as mentioned in comment 2). I agree that what you suggest is the best solution, but it would involve some significant changes to the lookup parsing code. This introduces a pretty big risk of regression for a pretty small win.

I'd be inclined to either take that patch or not take anything at all.. but that's just me.

Comment 9

[cmtalbert]

Ok, we have an impasse here.  Heather, what is your opinion on what to do for 2.0?

Comment 10

[cmtalbert]

We'll take this for now, but please file a follow on bug to fix the awfulness of lookups kthxbye

Comment 11

[Heather Arthur [:harth]]

Comment on attachment 517576 [details] [diff] [review]
Patch 1.0 - Simple fix

As discussed in mozmill meeting, we'll take this quick fix for now and make it better later.

Comment 12

[Andrew Halberstadt [:ahal]]

Filed a follow up for Harth's suggestion: bug 649731

master: https://github.com/mozautomation/mozmill/commit/c7b4c9e2f5072efe0f590a65c73154f0e526746f