Summary: | REGRESSION: CSS2: outline is applied to both <li> element and its enclosing text with context dependent selector | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hanspeter Schaub <Hanspeterschaub> | ||||||||||||||||||
Component: | CSS | Assignee: | Dave Hyatt <hyatt> | ||||||||||||||||||
Status: | VERIFIED FIXED | ||||||||||||||||||||
Severity: | Normal | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 412 | ||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||||||
URL: | http://homepage.mac.com/hanspeterschaub/work/conferences.html | ||||||||||||||||||||
Attachments: |
|
Description
Hanspeter Schaub
2005-06-07 16:11:23 PDT
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 ! 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. Created attachment 2167 [details]
Reduced test case
Created attachment 2188 [details]
simple test case to see "outline" error with lists
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.
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.
Created attachment 2474 [details]
Actual behavior
Created attachment 2475 [details] Expected behavior Expected behavior (seen after patch in Attachment 2463 [details] is applied). 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. See also Bug 3611 for another outline property issue. 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.
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.
+ is not what you should set. ? is how you ask for review. + means the patch received review. 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.
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.
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.
Comment on attachment 2661 [details]
minor revision to patch, removed space between closing parentheses
r=me
|