WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
36509
Web Inspector: Provide pseudo element styles information into the styles sidebar panel.
https://bugs.webkit.org/show_bug.cgi?id=36509
Summary
Web Inspector: Provide pseudo element styles information into the styles side...
Pavel Feldman
Reported
2010-03-23 14:59:54 PDT
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.
Attachments
[PATCH] Proposed change.
(12.78 KB, patch)
2010-03-23 15:04 PDT
,
Pavel Feldman
no flags
Details
Formatted Diff
Diff
[PATCH] Same with front-end changes. Now iterating over internal pseudo element styles as well.
(38.65 KB, patch)
2010-03-24 08:45 PDT
,
Pavel Feldman
no flags
Details
Formatted Diff
Diff
[IMAGE] Screenshot while running with patch.
(105.65 KB, image/png)
2010-03-24 08:49 PDT
,
Pavel Feldman
no flags
Details
[IMAGE] Playing with styles to reduce clutter.
(95.11 KB, image/png)
2010-03-24 10:44 PDT
,
Pavel Feldman
no flags
Details
[PATCH] Review comments addressed. dhyatt: could you please take another look?
(38.98 KB, patch)
2010-03-25 01:47 PDT
,
Pavel Feldman
timothy
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Pavel Feldman
Comment 1
2010-03-23 15:04:14 PDT
Created
attachment 51457
[details]
[PATCH] Proposed change.
WebKit Review Bot
Comment 2
2010-03-23 15:09:34 PDT
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
Comment 3
2010-03-24 08:45:13 PDT
Created
attachment 51503
[details]
[PATCH] Same with front-end changes. Now iterating over internal pseudo element styles as well.
Pavel Feldman
Comment 4
2010-03-24 08:49:16 PDT
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
Comment 5
2010-03-24 10:19:07 PDT
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
Comment 6
2010-03-24 10:44:14 PDT
Created
attachment 51520
[details]
[IMAGE] Playing with styles to reduce clutter.
Pavel Feldman
Comment 7
2010-03-24 10:46:41 PDT
(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 8
2010-03-24 11:33:11 PDT
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
Comment 9
2010-03-24 11:35:53 PDT
(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
Comment 10
2010-03-24 11:48:33 PDT
I don't not like the clutter reduction change. It does not help in my opinion.
Timothy Hatcher
Comment 11
2010-03-24 11:51:07 PDT
(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
Comment 12
2010-03-24 12:21:07 PDT
(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
Comment 13
2010-03-24 12:21:59 PDT
(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
Comment 14
2010-03-25 01:47:06 PDT
Created
attachment 51607
[details]
[PATCH] Review comments addressed. dhyatt: could you please take another look?
Beth Dakin
Comment 15
2010-03-25 10:56:14 PDT
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 16
2010-03-25 11:00:29 PDT
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
Comment 17
2010-03-26 00:13:49 PDT
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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug