Bug 96626 - Web Inspector: Group selectors to highlight matched selector in the Styles pane of Elements Panel
Summary: Web Inspector: Group selectors to highlight matched selector in the Styles pa...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
: 31211 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-09-13 03:00 PDT by Alexander Pavlov (apavlov)
Modified: 2012-09-21 03:56 PDT (History)
12 users (show)

See Also:


Attachments
Patch (14.62 KB, patch)
2012-09-14 02:50 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[IMAGE] Screenshot of matching group selectors with the patch applied (16.63 KB, image/png)
2012-09-14 02:52 PDT, Alexander Pavlov (apavlov)
no flags Details
Patch (17.91 KB, patch)
2012-09-14 07:20 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[IMAGE] Screenshot - take 2 (25.66 KB, image/png)
2012-09-14 08:14 PDT, Alexander Pavlov (apavlov)
no flags Details
Patch (17.79 KB, patch)
2012-09-17 02:23 PDT, Alexander Pavlov (apavlov)
vsevik: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2012-09-13 03:00:34 PDT
It would be really helpful if the Styles pane, in the Elements Panel of the Web Inspector, highlighted or displayed in some way which selector was matched in a group selector. On large projects, selectors can be extended many times, leading to large and confusing selectors. When debugging specificity it would be immensely helpful if we could easily see which selector was matched.
Comment 1 Alexander Pavlov (apavlov) 2012-09-14 02:50:47 PDT
Created attachment 164080 [details]
Patch
Comment 2 Alexander Pavlov (apavlov) 2012-09-14 02:52:08 PDT
Created attachment 164081 [details]
[IMAGE] Screenshot of matching group selectors with the patch applied
Comment 3 Alexander Pavlov (apavlov) 2012-09-14 03:28:58 PDT
Upstreaming http://code.google.com/p/chromium/issues/detail?id=145483
Comment 4 Pavel Feldman 2012-09-14 03:45:33 PDT
I would dim non-matching selectors instead.
Comment 5 Vsevolod Vlasov 2012-09-14 03:56:37 PDT
Comment on attachment 164080 [details]
Patch

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

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1154
> +        var selectors = selectorText.split(",").map(trim);

var selectors = selectorText.split(",").map(String.prototype.trim);

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1177
> +            if (matches && node === paneNode)

s/paneNode/this._parentPane.node/

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1178
> +                selectors[selectorIndex] = "<span class=\"selector-matches\">" + selectors[selectorIndex].escapeHTML() + "</span>";

Let's create DOM element and setContent instead.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1179
> +            if (isLast) {

selectorIndex === selectors.length - 1

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1180
> +                self._selectorElement.innerHTML = selectors.join(", ");

We should use appendChild
Comment 6 Pavel Feldman 2012-09-14 04:04:16 PDT
Comment on attachment 164080 [details]
Patch

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

>> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1180
>> +                self._selectorElement.innerHTML = selectors.join(", ");
> 
> We should use appendChild

+1. Let's agree that any assignment of innerHTML to non-empy string is an automatic r- in the inspector codebase.
Comment 7 Vsevolod Vlasov 2012-09-14 05:13:31 PDT
(In reply to comment #6)
> (From update of attachment 164080 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164080&action=review
> 
> >> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1180
> >> +                self._selectorElement.innerHTML = selectors.join(", ");
> > 
> > We should use appendChild
> 
> +1. Let's agree that any assignment of innerHTML to non-empy string is an automatic r- in the inspector codebase.

I don't think there is a valid reason to use innerHTML for empty strings.
element.textContent = "" FTW
Comment 8 Alexander Pavlov (apavlov) 2012-09-14 07:20:39 PDT
Created attachment 164143 [details]
Patch
Comment 9 Alexander Pavlov (apavlov) 2012-09-14 08:14:29 PDT
Created attachment 164156 [details]
[IMAGE] Screenshot - take 2

Non-matching selectors are dimmed. When a section is hovered, all non-matching selectors turn black to improve readability. Inherited element styles' selectors are matched against their respective elements.
Comment 10 Pavel Feldman 2012-09-14 09:17:27 PDT
UI lgtm. Why is html rendered as matching in the inherited from body section?
Comment 11 Alexander Pavlov (apavlov) 2012-09-17 00:45:16 PDT
(In reply to comment #10)
> UI lgtm. Why is html rendered as matching in the inherited from body section?

It's because the mouse pointer is hovering over the section. In that case we un-dim the dimmed selectors to aid users with poor monitors :) and eyesight. Seva said that we should not regress the UI readability in general (that is, it should be possible to view the UI that is not harder to read than it was previously.)
Comment 12 Pavel Feldman 2012-09-17 00:49:42 PDT
I would only make it solid upon selector editing. I think dimmed selector is still very distinguishable.
Comment 13 Alexander Pavlov (apavlov) 2012-09-17 02:09:53 PDT
(In reply to comment #12)
> I would only make it solid upon selector editing. I think dimmed selector is still very distinguishable.

I trust your eyesight! :) Uploading an updated patch shortly.
Comment 14 Alexander Pavlov (apavlov) 2012-09-17 02:23:40 PDT
Created attachment 164359 [details]
Patch
Comment 15 Alexander Pavlov (apavlov) 2012-09-17 05:46:38 PDT
Committed r128746: <http://trac.webkit.org/changeset/128746>
Comment 16 Pavel Feldman 2012-09-18 03:47:49 PDT
I don't see why we need to make 2 additional requests per matched selectors call. We should change the protocol to return ready-to-use data instead.
Comment 17 Alexander Pavlov (apavlov) 2012-09-18 04:14:46 PDT
(In reply to comment #16)
> I don't see why we need to make 2 additional requests per matched selectors call. We should change the protocol to return ready-to-use data instead.

Are you suggesting to send indices of matched selectors in groups up-front?
Comment 18 Pavel Feldman 2012-09-18 04:23:21 PDT
When sending rules, we can send selectors in the model form (array of entities withe the flag responsible for those matching).
Comment 19 Alexander Pavlov (apavlov) 2012-09-21 03:56:49 PDT
*** Bug 31211 has been marked as a duplicate of this bug. ***