865008 - Search by "ID" never finds any results

Status: VERIFIED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Michael Kraft [:morac]]

Starting with Firefox 23.0a1 (trunk build), if I open DOM Inspector and search by ID, it never finds said ID.  If I search by attribute and specify "id" as the attribute, then it works.  I'm not sure which load this broke under, but it works in Aurora, but not Nightly loads.

Steps to reproduce:
1. Open DOM Inspector and inspect the current window's (content or chrome) document.
2. Search by id for and id that's in the window (without quotes).

Expected results:
1. Should find the specified window

Actual Results: 
1. Reports "end of document reached" without finding id.

Work around:
1. Search by "attr" and specify "id" (no quotes) as the attribute.

Comment 1

[Colby Russell :crussell (no longer Mozilla-ing)]

(From http://hg.mozilla.org/dom-inspector/file/a97bec13e961/resources/content/viewers/dom/dom.js#l938)
>     for (var i = 0; i < node.attributes.length; i++) {
>       var attr = node.attributes[i];
>       if (attr.isId && re.test(attr.nodeValue)) {
>         return true;
>       }
>     }

The isId check is failing.  This might have broken due to some changes for the attributes-shouldn't-be-nodes-anymore stuff (bug id?).  The method is still intact, though, so dunno what's up with that: http://hg.mozilla.org/mozilla-central/file/40dafc376794/content/base/src/Attr.cpp#l287

In any case this method should just be using getElementById instead.

Comment 2

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to Colby Russell :crussell from comment #1)
> In any case this method should just be using getElementById instead.

Actually, since the find dialog allows regular expressions, getElementById won't work.

Comment 3

[Boris Zbarsky [:bzbarsky]]

dom/webidl/Attr.webidl does not have an isId attribute.

Neither does the spec at http://dom.spec.whatwg.org/#attr

So presumably a regression from bug 851470.

We could put the property back for now, but long-term it's going away unless the spec is changed to add it.  Which is unlikely, since the current plan is that the attribute named "id" is an ID on all elements and all other attributes are not.

In the case of the inspector code, I think you should just test for the attr name being "id"....

Comment 4

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: check that attribute name is "id"

Comment 5

[[e-mail]]

Can we not just use node.id instead of searching for an attribute named id?

Comment 6

[[e-mail]]

Oh, backward compatibility, so how about node.has/getAttribute("id")? (Can't use getAttribute directly because it returns null for some nodes.)

Comment 7

[[e-mail]]

Actually, node.id would probably work in older versions too if you added a typeof node.id == "string" or other similar check.

Comment 8

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to [e-mail] from comment #6)
> Oh, backward compatibility, so how about node.has/getAttribute("id")? (Can't
> use getAttribute directly because it returns null for some nodes.)

Which nodes are those?  Non-element nodes?  We're already checking that the node is an element:
(From http://hg.mozilla.org/dom-inspector/file/a97bec13e961/resources/content/viewers/dom/dom.js#l932)
>     if (node.nodeType != Components.interfaces.nsIDOMNode.ELEMENT_NODE) {
>       return false;
>     }

Even if getAttribute does return null instead of a string for some element nodes, that's okay, right?  Surely nodes which return null on getAttribute calls aren't the element we're looking for?  Or is it still possible to get null from a getAttribute call on a node that a) is an element, and b) actually has an id?

Comment 9

[Colby Russell :crussell (no longer Mozilla-ing)]

Ah, nevermind.  I just read the getAttribute spec. <http://dom.spec.whatwg.org/#dom-element-getattribute>  (I was assuming it would return an empty string if the attribute isn't present.)

Comment 10

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: use getAttribute to check that the id matches

(In reply to [e-mail] from comment #7)
> Actually, node.id would probably work in older versions too if you added a
> typeof node.id == "string" or other similar check.

No need to even branch since getAttribute will work for both, no?

Comment 11

[[e-mail]]

Comment on attachment 743981 [details] [diff] [review]
use getAttribute to check that the id matches

>+    // NB: In HTML getAttribute can return null, so we have to check that it's
>+    // actually set; if we don't and the search string is "null", implicit
>+    // toString conversion means that we'll match on every element without an
Indeed, search by attribute also has this bug...

Comment 12

[Colby Russell :crussell (no longer Mozilla-ing)]

([e-mail] wrote in comment #11)
> Indeed, search by attribute also has this bug...

Dear meandering bugzilla reader, that's bug 867540.

Comment 13

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: use hasAttribute to check that the attribute exists

On second thought, I want to use hasAttribute to verify "id" exists.  That get's rid of the id-value-can't-be-empty-string-caveat I commented about, and I don't know why I didn't use it the first time.

Comment 14

[[e-mail]]

(In reply to Colby Russell from comment #12)
> (In reply to comment #11)
> > Indeed, search by attribute also has this bug...
> 
> Dear meandering bugzilla reader, that's bug 867540.

Well, it is now, but I hadn't filed it back then...

Comment 15

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to [e-mail] from comment #14) 
> Well, it is now, but I hadn't filed it back then...

That comment's for the benefit meandering bugzilla reader who comes across this bug in the future.  Good cross referencing makes bug archaelogy suck less. :)

Comment 16

[[e-mail]]

Comment on attachment 744221 [details] [diff] [review]
use hasAttribute to check that the attribute exists

>+    // ID.  Additionally, for elements without an ID, getAttribute returns an
>+    // empty string in XUL (bug 232598), so our check must handle that...
Is this just trying to say that you can't compare getAttribute to null because that won't handle the XUL case? If so, r=me.

Comment 17

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to [e-mail] from comment #16)
> Is this just trying to say that you can't compare getAttribute to null
> because that won't handle the XUL case? If so, r=me.

Yup.

Pushed: http://hg.mozilla.org/dom-inspector/rev/561155ad5353

Comment 19

[Onno Ekker [:nONoNonO UTC+1]]

Any chance of releasing 2.0.15 any day soon?

Comment 21

[Onno Ekker [:nONoNonO UTC+1]]

If it's too much trouble releasing 2.0.15, maybe it's possible to make a release on amo's development channel or an alpha/beta/rc on dom i's website? It's really hard to do debugging when you can't search on id.

Comment 22

[Tony Mechelynck [:tonymec]]

(In reply to Onno Ekker [:nONoNonO UTC+1] from comment #21)
> If it's too much trouble releasing 2.0.15, maybe it's possible to make a
> release on amo's development channel or an alpha/beta/rc on dom i's website?
> It's really hard to do debugging when you can't search on id.

The bug about releasing DOMi 2.0.15 to AMO is bug 957149. In the meantime, I'll unofficially attach to this bug the 2.0.15pre found as part of the latest SeaMonkey nightly (2014-02-14). No guarantee about its usability for any purposes including merchantability.

Comment 23

[Tony Mechelynck [:tonymec]]

Attachment: DOMi 2.0.15pre as of 2014-02-14

Comment 24

[Tony Mechelynck [:tonymec]]

Comment on attachment 8379295 [details]
DOMi 2.0.15pre as of 2014-02-14

Oops! A typo!

Comment 25

[Onno Ekker [:nONoNonO UTC+1]]

Thank you Tony! Searching works again with this version. I haven't yet tested the rest, but i suppose it will also be alright if this version is shipped with SeaMonkey...

Comment 26

[Tony Mechelynck [:tonymec]]

(In reply to Onno Ekker [:nONoNonO UTC+1] from comment #25)
> Thank you Tony! Searching works again with this version. I haven't yet
> tested the rest, but i suppose it will also be alright if this version is
> shipped with SeaMonkey...

My pleasure. It was shipped with a _trunk nightly_ build of SeaMonkey, which means it was the tip of the default branch of the DOMi repository when the source for that nightly was checked out. It won't be shipped with release or beta builds of SeaMonkey until an appropriate tag is pushed to the DOMi repo, which probably means a DOMi release. I'm not sure about how Aurora builds of SeaMonkey get their DOMi source.

For Firefox and Thunderbird, of course, DOMi is not shipped as part of the application, which means that their users will have to either wait for an AMO release (see bug 957149) or avail themselves of the attached bootleg copy.

I'm setting VERIFIED according to comment #25. If someone sees the bug after installing the attached XPI (and restarting), please describe your symptoms in a comment, in as much detail as you can.