Skip to content

Commit

Permalink
Issue 5764: Change JSTerm's $ helper function from getElementById to …
Browse files Browse the repository at this point in the history
…querySelector
  • Loading branch information
steve authored and sroussey committed Aug 4, 2012
1 parent 893d740 commit 4bbd8f9
Showing 1 changed file with 24 additions and 4 deletions.
28 changes: 24 additions & 4 deletions extension/content/firebug/console/commandLine.js
Expand Up @@ -1043,14 +1043,34 @@ Firebug.CommandLine.CommandHandler = Obj.extend(Object,

function FirebugCommandLineAPI(context)
{
this.$ = function(id) // returns unwrapped elements from the page
this.$ = function(selector, start) // returns unwrapped elements from the page
{
return Wrapper.unwrapObject(context.baseWindow.document).getElementById(id);
var result;
if (start && start.querySelector && (start.nodeType == 1 || start.nodeType == 9 || start.nodeType == 11))
result = start.querySelector(selector);
else
{
result = Wrapper.unwrapObject(context.baseWindow.document).querySelector(selector);
if (result == null && (selector||"")[0] !== "#")
{
result = Wrapper.unwrapObject(context.baseWindow.document).querySelector("#" + selector);
if (result != null)
{
Firebug.Console.log("The console function $() has changed from $=getElementById(id) to $=querySelector(selector). You might try $(\"#" + selector + "\")", context, "warn");
result = null;
}
}
}
return result;
};

this.$$ = function(selector) // returns unwrapped elements from the page
this.$$ = function(selector, start) // returns unwrapped elements from the page
{
var result = Wrapper.unwrapObject(context.baseWindow.document).querySelectorAll(selector);
var result;
if (start && start.querySelectorAll && (start.nodeType == 1 || start.nodeType == 9 || start.nodeType == 11))
result = start.querySelectorAll(selector);
else
result = Wrapper.unwrapObject(context.baseWindow.document).querySelectorAll(selector);
return Arr.cloneArray(result);
};

Expand Down

3 comments on commit 4bbd8f9

@SebastianZ
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three points:

  • $() doesn't seem to work for me as expected (try $(".class1") on Joe's test case)
  • Better use constants instead of magic numbers.
  • Add a comment that the warning should be removed in a future version

Sebastian

@sroussey
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Wow, that is an old page of Joe's! It also loads prototype.js on the page, so Firebug will use that version of $, of course. I deal with more PrototypeJS stuff than jQuery stuff, believe it or not. So, yeah, this is a problem with the two JS libs defining incompatible versions, and our switching around. This is an example of the bogus bug reports you will get. ;)
  • Yeah, I will change if we decide to keep this functionality (or before)
  • Yes, one release later I think, we should pull the warning. I'll comment it up.

@SebastianZ
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Wow, that is an old page of Joe's!

Yes. Though I use it a lot for reproducing issues.

It also loads prototype.js on the page, so Firebug will use that version of $

Oh my! I totally forgot about that! Sorry, Steven! So I tested it on other sites and it's working as expected.

This is an example of the bogus bug reports you will get. ;)

Right. I wonder why we don't get this kind of reports more often already...

  • Yeah, I will change if we decide to keep this functionality (or before)
  • Yes, one release later I think, we should pull the warning. I'll comment it up.

Good.

Sebastian

Please sign in to comment.