Bug 3317 – REGRESSION: CSS2: outline is applied to both <li> element and its enclosing text with context dependent selector

• Component: CSS
• Status: VERIFIED FIXED

[Hanspeter Schaub]

Starting with Safary 1.3/2.0, I found that the CSS outline command appears to be applied twice when used 
on a list element.  I'm using a hover command on the <ul> <li> element, and it used to cause the list 
entry to be highlighted with a outline when the mouse was over it.  Starting with 1.3/2.0, now the outline 
is still there, but there is a 2nd, inner outline right around the text entry itself?  

This bug still seems to be there in the latest build I tried today.

thanks,

Hanspeter Schaub

[Nicholas Shanks]

Could you please post a public URL or attach the "conferences.html" file to this bug - we are unable to see 
what's on your private iDisk !

[Chris Petersen]

I have reduced the page to a simple text case that reproduces the problem.

The test case contains a simple ordered list uses class='test':

<ol class="test">
<li>
OL element with a single child LI element 
</li> 
</ol>

A CSS rule is applied to the OL:

ol.test li:hover {
	outline: dashed;
	padding:0.5em;
}

The problem occurs when padding is specified (padding: 0.5em)

If you open the test case "outline_sample.html" and hover over the OL,  both the OL and LI appear with 
a dashed outline. If  padding is removed, this issue will no longer reproduce.

Under Firefox 1.0.4 , this test case is rendered with just the outline for the OL.

[Chris Petersen]

Created attachment 2167 [details]
Reduced test case

[Hanspeter Schaub]

Created attachment 2188 [details]
simple test case to see "outline" error with lists

[Hanspeter Schaub]

Created attachment 2463 [details]
proposed patch

I found that the code in renderflow.cpp was causing the erroneous outline to be
drawn on the list elements.  In this patch I simply commented out the old code.
 Please not, I'm not a professional programmer, but an engineer :-)  I found
this fix by searching through the code for anything related to drawing outlines
and testing those code sections.  

I did run the regression test and the modified code passes all the test.

[Hanspeter Schaub]

Created attachment 2464 [details]
list outline regression test

This is the regression test for the proposed patch.  It shows a simple
unordered list with the outline applied to the list element.  If the test is
successful, then a single outline should appear over the list element.	If the
browser fails, then two outlines are visible.

[David Kilzer (:ddkilzer)]

Created attachment 2474 [details]
Actual behavior

[David Kilzer (:ddkilzer)]

Created attachment 2475 [details]
Expected behavior

Expected behavior (seen after patch in Attachment 2463 [details] is applied).

[David Kilzer (:ddkilzer)]

After looking into this bug, it appears that the outline property is being applied to both the <li> element 
as well as its enclosing text.  Updated title to reflect this.

[David Kilzer (:ddkilzer)]

See also Bug 3611 for another outline property issue.

[Dave Hyatt]

Comment on attachment 2463 [details]
proposed patch

This patch is incorrect and will break all non-auto outline styles on inlines.

The real bug occurs in render_line.cpp around line 754.  Root lines should not
be adding their block objects to the outlineObjects dict.

[Hanspeter Schaub]

Created attachment 2559 [details]
fixes the issue where the outline is applied to the list text element

Following Dave Hyatt's suggestion, I added a check to render_line.cpp (line
754) to check that the object is not a root object.  The previously supplied
regression test case renders properly, and running run-webkit-tests passes all
previous tests.

[Dave Hyatt]

+ is not what you should set.  ? is how you ask  for review.  + means the patch received review.

[Darin Adler]

Comment on attachment 2559 [details]
fixes the issue where the outline is applied to the list text element

Dave Hyatt should be the one to review this.

[Dave Hyatt]

Comment on attachment 2559 [details]
fixes the issue where the outline is applied to the list text element

Style is wrong.  Make sure to remove the extra space between the two close
parentheses.

Other than that, r=me.

[Hanspeter Schaub]

Created attachment 2661 [details]
minor revision to patch, removed space between closing parentheses 

fixes a small style error in the earlier patch.  The space between the two
closing parentheses has been removed in this version.

[Dave Hyatt]

Comment on attachment 2661 [details]
minor revision to patch, removed space between closing parentheses 

r=me