Bug 137418 – Implement Element.closest() API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dhi Aurrahman]

Implement closest() API

[Dhi Aurrahman]

Created attachment 239265 [details]
Patch

[Darin Adler]

Comment on attachment 239265 [details]
Patch

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

review- because of the bad cast

> Source/WebCore/dom/Element.cpp:2329
> +    SelectorQuery* selectorQuery = document().selectorQueryForString(selector, ec);
> +    if (selectorQuery)
> +        return selectorQuery->closest(*this);
> +    return nullptr;

WebKit coding style prefers early return, meaning:

    if (!selectorQuery)
        return nullptr;

> Source/WebCore/dom/SelectorQuery.cpp:128
> +    Element* currentNode = (Element *) &rootNode;

The formatting here isn’t right, but more importantly this is a bad cast if the root node is a document node. That needs to be fixed.

> Source/WebCore/dom/SelectorQuery.cpp:151
> +    unsigned selectorCount = m_selectors.size();
> +    for (unsigned i = 0; i < selectorCount; ++i) {
> +        Element* closestElement = selectorClosest(m_selectors[i], targetElement, targetElement);

This should use a modern for loop:

    for (auto& selector : m_selectors) {
        if (Element* closestElement = selectorClosest(selector, targetElement, targetElement))
            return closestElement;
    }

[Benjamin Poulain]

Comment on attachment 239265 [details]
Patch

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

The patch is in good shape. There is a bug not mentioned by Darin or I but I would prefer you to discover it through tests. Good test coverage is extremely important to validate a feature.

> LayoutTests/fast/selectors/closest-general.html:43
> +shouldBe('theTarget.closest("#theTarget")', "theTarget");
> +shouldBe('theTarget.closest("ancestor")', "ancestor");
> +shouldBeNull('theTarget.closest("hihi")', "ancestor");

You need a few more test cases to be sure everything works properly.

First, you need to test all the combinators:
    a b
    a+b
    a>b
    a~b
Ideally, you should test a use case that finds an ancestor, and one that does not.

The other kind of use case you need to test is coma separated selectors.
For example:
    a, b
    b, a

Finally, it would be good to have negative and positive cases for the big kind of matching: 
    #id
    .class
    [attribute]
    tagname

> LayoutTests/fast/selectors/closest-general.html:50
> +shouldBeTrue('failOnEmptyArguments()');
> +shouldBeTrue('failOnEmptyStringArgument()');

You should use shouldThrow() to test the exceptions.

There are other cases to cover here, invalid selectors for example ".123", "    ", ")", "()", "}", etc, etc.

[Dhi Aurrahman]

Created attachment 239279 [details]
Patch

[Benjamin Poulain]

Comment on attachment 239279 [details]
Patch

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

The tests are great.

You should look into one more improvement to your algorithm:

Currently, the algorithm has to evaluate each selectors for all ancestors before knowing which one matches.

For example, given a selector "foo, bar", if "foo" is not one of the ancestor, all the parents need to be evaluated for "foo" before looking for "bar".

What you could do instead is go up the parents and test for both "foo" and "bar" at each step.

> Source/WebCore/ChangeLog:5
> +

In the changelog, authors generally explain their change. (see the other changelogs for an example).

Here your patch is pretty self explanatory so you do not necessarily need to explain it in the changelog.
It would be good to add a link to the specification you are implementing though.

> Source/WebCore/dom/Element.cpp:2328
> +    SelectorQuery* selectorQuery = document().selectorQueryForString(selector, ec);
> +    if (selectorQuery)
> +        return selectorQuery->closest(*this);

You can scope this one in the if()
    if (SelectorQuery* selectorQuery = document().selectorQueryForString(selector, ec))
        return selectorQuery->closest(*this);

> LayoutTests/ChangeLog:9
> +        * fast/selectors/closest-general-expected.txt: Added.
> +        * fast/selectors/closest-general.html: Added.

The new tests are great, very wide coverage.

> LayoutTests/fast/selectors/closest-general.html:21
> +	    </ancestor>

The indentation is wrong here.

> LayoutTests/fast/selectors/closest-general.html:116
> +shouldThrow('theTarget.closest("^_^")');

:)

[Dhi Aurrahman]

Created attachment 239285 [details]
Patch

[Benjamin Poulain]

Comment on attachment 239285 [details]
Patch

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

> Source/WebCore/dom/SelectorQuery.cpp:149
> +            Element* candidateElement = selectorClosest(selector, *currentNode, *currentNode);

Shouldn't the scope be targetElement instead of currentNode?

> LayoutTests/fast/selectors/closest-scope.html:24
> +shouldBe('theTarget.closest("body>:scope")', 'theTarget');

Let's add a test for the :scope bug.

theTarget.closest('body:scope') should be null.

[Dhi Aurrahman]

Created attachment 239286 [details]
Patch

[Benjamin Poulain]

Comment on attachment 239286 [details]
Patch

lgtm

[WebKit Commit Bot]

Comment on attachment 239286 [details]
Patch

Clearing flags on attachment: 239286

Committed r174324: <http://trac.webkit.org/changeset/174324>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.