Bug 3983 - outline:auto improperly puts outline around contained elements, not the actual div
Summary: outline:auto improperly puts outline around contained elements, not the actua...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-07-13 10:28 PDT by Dan Wood
Modified: 2005-12-10 11:55 PST (History)
1 user (show)

See Also:


Attachments
HTML file showing problem with outline auto (1.05 KB, text/html)
2005-07-13 10:31 PDT, Dan Wood
no flags Details
further reduced testcase (202 bytes, text/html)
2005-07-18 12:11 PDT, David Storey
no flags Details
Patch that seems to fix the two testcases (885 bytes, patch)
2005-12-01 07:56 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
reduced testcase for non-rectangular outlines (454 bytes, text/html)
2005-12-03 18:21 PST, Graham Dennis
no flags Details
patch (1.02 KB, patch)
2005-12-03 18:23 PST, Graham Dennis
hyatt: review-
Details | Formatted Diff | Diff
patch 2 (4.61 KB, patch)
2005-12-04 03:23 PST, Graham Dennis
hyatt: review-
Details | Formatted Diff | Diff
patch 3 (1.05 KB, patch)
2005-12-05 00:09 PST, Graham Dennis
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Wood 2005-07-13 10:28:53 PDT
Summary: When you set outline:auto on a div, it shows the "halo" outline around the elements *inside* 
the div, rather than the div itself

To reproduce: see attached test case.

Expected results: We should see a blue halo around the bottom-most rectangle, just like we see a solid 
blue halo around the div just above it.  We should not see the outline around the contained text or 
image.

Actual results: we see a halo around the text elements, and around the image (sometimes clipped), but 
not around the div itself.

Regression: Nothing formal here, but I recall on older webkits (pre-10.4.2), I couldn't get the halo to 
show up at all if the div wasn't "focused".
Comment 1 Dan Wood 2005-07-13 10:31:43 PDT
Created attachment 2945 [details]
HTML file showing problem with outline auto

Note: I'm now noticing that I am not seeing the "halo" outline around the
elements until you select the elements by dragging the mouse over them.
Comment 2 David Storey 2005-07-18 12:09:36 PDT
I'm using 10.3.9.  The outline shows as you state (it doesn't need to be in focus). However I'm not 
totally sure what correct behaviour is as no other browser I tested render it at all.  outline: auto is a css 
3 property so other browsers have not implemented it yet.  Opera and IE Mac show outlines for the css 
2 properties, but Gecko browsers do not show them at all.

The css spec states "in CSS3, 'outline-style' accepts the value 'auto'. The 'auto' value permits the user 
agent to render a custom outline style, typically a style which is either a user interface default for the 
platform, or perhaps a style that is richer than can be described in detail in CSS, e.g. a rounded edge 
outline with semi-translucent outer pixels that appears to glow. As such, this specification does not 
define how the 'outline-color' is incorporated or used (if at all) when rendering 'auto' style outlines. 
User agents may treat 'auto' as 'solid'."  

It seems to not state what the correct rendering is, but I'd expect that it should render the same as 
other styles, just in an OS X look and feel, instead of a flat colour for example.  So  suspect it is a bug.  
I'll attach a further reduced test case with just a div that has outline-style: auto; applied to it.
Comment 3 David Storey 2005-07-18 12:11:59 PDT
Created attachment 2995 [details]
further reduced testcase
Comment 4 Oliver Hunt 2005-07-21 17:36:12 PDT
First testcase is good, second testcase doesn't match firefox/mac behaviour at all (ffx gives no outline).  
Confirmed in current ToT
Comment 5 Kimmo Kinnunen 2005-12-01 07:56:03 PST
Created attachment 4894 [details]
Patch that seems to fix the two testcases

Here is a patch that draws the outlines in these two cases. How ever, since
it's so simple and reduces "functionality", I'm not sure at all that it will
fix the issue at hand fully and without side effects. However, somebody who
wants to actually fix the bug (or is interested in the possible bounty) can
perhaps make use of the information in this patch.
Comment 6 Dave Hyatt 2005-12-02 17:23:09 PST
The intent behind my implementation of focus ring outline was to encompass overflowed elements, 
producing irregular shapes that really fit around all content (even content that sticks out of the block).  
I think the real bug is in RenderFlow::addFocusRingRects, namely that the block's rect is never added to 
the set of rects as well.  You just need to call the base class if you're a block I think.

This completely bogus code:

if (element() && element()->isContentEditable()) {
        if (element()->parentNode() && !element()->parentNode()->isContentEditable() && !element()-
>hasTagName(bodyTag))
            p->addFocusRingRect(_tx, _ty, width(), height());
        return;
    }

should be removed as well.  I have no idea who put that there or why.  It makes no sense.

Comment 7 Graham Dennis 2005-12-03 18:21:01 PST
Created attachment 4922 [details]
reduced testcase for non-rectangular outlines

A further testcase to demonstrate the behaviour of an outline when it should be
non-rectangular. Currently, both 'auto' and styled outlines are rectangular.
Comment 8 Graham Dennis 2005-12-03 18:23:19 PST
Created attachment 4923 [details]
patch

This patch fixes addFocusRingRects to include the outermost rect in the focus
ring. This has the effect of causing 'auto'-styled outlines to be
non-rectangular, as in the previous testcase. However, styled outlines are
still rectangular, and follow the outer-most element.
Comment 9 Darin Adler 2005-12-03 18:49:45 PST
Comment on attachment 4923 [details]
patch

Dave should review this.
Comment 10 Dave Hyatt 2005-12-03 22:36:07 PST
Comment on attachment 4923 [details]
patch

You definitely only want to add in the focus ring rects for block flows and not
for inlines (RenderFlow is the base class of both).
Comment 11 Graham Dennis 2005-12-04 03:23:05 PST
Created attachment 4931 [details]
patch 2

Moved the addFocusRingRects function to RenderBlock.
Comment 12 Dave Hyatt 2005-12-04 22:23:52 PST
Comment on attachment 4931 [details]
patch 2

I just meant that the new code only needed to be done for RenderBlocks.  You
want to still call through to the base class for the rest of the code (which
should be in RenderFlow still and executed by both inlines and blocks).
Comment 13 Graham Dennis 2005-12-05 00:09:04 PST
Created attachment 4954 [details]
patch 3

Oops. I misunderstood. I added an if (isRenderBlock()) to patch 1.
Comment 14 Graham Dennis 2005-12-05 00:11:23 PST
Comment on attachment 4954 [details]
patch 3

I tried entering 'hyatt' as the requestee, but bugzilla just tells me that
'hyatt' doesn't match anything...
Comment 15 Darin Adler 2005-12-05 06:31:39 PST
Comment on attachment 4954 [details]
patch 3

Need to enter "hyatt@apple.com" as the requestee.
Comment 16 Dave Hyatt 2005-12-07 15:05:28 PST
Comment on attachment 4954 [details]
patch 3

r=me
Comment 17 Dave Hyatt 2005-12-07 15:08:39 PST
We should probably add some code to prevent the outlines from being drawn on focus for the body 
element.  This could be done in html4.css.

Instead of *:focus { outline ... }

I think:

:not(html):not(body):focus { outline ... }

would work.
Comment 18 Darin Adler 2005-12-10 11:17:49 PST
Dave, html4.css already has this:

    :focus { 
        outline: auto 3px #1f5ccf
    }

    html:focus, body:focus { 
        outline: none
    }