Bug 47971 – css combinator "+" in combination with NAV tag is buggy

• Component: CSS
• Status: RESOLVED FIXED

when the white space between "+" and nav tag is removed on file compression selectors like "div+nav" stop working. while the uncompressed version "div + nav" works without problems.

[Alexey Proskuryakov]

Do you have a test case that you could attach to the bug, or a link to an actual Web site that's affected by this problem?

[Philippe Wittenbergh]

Created attachment 71360 [details]
testcase

(In reply to comment #1)
> Do you have a test case that you could attach to the bug, or a link to an actual Web site that's affected by this problem?

testcase: the nav element (blue text - 'a list of links') should also have a blue border. It does in Gecko 1.9.2 but not WebKit latest nightly and Safari 5.

nav {display: block;}
div + nav { color: blue; }
div+nav {border: 1px solid;}

[Philippe Wittenbergh]

This seems very specific to the '+' combinator and the html5 'nav' element.

div~nav {} works fine
div+footer {} works fine
(and repeat for other elements)

[noel gordon]

Created attachment 86296 [details]
test-case-section-element

[noel gordon]

Created attachment 86297 [details]
test-case-nav-element.html

[noel gordon]

(In reply to comment #3)
> This seems very specific to the '+' combinator and the html5 'nav' element.

Agree after testing webkit nightly

A + nav {}     OK
A+nav {}       FAIL

[Alexandru Chiculita]

The problem is in tokenizer.flex. There's a rule called "nth" that matches "+n"

nth             [\+-]?{intnum}*n([\t\r\n ]*[\+-][\t\r\n ]*{intnum})?

So the tokens for "#div+nav {}" are:
IDSEL   "div"
nth     "+n"
IDENT   "av"

[Alexandru Chiculita]

Created attachment 96584 [details]
Patch

[noel gordon]

+ojan + mike.lawther since you two have recently modified the flex tokenizer.

[noel gordon]

http://crbug.com/57981

[Dominic Cooney]

*** Bug 62437 has been marked as a duplicate of this bug. ***

[Mike Lawther]

Comment on attachment 96584 [details]
Patch

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

Hi Alex - thanks for the patch. I'm not a reviewer, but I've done a shadow review of the test case (mostly just nits). The actual fix in the flex looks OK to me.

> LayoutTests/fast/css/div_plus_nav_bug47971.html:5
> +<style type="text/css">

nit: no need for the type, it's assumed

> LayoutTests/fast/css/div_plus_nav_bug47971.html:6
> +#one + nav { color: green; }

Can you also put in a test for a case not involving the ID selector? eg "label+nav".

> LayoutTests/fast/css/div_plus_nav_bug47971.html:7
> + #two+nav { color: green; }

nit: extra space on the beginning of this line

> LayoutTests/fast/css/div_plus_nav_bug47971.html:9
> +<script>

nit: put this down in the <script> section below - no need for a separate one

> LayoutTests/fast/css/div_plus_nav_bug47971.html:16
> +<div>

Do you need this outer div?

> LayoutTests/fast/css/div_plus_nav_bug47971.html:17
> +    <div id="one">DIV1</div>

Nit: "one" and "two" ids, and DIV1 and DIV2 text is not needed. It distracts a bit when reading the test and looking at the results.

> LayoutTests/fast/css/div_plus_nav_bug47971.html:21
> +<div>

ditto: is this div needed?

> LayoutTests/fast/css/div_plus_nav_bug47971.html:26
> +<script type="text/javascript">

nit: no need for the type, it's assumed

> LayoutTests/fast/css/div_plus_nav_bug47971.html:27
> +function ArrayContains(array, value, ci)

You may not need this function - see below

> LayoutTests/fast/css/div_plus_nav_bug47971.html:29
> +  ci = ci == true ? true : false;

Should this line be here?

> LayoutTests/fast/css/div_plus_nav_bug47971.html:33
> +    {

No braces for single lines.

> LayoutTests/fast/css/div_plus_nav_bug47971.html:52
> +    if (ArrayContains(greenValues, val, false))

Can you lowercase val on line 51, and say here "if (greenValues.indexOf(val) != -1)" ?

> LayoutTests/fast/css/div_plus_nav_bug47971.html:57
> +  }catch(e){}

I don't think you want to swallow exceptions in test code like this.

> Source/WebCore/ChangeLog:10
> +        like "#div+nav" as: "#div" "+n" "av".

Is the id selector necessary? The bug only mentions "div+nav", but the id selector doesn't seem to be involved.

[Ojan Vafai]

Comment on attachment 96584 [details]
Patch

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

The code change looks good to me. Please address Mike's and my comments on the test then post a new patch for review.

> LayoutTests/fast/css/div_plus_nav_bug47971.html:1
> +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Strict//EN">

These days we usually use the HTML5 doctype in tests.
<!DOCTYPE HTML>

> LayoutTests/fast/css/div_plus_nav_bug47971.html:4
> +<html>
> +<head>
> +<title>CSS Test: #div+nav</title>

No need to include these elements. The more sparse the test, the easier it is to see what it's testing.

>> LayoutTests/fast/css/div_plus_nav_bug47971.html:17
>> +    <div id="one">DIV1</div>
> 
> Nit: "one" and "two" ids, and DIV1 and DIV2 text is not needed. It distracts a bit when reading the test and looking at the results.

If you're doing this just so that the div will be visible, you could give the div a width/height with CSS.

> LayoutTests/fast/css/div_plus_nav_bug47971.html:44
> +function TestCase(element, resultElement)

This isn't really a class, but more of a method. Other tests will typically call this "runTest".

>> LayoutTests/fast/css/div_plus_nav_bug47971.html:52
>> +    if (ArrayContains(greenValues, val, false))
> 
> Can you lowercase val on line 51, and say here "if (greenValues.indexOf(val) != -1)" ?

More importantly, why do you need to compare to a list of values? Won't the results of getComputedStyle be the same string for all of these cases?

[Alexandru Chiculita]

Created attachment 98129 [details]
Patch

Thank you for reviewing the patch. I've simplified the test and addressed all the issues.

- You're right about #div+nav. It doesn't matter what's before "+nav" in the selector. Just to make sure, I've also added another test for label+nav. 
- I've kept "one" and "two" IDs, because I need them in the selectors.
- I kept only two values in greenValues array. It looks like most of the latest browsers return "rgb(0, 128, 0)" while Opera returns "#008000". It is easier to compare the results with other browsers this way.

[Ojan Vafai]

Comment on attachment 98129 [details]
Patch

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

Patch looks good. Please consider addressing my two comments before committing (or if you're not a committer, you can post a new patch and I can cq+ it for you).

> LayoutTests/fast/css/div_plus_nav_bug47971.html:6
> +#one + nav { color: green; }
> +#two+nav { color: green; }
> +label+nav { color: green; }

For thoroughness sake, maybe add the following cases as well:
#foo +nav
#bar+ nav

> LayoutTests/fast/css/div_plus_nav_bug47971.html:42
> +    } catch(e) {
> +        document.writeln("Fail: " + element + ": Exception \"" + e + "\".<br />");
> +    }

When would you ever hit this exception? Do you need the try/catch at all? It's fine for the test to throw an error if there is a JS error. It's worth having less extra cruft in the test.

[Alexandru Chiculita]

Created attachment 98319 [details]
Patch

Added #foo +nav and #bar+ nav selectors in the testcase.
Removed try/catch.

[WebKit Review Bot]

Comment on attachment 98319 [details]
Patch

Clearing flags on attachment: 98319

Committed r89826: <http://trac.webkit.org/changeset/89826>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.