Closed Bug 865008 Opened 7 years ago Closed 7 years ago

Search by "ID" never finds any results

Categories

(Other Applications :: DOM Inspector, defect)

x86
Windows XP
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: morac, Assigned: crussell)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
(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.
(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.
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"....
Blocks: 851470
Attachment #743621 - Attachment description: check that attribute name is → check that attribute name is "id"
Assignee: nobody → Sevenspade
Blocks: DOMi2.0.16
Can we not just use node.id instead of searching for an attribute named id?
Oh, backward compatibility, so how about node.has/getAttribute("id")? (Can't use getAttribute directly because it returns null for some nodes.)
Actually, node.id would probably work in older versions too if you added a typeof node.id == "string" or other similar check.
(In reply to neil@parkwaycc.co.uk 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?
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.)
(In reply to neil@parkwaycc.co.uk 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?
Attachment #743981 - Flags: review?(neil)
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...
Attachment #743981 - Flags: review?(neil) → review+
(neil@parkwaycc.co.uk wrote in comment #11)
> Indeed, search by attribute also has this bug...

Dear meandering bugzilla reader, that's bug 867540.
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.
Attachment #743621 - Attachment is obsolete: true
Attachment #743981 - Attachment is obsolete: true
Attachment #743621 - Flags: review?(neil)
Attachment #744221 - Flags: review?(neil)
(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...
(In reply to neil@parkwaycc.co.uk 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 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.
Attachment #744221 - Flags: review?(neil) → review+
(In reply to neil@parkwaycc.co.uk 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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Duplicate of this bug: 887722
Any chance of releasing 2.0.15 any day soon?
Duplicate of this bug: 923993
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.
(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 on attachment 8379295 [details]
DOMi 2.0.15pre as of 2014-02-14

Oops! A typo!
Attachment #8379295 - Attachment description: DOMi 2.0.15pre as of 2015-02-14 → DOMi 2.0.15pre as of 2014-02-14
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...
(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.
Status: RESOLVED → VERIFIED
Blocks: 1034858
Blocks: DOMi2.0.15
No longer blocks: DOMi2.0.16
You need to log in before you can comment on or make changes to this bug.