Bug 137378 - Web Inspector: Highlighted selectors in Rules sidebar break with selectors that contain nested selector lists
Summary: Web Inspector: Highlighted selectors in Rules sidebar break with selectors th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-10-02 20:04 PDT by Joseph Pecoraro
Modified: 2014-10-06 18:55 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.58 KB, patch)
2014-10-03 20:16 PDT, Benjamin Poulain
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-10-02 20:04:06 PDT
* 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"
Comment 1 Radar WebKit Bug Importer 2014-10-02 20:04:29 PDT
<rdar://problem/18535308>
Comment 2 Joseph Pecoraro 2014-10-02 20:09:39 PDT
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());
        }
        ...
    }
Comment 3 Benjamin Poulain 2014-10-03 20:16:45 PDT
Created attachment 239260 [details]
Patch
Comment 4 Joseph Pecoraro 2014-10-06 11:38:15 PDT
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"
Comment 5 Joseph Pecoraro 2014-10-06 15:19:13 PDT
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.
Comment 6 Benjamin Poulain 2014-10-06 18:55:25 PDT
Committed r174379: <http://trac.webkit.org/changeset/174379>