| Summary: | Web Inspector: Update $ to alias to querySelector rather than getElementById | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Addy Osmani <addyo> | ||||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Addy Osmani <addyo> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | dglazkov, paulirish, pfeldman, rcampbell, sroussey, syoichi, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 111053 | ||||||||||||
| Attachments: |
|
||||||||||||
Related Firebug ticket https://bugzilla.mozilla.org/show_bug.cgi?id=778732 (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 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. (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 Ah also here was the full poll introduction: https://docs.google.com/a/google.com/spreadsheet/viewform?formkey=dHA5RjFzbF9tcElCa3VXYm13ZTctdkE6MQ (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. fwiw, we implemented this in Firefox on August 3. Will be released in Firefox 17 in January 2013. @addy: do you have a change for this? (In reply to comment #8) > @addy: do you have a change for this? I do. I'll submit a patch for it today. 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. Created attachment 158437 [details]
Patch
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 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
(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! (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. 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 on attachment 158437 [details]
Patch
r- for failing tests.
Created attachment 159703 [details]
Patch
(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 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);
> 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?
@addy: do you want me to pick it up? (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. Created attachment 161670 [details]
Patch
Comment on attachment 161670 [details] Patch Clearing flags on attachment: 161670 Committed r127266: <http://trac.webkit.org/changeset/127266> All reviewed patches have been landed. Closing bug. |
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.