Bug 140647

Summary: Web Inspector: Should show dynamic specificity values
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, commit-queue, graouts, joepeck, jonowells, mattbaker, nvasilyev, rniwa, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
joepeck: review-, buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-mavericks-wk2
none
[PATCH] Proposed Fix
none
[IMAGE] Tooltip - Dynamic Specificity matching current element
none
[IMAGE] Tooltip - Dynamic Specificity matching parent element (inherited)
none
[IMAGE] Tooltip - Dynamic Specificity not matching current element (no value) none

Joseph Pecoraro
Reported 2015-01-19 16:44:19 PST
* SUMMARY Should show dynamic specificity values of :matches(...) against the current selected element. * TEST <style> :matches(div, .foo) { color: blue; } </style> <div>Specificity for this is (0, 0, 1)</div> <p class="foo">Specificity for this is (0, 1, 0)</p> * STEPS TO REPRODUCE 1. Inspect the <div> in the test case. => specificity of :matches should be (0, 0, 1) 2. Inspect the <p> in the test case => specificity of :matches should be (0, 1, 0) * NOTES - It might also be nice to show a * or some explanatory text (e.g. "(0, 0, 1)*") to clarify that the specificity is dynamic and may change for different elements.
Attachments
[PATCH] Proposed Fix (23.01 KB, patch)
2015-01-19 16:49 PST, Joseph Pecoraro
joepeck: review-
buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (885.07 KB, application/zip)
2015-01-19 17:44 PST, Build Bot
no flags
[PATCH] Proposed Fix (29.44 KB, patch)
2015-01-20 14:50 PST, Joseph Pecoraro
no flags
[IMAGE] Tooltip - Dynamic Specificity matching current element (20.14 KB, image/png)
2015-01-20 14:52 PST, Joseph Pecoraro
no flags
[IMAGE] Tooltip - Dynamic Specificity matching parent element (inherited) (20.69 KB, image/png)
2015-01-20 14:53 PST, Joseph Pecoraro
no flags
[IMAGE] Tooltip - Dynamic Specificity not matching current element (no value) (25.22 KB, image/png)
2015-01-20 14:53 PST, Joseph Pecoraro
no flags
Radar WebKit Bug Importer
Comment 1 2015-01-19 16:44:40 PST
Joseph Pecoraro
Comment 2 2015-01-19 16:49:11 PST
Created attachment 244940 [details] [PATCH] Proposed Fix
WebKit Commit Bot
Comment 3 2015-01-19 16:51:49 PST
Attachment 244940 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/InspectorStyleSheet.h:241: The parameter name "element" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/inspector/InspectorStyleSheet.h:242: The parameter name "element" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Benjamin Poulain
Comment 4 2015-01-19 17:17:04 PST
Comment on attachment 244940 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=244940&action=review > LayoutTests/ChangeLog:9 > + * inspector/css/selector-dynamic-specificity.html: Added. Gosh, those Inspector tests aren't easy to write. > Source/WebCore/inspector/InspectorCSSAgent.h:155 > + RefPtr<Inspector::Protocol::CSS::CSSRule> buildObjectForRule(StyleRule*, StyleResolver&, Element*); Shouldn't element be const? > Source/WebCore/inspector/InspectorStyleSheet.cpp:1010 > + selectorChecker.match(&selector, element, context, specificity); It would be good to get the result and ASSERT it is true. The specificity is nonsense if matching fails.
Build Bot
Comment 5 2015-01-19 17:44:06 PST
Comment on attachment 244940 [details] [PATCH] Proposed Fix Attachment 244940 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5094626008498176 New failing tests: inspector/css/selector-specificity.html
Build Bot
Comment 6 2015-01-19 17:44:09 PST
Created attachment 244945 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Joseph Pecoraro
Comment 7 2015-01-19 17:52:12 PST
> > Source/WebCore/inspector/InspectorStyleSheet.cpp:1010 > > + selectorChecker.match(&selector, element, context, specificity); > > It would be good to get the result and ASSERT it is true. > The specificity is nonsense if matching fails. Oh, darn, this is what caused the existing test to fail. Maybe we should keep the old static calculation in if a selector is not-dynamic and doesn't match. That way at least we can know the specificity.
Benjamin Poulain
Comment 8 2015-01-19 20:05:06 PST
(In reply to comment #7) > > > Source/WebCore/inspector/InspectorStyleSheet.cpp:1010 > > > + selectorChecker.match(&selector, element, context, specificity); > > > > It would be good to get the result and ASSERT it is true. > > The specificity is nonsense if matching fails. > > Oh, darn, this is what caused the existing test to fail. > > Maybe we should keep the old static calculation in if a selector is > not-dynamic and doesn't match. That way at least we can know the specificity. Why would you have the selector if it does not match the element?
Joseph Pecoraro
Comment 9 2015-01-19 20:13:31 PST
If a CSS Rule matches the current selected element, we show that rule in the sidebar. We calculate the specificity for each selector in that rule, some may match the element, some may not. Given this test: <style> .foo, .bar { color: green; } </style> <div class="foo">Inspect Me</div> Inspecting .foo, we would expect to show the specificity for both ".foo" and ".bar" in the sidebar, despite only ".foo" matching.
Joseph Pecoraro
Comment 10 2015-01-20 13:58:55 PST
> > Source/WebCore/inspector/InspectorCSSAgent.h:155 > > + RefPtr<Inspector::Protocol::CSS::CSSRule> buildObjectForRule(StyleRule*, StyleResolver&, Element*); > > Shouldn't element be const? Unfortunately, eventually it has to pass down to SelectorChecker::match which takes a non-const Element. Maybe that could be made const, I didn't check.
Joseph Pecoraro
Comment 11 2015-01-20 14:50:46 PST
Created attachment 245019 [details] [PATCH] Proposed Fix
Joseph Pecoraro
Comment 12 2015-01-20 14:52:51 PST
Created attachment 245020 [details] [IMAGE] Tooltip - Dynamic Specificity matching current element
Joseph Pecoraro
Comment 13 2015-01-20 14:53:16 PST
Created attachment 245021 [details] [IMAGE] Tooltip - Dynamic Specificity matching parent element (inherited)
Joseph Pecoraro
Comment 14 2015-01-20 14:53:35 PST
Created attachment 245023 [details] [IMAGE] Tooltip - Dynamic Specificity not matching current element (no value)
WebKit Commit Bot
Comment 15 2015-01-20 16:30:34 PST
Comment on attachment 245019 [details] [PATCH] Proposed Fix Clearing flags on attachment: 245019 Committed r178767: <http://trac.webkit.org/changeset/178767>
WebKit Commit Bot
Comment 16 2015-01-20 16:30:39 PST
All reviewed patches have been landed. Closing bug.
Alexey Proskuryakov
Comment 17 2015-01-20 23:30:28 PST
The new test appears to be horrible slow on some bots, any insight why? http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fcss%2Fselector-dynamic-specificity.html Strangely, of all Mac bots only Yosemite Debug WK2 is affected.
Note You need to log in before you can comment on or make changes to this bug.