[Webkit-unassigned] [Bug 47971] css combinator "+" in combination with NAV tag is buggy

Tue Jun 21 10:32:31 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=47971

Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #96584|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |

--- Comment #13 from Ojan Vafai <ojan at chromium.org>  2011-06-21 10:32:30 PST ---
(From update of attachment 96584)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.