Bug 92648 - Web Inspector: Update $ to alias to querySelector rather than getElementById
: Web Inspector: Update $ to alias to querySelector rather than getElementById
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Addy Osmani
:
Depends on:
Blocks: 111053
  Show dependency treegraph
 
Reported: 2012-07-30 07:51 PDT by Addy Osmani
Modified: 2013-02-28 01:33 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.34 KB, patch)
2012-08-14 16:13 PDT, Addy Osmani
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (430.78 KB, application/zip)
2012-08-14 17:56 PDT, WebKit Review Bot
no flags Details
Patch (3.89 KB, patch)
2012-08-21 09:45 PDT, Addy Osmani
no flags Details | Formatted Diff | Diff
Patch (8.20 KB, patch)
2012-08-31 05:20 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Addy Osmani 2012-07-30 07:51:49 PDT
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.
Comment 1 Addy Osmani 2012-07-30 07:56:02 PDT
Related Firebug ticket https://bugzilla.mozilla.org/show_bug.cgi?id=778732
Comment 2 Addy Osmani 2012-07-30 08:03:37 PDT
(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
Comment 3 Steven Roussey 2012-07-31 23:10:34 PDT
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.
Comment 4 Paul Irish 2012-08-01 10:54:52 PDT
(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
Comment 5 Paul Irish 2012-08-01 13:43:56 PDT
Ah also here was the full poll introduction: https://docs.google.com/a/google.com/spreadsheet/viewform?formkey=dHA5RjFzbF9tcElCa3VXYm13ZTctdkE6MQ
Comment 6 Steven Roussey 2012-08-01 15:39:50 PDT
(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.
Comment 7 Rob Campbell 2012-08-13 21:05:09 PDT
fwiw, we implemented this in Firefox on August 3. Will be released in Firefox 17 in January 2013.
Comment 8 Pavel Feldman 2012-08-14 06:13:36 PDT
@addy: do you have a change for this?
Comment 9 Addy Osmani 2012-08-14 06:15:51 PDT
(In reply to comment #8)
> @addy: do you have a change for this?

I do. I'll submit a patch for it today.
Comment 10 Steven Roussey 2012-08-14 11:46:35 PDT
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.
Comment 11 Addy Osmani 2012-08-14 16:13:29 PDT
Created attachment 158437 [details]
Patch
Comment 12 WebKit Review Bot 2012-08-14 17:56:54 PDT
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
Comment 13 WebKit Review Bot 2012-08-14 17:56:58 PDT
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
Comment 14 Addy Osmani 2012-08-14 18:03:54 PDT
(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!
Comment 15 Pavel Feldman 2012-08-14 23:50:09 PDT
(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.
Comment 16 Steven Roussey 2012-08-15 08:55:51 PDT
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?
Comment 17 Pavel Feldman 2012-08-15 09:48:51 PDT
Comment on attachment 158437 [details]
Patch

r- for failing tests.
Comment 18 Addy Osmani 2012-08-21 09:45:55 PDT
Created attachment 159703 [details]
Patch
Comment 19 Addy Osmani 2012-08-21 09:49:55 PDT
(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.
Comment 20 Pavel Feldman 2012-08-21 10:07:08 PDT
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);
Comment 21 Steven Roussey 2012-08-21 11:28:14 PDT
> 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?
Comment 22 Pavel Feldman 2012-08-31 02:56:46 PDT
@addy: do you want me to pick it up?
Comment 23 Addy Osmani 2012-08-31 03:58:03 PDT
(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.
Comment 24 Pavel Feldman 2012-08-31 05:20:30 PDT
Created attachment 161670 [details]
Patch
Comment 25 WebKit Review Bot 2012-08-31 08:39:51 PDT
Comment on attachment 161670 [details]
Patch

Clearing flags on attachment: 161670

Committed r127266: <http://trac.webkit.org/changeset/127266>
Comment 26 WebKit Review Bot 2012-08-31 08:39:55 PDT
All reviewed patches have been landed.  Closing bug.