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
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 111053
  Show dependency treegraph
 
Reported: 2012-07-30 07:51 PST by
Modified: 2013-02-28 01:33 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-07-30 07:51:49 PST
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 From 2012-07-30 07:56:02 PST -------
Related Firebug ticket https://bugzilla.mozilla.org/show_bug.cgi?id=778732
------- Comment #2 From 2012-07-30 08:03:37 PST -------
(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 From 2012-07-31 23:10:34 PST -------
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 From 2012-08-01 10:54:52 PST -------
(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 From 2012-08-01 13:43:56 PST -------
Ah also here was the full poll introduction: https://docs.google.com/a/google.com/spreadsheet/viewform?formkey=dHA5RjFzbF9tcElCa3VXYm13ZTctdkE6MQ
------- Comment #6 From 2012-08-01 15:39:50 PST -------
(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 From 2012-08-13 21:05:09 PST -------
fwiw, we implemented this in Firefox on August 3. Will be released in Firefox 17 in January 2013.
------- Comment #8 From 2012-08-14 06:13:36 PST -------
@addy: do you have a change for this?
------- Comment #9 From 2012-08-14 06:15:51 PST -------
(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 From 2012-08-14 11:46:35 PST -------
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 From 2012-08-14 16:13:29 PST -------
Created an attachment (id=158437) [details]
Patch
------- Comment #12 From 2012-08-14 17:56:54 PST -------
(From update of attachment 158437 [details])
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 From 2012-08-14 17:56:58 PST -------
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
------- Comment #14 From 2012-08-14 18:03:54 PST -------
(In reply to comment #13)
> Created an attachment (id=158463) [details] [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 From 2012-08-14 23:50:09 PST -------
(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 From 2012-08-15 08:55:51 PST -------
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 From 2012-08-15 09:48:51 PST -------
(From update of attachment 158437 [details])
r- for failing tests.
------- Comment #18 From 2012-08-21 09:45:55 PST -------
Created an attachment (id=159703) [details]
Patch
------- Comment #19 From 2012-08-21 09:49:55 PST -------
(In reply to comment #18)
> Created an attachment (id=159703) [details] [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 From 2012-08-21 10:07:08 PST -------
(From update of attachment 159703 [details])
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 From 2012-08-21 11:28:14 PST -------
> 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 From 2012-08-31 02:56:46 PST -------
@addy: do you want me to pick it up?
------- Comment #23 From 2012-08-31 03:58:03 PST -------
(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 From 2012-08-31 05:20:30 PST -------
Created an attachment (id=161670) [details]
Patch
------- Comment #25 From 2012-08-31 08:39:51 PST -------
(From update of attachment 161670 [details])
Clearing flags on attachment: 161670

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