Bug 47971 - css combinator "+" in combination with NAV tag is buggy
Summary: css combinator "+" in combination with NAV tag is buggy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords:
: 62437 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-10-20 00:18 PDT by lukasz.wiacek
Modified: 2011-06-27 09:55 PDT (History)
7 users (show)

See Also:


Attachments
testcase (674 bytes, text/html)
2010-10-20 16:42 PDT, Philippe Wittenbergh
no flags Details
test-case-section-element (1.05 KB, text/html)
2011-03-20 22:50 PDT, noel gordon
no flags Details
test-case-nav-element.html (1009 bytes, text/html)
2011-03-20 22:51 PDT, noel gordon
no flags Details
Patch (5.47 KB, patch)
2011-06-09 07:26 PDT, Alexandru Chiculita
ojan: review-
ojan: commit-queue-
Details | Formatted Diff | Diff
Patch (5.11 KB, patch)
2011-06-22 00:47 PDT, Alexandru Chiculita
ojan: review+
ojan: commit-queue-
Details | Formatted Diff | Diff
Patch (5.15 KB, patch)
2011-06-23 00:47 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description lukasz.wiacek 2010-10-20 00:18:38 PDT
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.
Comment 1 Alexey Proskuryakov 2010-10-20 12:01:40 PDT
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?
Comment 2 Philippe Wittenbergh 2010-10-20 16:42:30 PDT
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;}
Comment 3 Philippe Wittenbergh 2010-10-20 16:47:19 PDT
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)
Comment 4 noel gordon 2011-03-20 22:50:18 PDT
Created attachment 86296 [details]
test-case-section-element
Comment 5 noel gordon 2011-03-20 22:51:10 PDT
Created attachment 86297 [details]
test-case-nav-element.html
Comment 6 noel gordon 2011-03-20 22:55:03 PDT
(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
Comment 7 Alexandru Chiculita 2011-06-09 05:26:16 PDT
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"
Comment 8 Alexandru Chiculita 2011-06-09 07:26:30 PDT
Created attachment 96584 [details]
Patch
Comment 9 noel gordon 2011-06-09 22:53:44 PDT
+ojan + mike.lawther since you two have recently modified the flex tokenizer.
Comment 10 noel gordon 2011-06-09 22:59:08 PDT
http://crbug.com/57981
Comment 11 Dominic Cooney 2011-06-12 19:50:08 PDT
*** Bug 62437 has been marked as a duplicate of this bug. ***
Comment 12 Mike Lawther 2011-06-20 23:54:03 PDT
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.
Comment 13 Ojan Vafai 2011-06-21 10:32:30 PDT
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?
Comment 14 Alexandru Chiculita 2011-06-22 00:47:00 PDT
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.
Comment 15 Ojan Vafai 2011-06-22 11:56:24 PDT
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.
Comment 16 Alexandru Chiculita 2011-06-23 00:47:25 PDT
Created attachment 98319 [details]
Patch

Added #foo +nav and #bar+ nav selectors in the testcase.
Removed try/catch.
Comment 17 WebKit Review Bot 2011-06-27 09:55:50 PDT
Comment on attachment 98319 [details]
Patch

Clearing flags on attachment: 98319

Committed r89826: <http://trac.webkit.org/changeset/89826>
Comment 18 WebKit Review Bot 2011-06-27 09:55:56 PDT
All reviewed patches have been landed.  Closing bug.