Bug 92648 – Web Inspector: Update $ to alias to querySelector rather than getElementById

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Addy Osmani]

We currently alias $ to document.getElementById(), however this limits the capabilities of the selector to only the ID. With querySelector available and capable of doing a lot more, I think we should switch to it.

Paul Irish and I have spoken to the Firebug guys about this and their implementation is effectively $ == getElementById and $$ == qSA too. The good news is they're up for switching to $ = qS and $$ = qSA if we are, at the same time.

If we're up for this, I'd like to land the patch (it would be my first) :)

PS: opting for qS over qSA to avoid people having to learn to use $("#id")[0] etc.

[Addy Osmani]

Related Firebug ticket https://bugzilla.mozilla.org/show_bug.cgi?id=778732

[Addy Osmani]

(In reply to comment #1)
> Related Firebug ticket https://bugzilla.mozilla.org/show_bug.cgi?id=778732

Correction: http://code.google.com/p/fbug/issues/detail?id=5764

[Steven Roussey]

What is the point of this? Other than to make the PrototypeJS based design more like a jQuery one? I'm not necessarily opposed per se, but changing this underneath a global set of developers could be a... uh... bad experience.

[Paul Irish]

(In reply to comment #3)
> but changing this underneath a global set of developers could be a... uh... bad experience.

It appears to definitely match expectations better. I polled ~500 developers and 88% are in favor.
https://docs.google.com/a/google.com/spreadsheet/viewanalytics?formkey=dHA5RjFzbF9tcElCa3VXYm13ZTctdkE6MQ

[Paul Irish]

Ah also here was the full poll introduction: https://docs.google.com/a/google.com/spreadsheet/viewform?formkey=dHA5RjFzbF9tcElCa3VXYm13ZTctdkE6MQ

[Steven Roussey]

(In reply to comment #4)
> (In reply to comment #3)
> > but changing this underneath a global set of developers could be a... uh... bad experience.
> 
> It appears to definitely match expectations better. I polled ~500 developers and 88% are in favor.
> https://docs.google.com/a/google.com/spreadsheet/viewanalytics?formkey=dHA5RjFzbF9tcElCa3VXYm13ZTctdkE6MQ

From my point of view, the jQuery team asked people that follow them (jQuery users), about how they want to access non-jQuery sites in the console. They want it jQuery-like. I'm hardly surprised. (The vote count also tracks the position of the answer in the list for both questions, and the questions are leading, but those are separate issues.)

I'm not necessarily opposed per se, but I'm just not convinced either.

We could popup a poll page from Firebug for people that are regularly using the $() where it is not already defined (likely not jQuery pages, though they can be jQuery devs). That poll result would be more interesting. But I'm not sure bothering people like that is worth it.

[Rob Campbell]

fwiw, we implemented this in Firefox on August 3. Will be released in Firefox 17 in January 2013.

[Pavel Feldman]

@addy: do you have a change for this?

[Addy Osmani]

(In reply to comment #8)
> @addy: do you have a change for this?

I do. I'll submit a patch for it today.

[Steven Roussey]

Firebug has a patch in the console-dollar branch at github. While I was at it, I added a second param which is an element from which to start the search. We also had to change a bunch of tests (not a one-liner change in the end), and we have a temp warning if people try to search for an id that is on the page and don't use # before the id name.

[Addy Osmani]

Created attachment 158437 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158437 [details]
Patch

Attachment 158437 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13493897

New failing tests:
inspector/console/command-line-api-inspect.html
inspector/console/command-line-api.html

[WebKit Review Bot]

Created attachment 158463 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Addy Osmani]

(In reply to comment #13)
> Created an attachment (id=158463) [details]
> Archive of layout-test-results from gce-cr-linux-01
> 
> The attached test failures were seen while running run-webkit-tests on the chromium-ews.
> Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

I'll fix the failing tests in the morning. Sorry for the delay!

[Pavel Feldman]

(In reply to comment #10)
> Firebug has a patch in the console-dollar branch at github. While I was at it, I added a second param which is an element from which to start the search. We also had to change a bunch of tests (not a one-liner change in the end), and we have a temp warning if people try to search for an id that is on the page and don't use # before the id name.

Those sound like good ideas. Temp warning if people try to search for id with no # is a great idea. I think we should do that as well. And we will also need to rebaseline some tests + add new ones for this warning message.

[Steven Roussey]

The wording for the warning (as it stands at the moment) is:

"The console function $() has changed from $=getElementById(id) to $=querySelector(selector). You might try $(\"#" + selector + "\")"

And only triggers if the querySelector failed but getElementById succeeded. I don't return anything, instead trying to push the user's muscle memory to add the #. But open to suggestions.

The second param which is an element from which to start the search, is something that I've seen people ask for in regards to $$, and I saw again in a discussion on Google+ in a thread started my Google's chrome evangelist in talking about Chrome's DevTools just last week. With the change to querySelector, it seems like the right time to make that switch also, as it now applies to $ as well as $$.

Are you changing $$ to return an array like Firebug does?

[Pavel Feldman]

Comment on attachment 158437 [details]
Patch

r- for failing tests.

[Addy Osmani]

Created attachment 159703 [details]
Patch

[Addy Osmani]

(In reply to comment #18)
> Created an attachment (id=159703) [details]
> Patch

Update to reflect our desire to have something like what Steve has implemented for Firebug. Some code style changes left to be updated and more thorough tests likely needed.

[Pavel Feldman]

Comment on attachment 159703 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159703&action=review

> Source/WebCore/inspector/InjectedScriptSource.js:932
> +    $: function (selector, start) {

{ should be on the next line.

> Source/WebCore/inspector/InjectedScriptSource.js:935
> +            result = start.querySelector(selector);

WebKit suggests that you do return start.querySelector(selector); here and remove else from below.

> Source/WebCore/inspector/InjectedScriptSource.js:939
> +                result = document.getElementById(selector);

This branch requires a separate case in command-line-api.html.

> Source/WebCore/inspector/InjectedScriptSource.js:941
> +                    console.warn("The console function $() has changed from $=getElementById(id) to $=querySelector(selector). You might try $(\"#" + selector + "\")");

You could use message format:
console.warn("The console function $() has changed from $=getElementById(id) to $=querySelector(selector). You might try $(\"#%s\")", selector);

> Source/WebCore/inspector/InjectedScriptSource.js:942
> +                    result = null;

return null;

> Source/WebCore/inspector/InjectedScriptSource.js:949
> +    $$: function (selector, start) {

{ on the next line.

> Source/WebCore/inspector/InjectedScriptSource.js:952
> +            result = start.querySelectorAll(selector);

return start.querySelectorAll(selector);

[Steven Roussey]

> Source/WebCore/inspector/InjectedScriptSource.js:956
> +        return result;

The other thing here (and this is a change that would match Firebug), is to return an array rather than a live node list. But perhaps another bug?

[Pavel Feldman]

@addy: do you want me to pick it up?

[Addy Osmani]

(In reply to comment #22)
> @addy: do you want me to pick it up?

I think you might need to if this is to land soon :/ I probably won't be able to get this updated until next week.

[Pavel Feldman]

Created attachment 161670 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161670 [details]
Patch

Clearing flags on attachment: 161670

Committed r127266: <http://trac.webkit.org/changeset/127266>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.