Bug 36509 – Web Inspector: Provide pseudo element styles information into the styles sidebar panel.

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

[Pavel Feldman]

This is the first (backend) part of providing pseudo elements information in the inspector styles sidebar pane.
Changes applied:
- CSSStyleSelector::pseudoStyleRulesForElement implemented. I changed the signature to accept PseudoId instead of a String since it better reflected the needs;
- I did not remove elementStyle checks as was agreed with Dave on IRC , but accompanied them with m_collectRulesOnly checks instead (as was also discussed). It just feels more safe to me, but I can reconsider based on the review feedback;
- Removed pseudoStyleRulesForElement usage from DOMWindow::getMatchedCSSRules. It was anyways returning 0, while i would need to String to PseudoId in order to leave the call in place;
- InspectorDOMAgent is now iterating over pseudo ids from FIRST_PUBLIC_PSEUDOID to FIRST_INTERNAL_PSEUDOID and serializes applicable ones.

[Pavel Feldman]

Created attachment 51457 [details]
[PATCH] Proposed change.

[WebKit Review Bot]

Attachment 51457 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSStyleSelector.cpp:1864:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Created attachment 51503 [details]
[PATCH] Same with front-end changes. Now iterating over internal pseudo element styles as well.

[Pavel Feldman]

Created attachment 51505 [details]
[IMAGE] Screenshot while running with patch.

Note that color:blue is overridden for pseudo element, but not for the element itself, although belonging to the same rule.

[Timothy Hatcher]

I don't think the title of the pseudo sections are good. It looks weird as "Pseudo ::before element". I think "Before Pseudo Element" would be better. They can see the real selectors below, so that section title does not need to mimic how it is written in the CSS code.

We really need to eliminate the nesting in these sections…

On a side note when we get around to supporting psuedo classes, I think those should be handled with a popup to select the state you want to view. The popup could live next to the gear menu. You would pick from "normal", "hover", "active", etc. And you would only see rules that apply during the selected state.

Another ide note, I think Firebug shows pseudo content/elements in the DOM tree, but I think that is really weird…

[Pavel Feldman]

Created attachment 51520 [details]
[IMAGE] Playing with styles to reduce clutter.

[Pavel Feldman]

(In reply to comment #5)
> I don't think the title of the pseudo sections are good. It looks weird as
> "Pseudo ::before element". I think "Before Pseudo Element" would be better.
> They can see the real selectors below, so that section title does not need to
> mimic how it is written in the CSS code.
>

I'd really need a friendly caption for literally every pseudo element... This thing is supposed to group all of ::after type together and show overrides local to pseudo elements.
 
> We really need to eliminate the nesting in these sections…
> 

See a screenshot attached. It somewhat reduced generated clutter.

> On a side note when we get around to supporting psuedo classes, I think those
> should be handled with a popup to select the state you want to view. The popup
> could live next to the gear menu. You would pick from "normal", "hover",
> "active", etc. And you would only see rules that apply during the selected
> state.
> 

I am not sure about this one. Presenting them all together boosts discoverability.

> Another ide note, I think Firebug shows pseudo content/elements in the DOM
> tree, but I think that is really weird…

Nah.

[Timothy Hatcher]

Comment on attachment 51503 [details]
[PATCH] Same with front-end changes. Now iterating over internal pseudo element styles as well.

> +        This change provides pseudo elements information into the
> +        inspector styles sidebar pane.

Why wrap this to two lines when most of the ChangeLog is longer than this?


> +            String attributeName = attribute->localName();
> +            styleAttributes.set(attributeName.utf8().data(), buildObjectForStyle(attribute->style(), true));

Why dosen't set take a String instead of a UTF8 char*?


> +WebInspector.StylesSidebarPane.PseudoIdNames = [
> +    "", "first-line", "first-letter", "before", "after", "selection", "", "-webkit-scrollbar", "-webkit-file-upload-button",

Why are some empty? Comment about this? I can see this getting out of sync really easy. It would help to add a comment in RenderStyleConstants.h too. Is there anyway to send the string instead of the PseudoId?


> -        if (!refresh) {
> +        if (refresh)
> +            for (var pseudoId in this.sections) {
> +                var styleRules = this._refreshStyleRules(this.sections[pseudoId], styles);
> +                var usedProperties = {};
> +                var disabledComputedProperties = {};
> +                this._markUsedProperties(styleRules, usedProperties, disabledComputedProperties);
> +                this._refreshSectionsForStyleRules(styleRules, usedProperties, disabledComputedProperties, editedSection);
> +            }
> +        else {

There should be brace around all of this, since it is multi-line even if it is a single statement.


> -PassRefPtr<CSSRuleList> DOMWindow::getMatchedCSSRules(Element* elt, const String& pseudoElt, bool authorOnly) const
> +PassRefPtr<CSSRuleList> DOMWindow::getMatchedCSSRules(Element* elt, const String&, bool authorOnly) const
>  {
>      if (!m_frame)
>          return 0;
>  
>      Document* doc = m_frame->document();
> -
> -    if (!pseudoElt.isEmpty())
> -        return doc->styleSelector()->pseudoStyleRulesForElement(elt, pseudoElt, authorOnly);
>      return doc->styleSelector()->styleRulesForElement(elt, authorOnly);
>  }

Why this change?

[Timothy Hatcher]

(In reply to comment #7)
> (In reply to comment #5)
> > On a side note when we get around to supporting psuedo classes, I think those
> > should be handled with a popup to select the state you want to view. The popup
> > could live next to the gear menu. You would pick from "normal", "hover",
> > "active", etc. And you would only see rules that apply during the selected
> > state.
> > 
> 
> I am not sure about this one. Presenting them all together boosts
> discoverability.

I don't think pseudo classes should be shown all together because it makes you think all of them apply at the same time, and they don't.

It might be an issue if you have multiple ones applying at the same time though not just one.

I just fear the clutter pseudo classes will cause.

[Timothy Hatcher]

I don't not like the clutter reduction change. It does not help in my opinion.

[Timothy Hatcher]

(In reply to comment #7)
> (In reply to comment #5)
> > I don't think the title of the pseudo sections are good. It looks weird as
> > "Pseudo ::before element". I think "Before Pseudo Element" would be better.
> > They can see the real selectors below, so that section title does not need to
> > mimic how it is written in the CSS code.
> >
> 
> I'd really need a friendly caption for literally every pseudo element... This
> thing is supposed to group all of ::after type together and show overrides
> local to pseudo elements.

It could be as simple as split on "-" remove empty strings and "webkit" and capatilize each component. Then insert in "%@ Pseudo Element".

Saying "Pseudo ::before element" is redundant in my book…

[Pavel Feldman]

(In reply to comment #8)
> (From update of attachment 51503 [details])
> > +        This change provides pseudo elements information into the
> > +        inspector styles sidebar pane.
> 
> Why wrap this to two lines when most of the ChangeLog is longer than this?
> 

I was changing the contents since it is now has both backend + frontend fix.
Fixed.

> 
> > +            String attributeName = attribute->localName();
> > +            styleAttributes.set(attributeName.utf8().data(), buildObjectForStyle(attribute->style(), true));
> 
> Why dosen't set take a String instead of a UTF8 char*?
> 

Just a legacy matter. Some day we need to change them all.

> 
> > +WebInspector.StylesSidebarPane.PseudoIdNames = [
> > +    "", "first-line", "first-letter", "before", "after", "selection", "", "-webkit-scrollbar", "-webkit-file-upload-button",
> 
> Why are some empty? Comment about this? I can see this getting out of sync
> really easy. It would help to add a comment in RenderStyleConstants.h too. Is
> there anyway to send the string instead of the PseudoId?
> 

Adding comment. The mapping is somewhat complex, there is no direct API for fetching textual name for the pseudo id. Looks like we are the only ones in need for this.

> 
> There should be brace around all of this, since it is multi-line even if it is
> a single statement.
> 

Fixed.

> > -
> > -    if (!pseudoElt.isEmpty())
> > -        return doc->styleSelector()->pseudoStyleRulesForElement(elt, pseudoElt, authorOnly);
> >      return doc->styleSelector()->styleRulesForElement(elt, authorOnly);
> >  }
> 
> Why this change?

It is mentioned in the ChangeLog. pseudoStyleRulesForElement used to return 0 at all times, now it is now. I thought that removing the call to pseudoStyleRulesForElement and preserving the original method logic would be a good thing to do. Given that we are likely to get rid of the getMatchedCSSRules as a whole (there are not internal clients of this method other than the js binding), I did not want to invest into making proper call to pseudoStyleRulesForElement. It would require converting pseudo name to id (and again we don't have good mapping anywhere).

[Pavel Feldman]

(In reply to comment #10)
> I don't not like the clutter reduction change. It does not help in my opinion.

Yes, it is not helping much. But we need to do something. I'll keep experimenting.

[Pavel Feldman]

Created attachment 51607 [details]
[PATCH] Review comments addressed. dhyatt: could you please take another look?

[Beth Dakin]

Comment on attachment 51607 [details]
[PATCH] Review comments addressed. dhyatt: could you please take another look?

The changes in CSSStyleSelector.cpp, CSSStyleSelector.h, DOMWindow.cpp, and RenderStyleConstants.h look good to me. Tim should look over the Web Inspector parts again if he hasn't already.

[Timothy Hatcher]

Comment on attachment 51607 [details]
[PATCH] Review comments addressed. dhyatt: could you please take another look?

I have looked over the Inspector parts. Thanks Beth.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/English.lproj/localizedStrings.js
	M	WebCore/css/CSSStyleSelector.cpp
	M	WebCore/css/CSSStyleSelector.h
	M	WebCore/inspector/InspectorDOMAgent.cpp
	M	WebCore/inspector/InspectorDOMAgent.h
	M	WebCore/inspector/front-end/StylesSidebarPane.js
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/page/DOMWindow.cpp
	M	WebCore/rendering/style/RenderStyleConstants.h
Committed r56608