Bug 137378 – Web Inspector: Highlighted selectors in Rules sidebar break with selectors that contain nested selector lists

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

[Joseph Pecoraro]

* TEST

    <style>
    .testClass, a,span  ,  :nth-child(n of *, p), bumble, p { color: blue; }
    </style>
    <p class="testClass">Test</p>

* STEPS TO REPRODUCE
1. Inspect the <p> on the test page
2. Open Styles Sidebar to the Rules section
  => in the Rules section ".testClass", ":nth-child(n of *", and "bumble" are highlighted as matching this particular element
  => expected ".testClass", ":nth-child(n of *, p)" and "p"

[Radar WebKit Bug Importer]

<rdar://problem/18535308>

[Joseph Pecoraro]

Nested selector lists are causing the CSSParser SourceData to be out of whack.

On the protocol, we see the list of selectors is broken at the comma of the nested list:

    "selectorList": {
        "selectors": [".testClass", "a", "span", ":nth-child(n of *", "p)", "bumble", "p"],
        "text": ".testclass, a, span, :nth-child(n of *, p), bumble, p",
        ...
    }

This is because CSSParser is marking the start/ends as it sees commas: (CSSGrammar.y.in)

    at_rule_header_end: { parser->markRuleHeaderEnd(); } ;
    at_selector_end: { parser->markSelectorEnd(); } ;

    selector_list:
        ...
        | selector_list at_selector_end ',' maybe_space before_selector_group_item complex_selector %prec UNIMPORTANT_TOK {
        ...

So if we are doing a nested selector list, we get an errant early markSelectorEnd().

The markStart/End is only used by the Inspector here, so that we can send the exact text from the stylesheet to the frontend. We want this to show the case of the selector. ".testClass" instead of ".testclass". (InspectorStyleSheet.cpp)


    static PassRefPtr<Inspector::Protocol::Array<String>> selectorsFromSource(const CSSRuleSourceData* sourceData, const String& sheetText)
    {
        DEPRECATED_DEFINE_STATIC_LOCAL(JSC::Yarr::RegularExpression, comment, ("/\\*[^]*?\\*/", TextCaseSensitive, JSC::Yarr::MultilineEnabled));
        RefPtr<Inspector::Protocol::Array<String>> result = Inspector::Protocol::Array<String>::create();
>       const SelectorRangeList& ranges = sourceData->selectorRanges;
        for (size_t i = 0, size = ranges.size(); i < size; ++i) {
            const SourceRange& range = ranges.at(i);
            String selector = sheetText.substring(range.start, range.length());

            // We don't want to see any comments in the selector components, only the meaningful parts.
            replace(selector, comment, "");
            result->addItem(selector.stripWhiteSpace());
        }
        return result.release();
    }

    PassRefPtr<Inspector::Protocol::CSS::SelectorList> InspectorStyleSheet::buildObjectForSelectorList(CSSStyleRule* rule)
    {
        ...
>       if (sourceData)
>           selectors = selectorsFromSource(sourceData.get(), m_parsedStyleSheet->text());
>       else {
            selectors = Inspector::Protocol::Array<String>::create();
            const CSSSelectorList& selectorList = rule->styleRule()->selectorList();
            for (const CSSSelector* selector = selectorList.first(); selector; selector = CSSSelectorList::next(selector))
                selectors->addItem(selector->selectorText());
        }
        ...
    }

[Benjamin Poulain]

Created attachment 239260 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 239260 [details]
Patch

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

Looks good to me! May want another reviewer though.

> Source/WebCore/ChangeLog:10
> +        To make implement this with low risk of unbalanced start+end and good error recovery,

Grammaro: "To make implement" => "To implement"

[Joseph Pecoraro]

Comment on attachment 239260 [details]
Patch

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

> Source/WebCore/css/CSSParser.h:378
> +    unsigned m_nestedSelectorLevel;

Nit: You might be able to move this member up next to m_numParsedPropertiesBeforeMarginBox and it might save a few bytes in the size of CSSParser. Not important at all though.

[Benjamin Poulain]

Committed r174379: <http://trac.webkit.org/changeset/174379>