Bug 140647 – Web Inspector: Should show dynamic specificity values

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* 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.

[Radar WebKit Bug Importer]

<rdar://problem/19526046>

[Joseph Pecoraro]

Created attachment 244940 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

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 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 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]

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]

> > 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]

(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]

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]

> > 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]

Created attachment 245019 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 245020 [details]
[IMAGE] Tooltip - Dynamic Specificity matching current element

[Joseph Pecoraro]

Created attachment 245021 [details]
[IMAGE] Tooltip - Dynamic Specificity matching parent element (inherited)

[Joseph Pecoraro]

Created attachment 245023 [details]
[IMAGE] Tooltip - Dynamic Specificity not matching current element (no value)

[WebKit Commit Bot]

Comment on attachment 245019 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 245019

Committed r178767: <http://trac.webkit.org/changeset/178767>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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.