Closed Bug 151375 Opened 22 years ago Closed 20 years ago

focus outline should be drawn outside of element

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jruderman, Assigned: aaronlev)

References

(Blocks 2 open bugs)

Details

(Keywords: access, css2, testcase)

Attachments

(7 files, 12 obsolete files)

testcase
1.08 KB, text/html
Details
More test cases with img and block frames
4.41 KB, text/html
Details
Clipping/overflow test cases
981 bytes, text/html
Details
Screenshot - poor results without views
3.98 KB, image/gif
Details
there
56.33 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
Patch with simple FinishAndStoreOverflow() inline wrapper
41.52 KB, patch
Details | Diff | Splinter Review
additional fix
12.94 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review 
Focus outlines are currently drawn inside spans, images, etc.  They should be
drawn outside so that thick outlines don't cover what they're trying to outline.
 This would be useful for accessibility, since keyboard users and users with
poor sight may prefer to use thicker outlines.

The CSS2 spec says "The outline is drawn starting just outside the border edge"
(http://www.w3.org/TR/REC-CSS2/ui.html#dynamic-outlines).  If we followed the
spec in cases where there is no border, our focus outlines would move out by one
pixel.  This would make Mozilla's focus outlines around text links surround a
slightly larger area than IE's and Netscape 4's.  It would make outlines around
image links more visible and keep them from obstructing the image, allowing them
to be thicker.

Kevin McCluskey said in bug 6647 that making outlines draw in the correct place
is hard: "To implement we would need to be able to draw to the outside of the
frame. This would require some major modifications."
Blocks: css2outline
Keywords: access, css2
Attached file testcase — 

    
    

    
  
Should be All/All, shouldn't it?
Keywords: testcase
OS: Windows 2000 → All
Hardware: PC → All

    
  
Blocks: 92286

    
    

    
  
I think this could be done if the focus outline was done like a caret, which
avoids layout turds somehow.

See also bug 53927 - Focus outlines should look like Mac focus outlines on Mac
OS [outline]



    
    

    
  
This could be done without too many major modifications by storing additional
outline information out-of-band, in a hashtable.  That would work fine for the
normal uses of outline, where you only have one or two per page at a time.

View sizing might be fun, though.

    
    

    
  
David, any idea if you will get to this?

If not, can you explain a little more how it should be approached, so that I
might take a stab at it?

I was thinking it might be done like the caret, since there should only be 1 at
a time, and it would need to listen for certain events to know if it needed to
redraw itself.

What exactly do you mean by
> storing additional outline information out-of-band, in a hashtable. 

and
> View sizing might be fun, though.
Blocks: 53927
Keywords: nsbeta1

    
    

    
  
Unlikely to happen before February.

You can't assume that there will be only one, but you can maintain a hashtable
on the assumption that there will generally be few, and it might be slow if
there are many.  That's basically what I was saying.

    
    

    
  
Why would there be more than one focus outline? There should only be only 1 page
element focused at a time. If there is more than one, that's a problem.

It's possible that the current HTML frame/iframe might show focus at the same
time as a page element, but I think that would be drawn inside the frame anyway,
as it currently is.

    
    

    
  
We're talking about the implementation of the CSS 'outline' property, which can
be applied to any element.

    
    

    
  
Taking. This is really a duplicate of bug 9809, I think.
Assignee: dbaron → roc+moz

    
    

    
  
aaronl: what dbaron said. You can set 'outline' on any and as many elements as
you like, using a stylesheet or a script or whatever.

Also, caret is broken in many ways because it does not use the normal drawing
path. We do not want to reproduce that. (IMHO we should fix that about the
caret, e.g. by moving caret drawing to nsBlockFrame, and using normal
invalidate/paint to make it blink. This is necessary to make caret obey all the
CSS2 formatting like z-index, clipping, etc.)

    
  
Keywords: topembed

    
    

    
  
I think this is a major problem with our rendering.

In usability tests people could not find the focus after find as you type.

    
  
Keywords: topembed → topembed+

    
    

    
  
adt: nsbeta1-
Keywords: nsbeta1 → nsbeta1-

    
    

    
  
*** Bug 116289 has been marked as a duplicate of this bug. ***

    
  
Keywords: nsbeta1-, 
          
            topembed+

    
    

    
  
Roc. this is still assigned to you. Any chance it can get back on your radar?
If not, assign it to me. 

In terms of accessibility and usability, I think this is actually pretty big.
It's nearly impossible to see focus outlines unless you alreayd have some idea
where it might be.

    
  
No longer blocks: css2outline
Depends on: css2outline

    
    

    
  
Here's what has to be done. My strategy is to make sure that every frame with
non-trivial 'outline' has a view.
-- Make sure that the outline area (if any) is added to the overflowArea of
every frame. GetOutlineRect would help with that.
-- Make sure (via FrameNeedsView and appropriate style hints for outline
property changes) that every frame with an outline has a view. This would also
be helped by changing the style change code so that if a view is needed, we
don't reframe, just call CreateViewForFrame on the frame (which works now for
frames that already have children).
-- Modify Paint and HandleEvent in nsPresShell so that for the frame with the
view, they check the outline area for events and painting.
-- Change the actual outline rendering code to render outside the frame.

    
    

    
  
(In reply to comment #15)
> -- Change the actual outline rendering code to render outside the frame.
This is the only part I know how to do:
Index: nsFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/base/src/nsFrame.cpp,v
retrieving revision 3.488
diff -u -r3.488 nsFrame.cpp
--- nsFrame.cpp 17 Jun 2004 00:12:51 -0000      3.488
+++ nsFrame.cpp 28 Jun 2004 03:04:44 -0000
@@ -917,9 +917,13 @@
   nsCSSRendering::PaintBorder(aPresContext, aRenderingContext, this,
                               aDirtyRect, rect, *border, mStyleContext,
                               aSkipSides);
-  nsCSSRendering::PaintOutline(aPresContext, aRenderingContext, this,
-                               aDirtyRect, rect, *border, *outline,
-                               mStyleContext, 0);
+  PRBool hasOutline;
+  rect = GetOutlineRect(&hasOutline);
+  if (hasOutline) {
+    nsCSSRendering::PaintOutline(aPresContext, aRenderingContext, this,
+                                aDirtyRect, rect, *border, *outline,
+                                mStyleContext, 0);
+  }
 }

 /**


I would understand if you didn't feel like actually implementing your planned
fix, but would be extremely glad if you did.

    
    

    
  
It's just not near the top of my priority list right now.

    
    

    
  
I think I'll give this outline fix a try -- can I get help with some questions.
Wish I understood this part of layout a little better.


(In reply to comment #15)
> Here's what has to be done. My strategy is to make sure that every frame with
> non-trivial 'outline' has a view.
* What do you mean non-trivial. Aren't they always outside of the current frame?
* The view does not need a widget, correct?

> -- Make sure that the outline area (if any) is added to the overflowArea of
> every frame. GetOutlineRect would help with that.
* This step seems like the hardest.
* The frame state should include NS_FRAME_OUTSIDE_CHILDREN, correct?
* In what method should add I the outline rect to the overflow area?
* Between GetOverflowAreaProperty(), StoreOverflow(), ConsiderChildOverflow()
etc. I'm not sure what I need to be doing. It looks a little mysterioius to me
still.

> -- Make sure (via FrameNeedsView and appropriate style hints for outline
> property changes) that every frame with an outline has a view. This would also
> be helped by changing the style change code so that if a view is needed, we
> don't reframe, just call CreateViewForFrame on the frame (which works now for
> frames that already have children).
* What do you mean by appropriate style hints?
* How does CreateViewForFrame() avoid a reframe? Because views automatically get
updated by the view manager? Isn't that something that's generally useful for
layout anyway -- should it be a separate bug?

> -- Modify Paint and HandleEvent in nsPresShell so that for the frame with the
> view, they check the outline area for events and painting.
* Do we really want to mouse events on the outline as mouse events in the frame?
* What other events besides mouse events need to be sent to the frame 
* How do we fix nsPresShell::Paint() ? I'm shooting in the dark here, something
lke this?

    // If the frame is absolutely positioned, then the 'clip' property
    // applies
+      PRBool setClipRect, = PR_FALSE, hasOutline = PR_FALSE;
+       if  ((GetStateBits() & (NS_FRAME_HAS_VIEW | NS_FRAME_OUTSIDE_CHILDREN)) == 
+    ( NS_FRAME_HAS_VIEW | NS_FRAME_OUTSIDE_CHILDREN) && 
+   frame->GetOutlineRect(&hasOutline);
+  }
+  if (!hasOutline) {
+      setClipRect = SetClipRect(aRenderingContext, frame);
+    }

-   PRBool setClipRect = SetClipRect(aRenderingContext, frame);
    rv = frame->Paint(mPresContext, aRenderingContext, aDirtyRect,
                      NS_FRAME_PAINT_LAYER_BACKGROUND);
    rv = frame->Paint(mPresContext, aRenderingContext, aDirtyRect,
                      NS_FRAME_PAINT_LAYER_FLOATS);
    rv = frame->Paint(mPresContext, aRenderingContext, aDirtyRect,
                      NS_FRAME_PAINT_LAYER_FOREGROUND);
                      
    if (setClipRect)
      aRenderingContext.PopState();


> -- Change the actual outline rendering code to render outside the frame.
* This part is pretty easy


    
    

    
  
(In reply to comment #18)
> I think I'll give this outline fix a try -- can I get help with some
> questions. Wish I understood this part of layout a little better.
> 
> > (In reply to comment #15)
> > Here's what has to be done. My strategy is to make sure that every frame
> > with non-trivial 'outline' has a view.
> * What do you mean non-trivial. Aren't they always outside of the current
> frame?

Yeah. By "non-trivial" I just meant "visible".

> * The view does not need a widget, correct?

Correct.

> > -- Make sure that the outline area (if any) is added to the overflowArea of
> > every frame. GetOutlineRect would help with that.
> * This step seems like the hardest.
> * The frame state should include NS_FRAME_OUTSIDE_CHILDREN, correct?

Yes.

> * In what method should add I the outline rect to the overflow area?

I would add it to nsFrame::StoreOverflow which gets called by most frames to
store the final overflow area. You may want to change the name to indicate that
you're going to compute the final overflow area too --- say,
FinishAndStoreOverflow. However, there are a few kinds of frames that don't do
StoreOverflow. Inline frames, for example. Looking at this query
http://lxr.mozilla.org/mozilla/search?string=NS_FRAME_OUTSIDE_CHILDREN
it looks like the only frames that set NS_FRAME_OUTSIDE_CHILDREN without going
through StoreOverflow are nsInlineFrame, nsPositionedInlineFrame,
nsFieldSetFrame, nsMathMLTokenFrame, and nsMathMLmpaddedFrame. You want to
convert those to use FinishAndStoreOverflow.

> > -- Make sure (via FrameNeedsView and appropriate style hints for outline
> > property changes) that every frame with an outline has a view. This would
> > also be helped by changing the style change code so that if a view is
> > needed, we don't reframe, just call CreateViewForFrame on the frame (which
> > works now for frames that already have children).
> * What do you mean by appropriate style hints?

In the style system, changes to the outline property that could make the outline
visible should return an nsChangeHint that creates a view for the frame. The
easiest thing for you to do right now is to just return nsChangeHint_Reframe.

> * How does CreateViewForFrame() avoid a reframe? Because views automatically
> get updated by the view manager? Isn't that something that's generally useful
> for layout anyway -- should it be a separate bug?

Yes. You can assign that part to me. For now, just use _Reframe to recreate the
frames. You may find that the frame recreation creates bugs with some kinds of
frames, but for regular blocks and inlines and so on it should work fine.

> > -- Modify Paint and HandleEvent in nsPresShell so that for the frame with
> > the view, they check the outline area for events and painting.
> * Do we really want to mouse events on the outline as mouse events in the
> frame?

I don't know. Up to you. I can see both sides of that argument.

> * What other events besides mouse events need to be sent to the frame

Only mouse events depend on the position, so if you decide that mouse events can
ignore the outline, you don't need to worry about events.

> * How do we fix nsPresShell::Paint() ? I'm shooting in the dark here,
> something like this?
> 
>     // If the frame is absolutely positioned, then the 'clip' property
>     // applies
> +      PRBool setClipRect, = PR_FALSE, hasOutline = PR_FALSE;
> +       if  ((GetStateBits() & (NS_FRAME_HAS_VIEW |
NS_FRAME_OUTSIDE_CHILDREN)) == 
> +    ( NS_FRAME_HAS_VIEW | NS_FRAME_OUTSIDE_CHILDREN) && 
> +   frame->GetOutlineRect(&hasOutline);
> +  }
> +  if (!hasOutline) {
> +      setClipRect = SetClipRect(aRenderingContext, frame);
> +    }
> 
> -   PRBool setClipRect = SetClipRect(aRenderingContext, frame);
>     rv = frame->Paint(mPresContext, aRenderingContext, aDirtyRect,
>                       NS_FRAME_PAINT_LAYER_BACKGROUND);
>     rv = frame->Paint(mPresContext, aRenderingContext, aDirtyRect,
>                       NS_FRAME_PAINT_LAYER_FLOATS);
>     rv = frame->Paint(mPresContext, aRenderingContext, aDirtyRect,
>                       NS_FRAME_PAINT_LAYER_FOREGROUND);
>                       
>     if (setClipRect)
>       aRenderingContext.PopState();

That's not correct because it means frames with 'outline' and clipping won't
clip at all. However, it's good enough to get you started. Just put an XXX in
there so that we remember to fix it before you check in :-)

    
    

    
  


  
Known flaws:
- Focus a link (such as "Groups" on www.google.com) and use the mouse to select
through the outline, the left and right lines dissapear. You can select in this
way if you drag from the blank space on the left of the link to the right.
- Dynamic changes not working perfectly. See testcase
  http://bugzilla.mozilla.org/attachment.cgi?id=151835&action=view

    
  
Assignee: roc → aaronleventhal
Status: NEW → ASSIGNED

    
    

    
  
+    clipRect.UnionRect(aFrame->GetOutlineRect(), clipRect);

This still isn't right. Please put in an XXX here so we don't forget to figure
out how this should work.

    
    

    
  


  

        Attachment #151892 -
        Attachment is obsolete: true

    
    

    
  
+  nscoord oldWidth = 0, newWidth = 0;
+  GetOutlineWidth(oldWidth);
+  aOther.GetOutlineWidth(newWidth);

Just access mOutlineWidth directly.

+      return NS_CombineHint(nsChangeHint_SyncFrameView, nsChangeHint_RepaintFrame);

Should be nsChangeHint_Reflow, not SyncFrameView

@@ -627,6 +627,13 @@
       }
     }
   }
+
+  PRBool hasOutline;
+  nsRect outlineRect = aFrame->GetOutlineRect(&hasOutline);
+  if (hasOutline) {
+   vm->ResizeView(aView, outlineRect, PR_TRUE);
+  }
+

Why is this necessary?

-      nsSize frameSize = aFrame->GetSize();
-      nsRect newSize(0, 0, frameSize.width, frameSize.height);
-      vm->ResizeView(aView, newSize, PR_TRUE);
+      vm->ResizeView(aView, aFrame->GetOutlineRect(), PR_TRUE);

Why is this necessary? if the outline is there then NS_FRAME_OUTSIDE_CHILDREN
should be set, right?

+  // XXX Is this necessary, since we've stored the outline in the metrics
+  // using FinishAndStoreOverflow() ?

I think it is necessary. Otherwise a change in the size of the frame might not
repaint the necessary outline area.

-  // repaint this frame's outline area.
-  // In CSS3 selection can change the outline style! and border and content too
-  Invalidate(GetOutlineRect(), PR_FALSE);
-

Why did you remove this?

Other than that, this looks good.

Before checking in, we need to figure out what to do with the clip situation. It
would also be good to test our XUL UI to see if changing -moz-outline does
horrible things to themes and stuff.

    
    

    
  


  

    
    

    
  


  

        Attachment #151966 -
        Attachment is obsolete: true

    
  

        Attachment #151991 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 151991 [details] [diff] [review]
Addresses comments, fixes issues with clipping and overflow

Funny thing about this patch ... it works just as well if I don't make any
changes to FinishAndStoreOverflow(), which must be because the view resizing
code I added. It would be a lot smaller patch to just return this to
StoreOverflow() and remove all of those changes. What is the
NS_FRAME_OUTSIDE_CHILDREN really getting us? It wasn't working to resize the
view on it's own.

    
    

    
  
+      nsFrameManager *frameMgr = GetPresContext()->FrameManager();
+      nsRect *overflowRect =
+        (nsRect*)frameMgr->GetFrameProperty(this, 
+                                            nsLayoutAtoms::overflowAreaProperty,
+                                            0);
+      if (overflowRect) {
+        r.UnionRect(r, *overflowRect);
+      }
+

Where did this come from? This can't be right. You're inflating the overflow
area by the outline width as if the outline was painted on the overflow area,
which it isn't.

+
+  // We need to ensure that the view is big enough for the outline.
+  // The outline may dynamically increase in size, and be larger than the
+  // original view created for it.
+  PRBool hasOutline;
+  nsRect outlineRect = aFrame->GetOutlineRect(&hasOutline);
+  if (hasOutline) {
+    vm->ResizeView(aView, outlineRect, PR_TRUE);
+  }
+

Whatever bug this was supposed to fix, this is not the right fix. The view
includes the frame's overflowing children, and you're just wiping it out with
the outline rect. The changes to StoreOverflow() should have made anything like
this unnecessary by computing the correct view bounds in the first place.

Now that I think about it, I guess we don't need to change clipping at all. The
current code will clip the outline which is what CSS 2.1 wants us to do.

I will build with your patch and do some tests.

    
    

    
  
I discovered one nasty problem: with 'outline', you can now have XUL frames that
overflow. XUL never handled this before.

    
    

    
  
Trying to make this work on XUL makes XUL listboxes overflow and do horrible things.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        work in progress (obsolete)
        — 
          — Splinter Review
      

    


  
Here's a snapshot of where I'm at with my extension of the patch. Apart from
trying to address XUL, I changed the way rendering is done so that PaintOutline
just does the right thing internally when passed the border-box. This was
necessary to fix the rendering of elements with outline and borders, as in
Jesse's original testcase. I also removed the prescontext parameter from
FinishAndStoreOverflow and GetOverflowAreaProperty, and modified
FinishAndStoreOverflow so it doesn't need an nsHTMLReflowMetrics, which is not
easily available in the XUL context.

    
    

    
  
The nsPrivateTextRange changes appear to be whitespace-only and unrelated.

    
  

        Attachment #151991 -
        Flags: review?(roc)

    
    

    
  
This really depends on fixing bug 53966. Currently once a frame has an outline,
if you start a selection inside it you can't select outside the frame because
the frame is capturing the mouse.

It also depends on fixing bug 78087. Frames with views will always appear on top
of sibling frames that don't have views, and that will sometimes be incorrect,
especially for frames with outlines that are likely to be inline.

Bug 249102 is relevant but not necessarily a dependency. Things *should* work OK
if you reconstruct the frames when an element first gets an outline, but in some
cases they probably won't, and a fix for 249102 would prevent such regressions
(and also improve performance a little bit).
Depends on: 53966, 78087

    
    

    
  
Thanks for taking this over. The current patch doesn't apply to the trunk. It
has conflicts in nsStyleStruct.cpp and nsTableRowGroupFrame.cpp.

What are the changes to nsGfxScrollFrame.cpp for?

    
    

    
  
Sorry, some bits of other patches crept in there. I'll attach a new patch.

I'm not taking this over, just kicking it along a bit. If you want to try to
narrow down some of the remaining bugs, please do :-)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated patch (obsolete)
        — 
          — Splinter Review
      

    


  
Here's the correct patch which leaves out the bogus changes in PrivateTextRange
and nsGfxScrollFrame. I don't know where your conflict in nsStyleStruct.cpp is
coming from.

    
    

    
  
Comment on attachment 152013 [details] [diff] [review]
updated patch

Applied patch this to an updated trunk. Images with outlines would not display.
Dynamic changes did not work. Scrollbars fail to function in HTML documents.

    
    

    
  
(In reply to comment #28)
> +      nsFrameManager *frameMgr = GetPresContext()->FrameManager();
> +      nsRect *overflowRect =
> +        (nsRect*)frameMgr->GetFrameProperty(this, 
> +                                            nsLayoutAtoms::overflowAreaProperty,
> +                                            0);
> +      if (overflowRect) {
> +        r.UnionRect(r, *overflowRect);
> +      }
> +
> 
> Where did this come from? This can't be right. You're inflating the overflow
> area by the outline width as if the outline was painted on the overflow area,
> which it isn't.
No, I think I'm just inflating the outline area to include the overflow text. Is
that not correct? It looks a lot better that way. See the last testcase.

> 
> +
> +  // We need to ensure that the view is big enough for the outline.
> +  // The outline may dynamically increase in size, and be larger than the
> +  // original view created for it.
> +  PRBool hasOutline;
> +  nsRect outlineRect = aFrame->GetOutlineRect(&hasOutline);
> +  if (hasOutline) {
> +    vm->ResizeView(aView, outlineRect, PR_TRUE);
> +  }
> +
> 
> Whatever bug this was supposed to fix, this is not the right fix. The view
> includes the frame's overflowing children, and you're just wiping it out with
> the outline rect. 
The code snippet above takes care of that. It makes sure that when the view gets
resized it includes the overflow area. The outline appears around the text and
the overflow put together. It just looks better. I wouldn't want a focus outline
to cut through the overflow text. It should go around the whole piece of text.
Is that actually specified somewhere to not do that?

> The changes to StoreOverflow() should have made anything like
> this unnecessary by computing the correct view bounds in the first place.
I must be missing something. How is StoreOverflow() supposed get used to compute
the initial view bounds? 
The view bounds are originally initialized in nsHTMLContainerFrame.cpp with this:
>  // Initialize the view
>  view->Init(viewManager, aFrame->GetRect(), parentView);
At that time, GetRect() returns 0,0 for the size of the frame. I guess it hasn't
done the initial reflow yet and doesn't know it's size?
So, this doe fixes the initial view size plus those itmes when the outline
dynamically increases in size. 



    
    

    
  
> No, I think I'm just inflating the outline area to include the overflow text.
> Is that not correct? It looks a lot better that way. See the last testcase.

It may look better in some cases but it definitely violates CSS 2.1.

> Is that actually specified somewhere to not do that?

Yes. CSS 2.1 is completely clear that the outline is drawn outside and adjacent
to the border-box.

> How is StoreOverflow() supposed get used to compute the initial view bounds?

What usually happens is that Reflow calls FinishAndStoreOverflow passing in the
nsHTMLReflowMetrics::mOverflowArea. FinishAndStoreOverflow updates the
mOverflowArea by adding in the outline rect. Reflow then returns the
nsHTMLReflowMetrics to its caller. The caller then resizes the frame and calls
SyncFrameViewAfterReflow, passing in the mOverflowArea. SyncFrameViewAfterReflow
sizes the frame's view to the overflow area. Get it?

    
    

    
  
Actually it appears to me that we don't need to create a view for outlines. We
might need to make a few small changes to painting and event handling but it
should just work once we've set NS_FRAME_OUTSIDE_CHILDREN and the overflow area.
That would break the dependency on bugs 53966, 78087 and 249102 and also quash
any worries about performance creating views for lots of frames. (I actually
have a fix in hand for 53966 but 78087 is a lot more work.)

    
    

    
  
(In reply to comment #40)
> Actually it appears to me that we don't need to create a view for outlines.
By now I've figured out most of the problems with the current patch.
FinishAndStoreOverflow() was using GetOutlineRect which used mRect and that
wasn't calculated yet.

Would it be so bad to check this in using views?


> We might need to make a few small changes to painting and event handling but 
> it should just work once we've set NS_FRAME_OUTSIDE_CHILDREN and the overflow 
> area.
If it's necessary for me to change the way it works now, so be it. Can you
explain what I have to do a little more? Actually, I'm still not completely
clear on when we use views in Gecko, because I'm not exactly 100% what a view
is. When do we need a view, and we do we need a widget? And why do we need both
views and view managers?

One thing I've found with the current patch is that it doesn't handle when the
frame resizes -- for example when an image finally gets loaded the view needs to
get resized. I would need to change the change hint for resizes when there's an
outline. Where do I do that?

    
    

    
  
The changes in your patch in layout/xul/base/src were what broke scrolling in HTML.

    
    

    
  


  

        Attachment #152013 -
        Attachment is obsolete: true

    
    

    
  
> FinishAndStoreOverflow() was using GetOutlineRect which used mRect and that
> wasn't calculated yet.

Where was that?

> Would it be so bad to check this in using views?

Yes, I think we're likely to create quite visible regressions if we use views
and we don't fix those dependencies.

> Can you explain what I have to do a little more?

Take out the CalcDifference hints that force view creation. You will however
still have to do a Reflow when the outline width changes. Take out the change to
FrameNeedsView. Check nsFrame::Paint and nsContainerFrame::PaintChildren to make
sure that a child with an outline will get painted even if the child's outline
intersects the paint area but the child's bounds do not. Check the same sort of
thing in GetFrameForPoint/GetFrameForPointUsing.

> I'm not exactly 100% what a view is

http://www.ocallahan.org/mozilla/view-talk/index.html

> One thing I've found with the current patch is that it doesn't handle when the
> frame resizes -- for example when an image finally gets loaded the view needs
> to get resized.

Can you post a testcase?

> I would need to change the change hint for resizes when there's an outline.
> Where do I do that?

nsFrame::CheckInvalidateSizeChange should already be doing the right
invalidation. If not, there's some other bug.

> The changes in your patch in layout/xul/base/src were what broke scrolling in
> HTML.

Yeah, I'm sure those changes don't work :-). The problem is that some form of
XUL changes are necessary to make XUL work properly with CSS2 'outline'.

    
    

    
  
(In reply to comment #44)
> > FinishAndStoreOverflow() was using GetOutlineRect which used mRect and that
> > wasn't calculated yet.
> Where was that?
When nsBlockFrame calls FinishAndStoreOverflow mRect has all 0's in it. Then
FinishAndStoreOverflow calls GetOutlineRect which takes mRect and inflates it by
the outline width.

> > One thing I've found with the current patch is that it doesn't handle when the
> > frame resizes -- for example when an image finally gets loaded the view needs
> > to get resized.
> 
> Can you post a testcase?
Shows up in the testcase attached in this bug called "More test cases with img
and block frames", although that only proves it for images as the image gets
loaded and the image not found icon gets replaced with the real image. Sounds
like I was assuming too much to say that resizing any frame with an outline
would do that. I'll do more testing once I get it working sans-views. 


    
    

    
  


  
The problem is that the stuff below gets painted after the outline, and gets
drawn on top of the outline. The outline is not supposed to be transparent.

    
    

    
  
> When nsBlockFrame calls FinishAndStoreOverflow mRect has all 0's in it. Then
> FinishAndStoreOverflow calls GetOutlineRect which takes mRect and inflates it
> by the outline width.

Ooooh good catch! We need to fix that by having FinishAndStoreOverflow compute
the *new* outline rect not by calling GetOutlineRect but by computing it from
scratch using the passed in aNewSize. That might fix a lot of issues.

> The problem is that the stuff below gets painted after the outline, and gets
> drawn on top of the outline. The outline is not supposed to be transparent.

Oh boy. CSS 2.1 says we have two choices about where to paint the outline in
z-order:
http://www.w3.org/TR/2004/CR-CSS21-20040225/zindex.html
-- Step 6.1.5 (paint it as soon as you've painted all the children of the
inline). This choice is actually what you're seeing here. I agree that it's not
that great.
-- Step 9 (paint it last in the stacking context, i.e. on top). This is probably
what you want. This is more of a radical change though. Hmm.

    
    

    
  
I suggest that for now we stick with the first option (the current behaviour)
even though it kinda sucks. It probably won't be that bad for narrow outline
widths and it is according to spec. Moving to the second option can be done as
part of the painting rework I'm planning for bug 78087.

    
    

    
  
(In reply to comment #47)
> Ooooh good catch! We need to fix that by having FinishAndStoreOverflow compute
> the *new* outline rect not by calling GetOutlineRect but by computing it from
> scratch using the passed in aNewSize. That might fix a lot of issues.
How about this (copied from most recent patch I posted):
+    if (mRect.height == 0 && mRect.width == 0) {
+      // We don't know the size of the current frame yet, so use the current 
+      // metrics (aNewSize) as the basis for the outline size
+      outlineRect.width += aNewSize.width;
+      outlineRect.height += aNewSize.height;
     }
This works rather nicely, since the current outlineRect is based on an inflated
(0,0,0,0) rect.



    
    

    
  
(In reply to comment #49)
> How about this (copied from most recent patch I posted):
> +    if (mRect.height == 0 && mRect.width == 0) {
> +      // We don't know the size of the current frame yet, so use the current 
> +      // metrics (aNewSize) as the basis for the outline size
> +      outlineRect.width += aNewSize.width;
> +      outlineRect.height += aNewSize.height;
>      }
> This works rather nicely, since the current outlineRect is based on an
> inflated (0,0,0,0) rect.

That won't handle size changes correctly. If mRect is going to grow from say
(0,0,10,10) to (0,0,100,100) with an outline width of 50, then the final outline
rect must be (-50,-50,150,150). We simply have to compute the outline rect based
on the new size, there's no getting around it.


    
    

    
  

      
      
      
      

        Attached patch
          
          
        Viewless outlines (obsolete)
        — 
          — Splinter Review
      

    


  

    
  

        Attachment #152129 -
        Attachment is obsolete: true

    
    

    
  
+  if (aMetrics.width > 400 ) {
+    FinishAndStoreOverflow(&aMetrics.mOverflowArea,
+                           nsSize(aMetrics.width, aMetrics.height));
+  }

Er ... '400'?

    
  

        Attachment #152130 -
        Flags: review?(roc)

    
    

    
  
+  PRBool hasOutline;
+  GetOutlineRect(&hasOutline);
+  if (hasOutline) {
+    nsCSSRendering::PaintOutline(aPresContext, aRenderingContext, this,
+                                  aDirtyRect, rect, *border, *outline,
+                                  mStyleContext, 0);
+  }

We may as well just call PaintOutline. It does the same check inside itself.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Remove '400' debugging line (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #152130 -
        Attachment is obsolete: true

    
    

    
  
It's not working on position: absolute with clipping or or with overflow: hidden
See the attachment "clipping/overflow testcases" attachment

    
    

    
  
+      { 
+        nsRect rect = GetOutlineRect(); // XXX Roc is this right?

What are you trying to do? This code is about painting selected images. I think
you don't want to touch this.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Addresses more comments (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #152131 -
        Attachment is obsolete: true

    
    

    
  
> CSS 2.1 says we have two choices about where to paint the outline in z-order

Ignore the last step... step 6.1.5 is the preferred one, the last one is only an 
allowance for browsers who can't do painting right, basically.

    
    

    
  
Even though it gives you the maybe unintuitive rendering shown?

I guess authors can add "position:relative" to bring it to the top.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        nearly there! (obsolete)
        — 
          — Splinter Review
      

    


  
This patch is pretty close. The problem with XUL and scrolling was that my
changes were forcing the view for the scrollport frame (an nsScrollBoxFrame) to
be sized to its overflowing children. So the scrollport's view was always
growing to the height of the scrolled content, which is definitely not what you
want. So I added the ability for XUL boxes to specify that they clip their
children and do not want to grow their views if their children overflow.

Without the XUL changes nsGfxScrollFrame was broken. You can see this in your
testcase for overflow:hidden; with the old patch, the yellow outline was not
repainted at the left, top and right if you moved the mouse out of the frame.
With the XUL changes, this is fixed.

Most of the UI seems to work and look just fine with these changes. Buttons do
their own outline painting so aren't really affected. The only problem I can
see right now is that XUL listboxes get horked. You can see this in the profile
list on startup, or in the "Languages" preference pane. I suspect one of the
listbox XUL boxes needs to be marked DoesClipChildren. Aaron, maybe you could
poke around and figure out which one, if you get a chance.

    
    

    
  
Hm, you're right, we should draw the outline in step 9, not earlier. I had 
misremembered the algorithm, I thought it said that the outlines should be 
painted right at the end of everything, not just at the end of each stacking 
context. So yeah, do it in step 9, and I'll fix the spec to only say that when 
we publish the next draft.

    
    

    
  
Thanks for the feedback Ian. I guess we'll change our rendering behaviour at a
later date :-)

    
    

    
  
You know you want to do it now..... ;-)

BTW, outline-offset might be of interest:
   http://www.w3.org/TR/css3-ui/#outline-offset
I believe Safari implements it.

Safari also implements outline-style:auto on Mac as meaning "use the Mac-style 
focus ring".

But I guess we'll leave those to a later bug.

    
    

    
  
outline-offset would be trivial to implement.

Another thing we'll need to do is to collapse outlines on multiple frames for
the same element so it's not just rendered as a bunch of overlapping rects.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        there
        — 
          — Splinter Review
      

    


  
Okay. I fixed the listbox thing --- actually it was a generic problem with
scrollbars. nsGfxScrollFrame tries to hide scrollbars by changing their width
or height to zero. With the new support for overflowing children in XUL, this
doesn't work anymore because the scrollbar buttons and thumb overflow the
scrollbar and can be painted. So I made scrollbars be treated as clipping their
children and we're in good shape again.

At this point I do not know of any bugs this introduces. I think it's ready for
review.

    
    

    
  
Comment on attachment 152223 [details] [diff] [review]
there

I think this outline patch is ready for review. We've squashed all the bugs in
it that we know about.

It is a bit risky partly because we're changing the behaviour of -moz-outline
which is used in quite a few places, and partly because we're messing around
with XUL which is not well understood. But Aaron and I have pounded on this a
bit, and any bugs that remain are not obvious. An alpha cycle is the perfect
time to check this in :-)

        Attachment #152223 -
        Flags: superreview?(dbaron)

        Attachment #152223 -
        Flags: review?(dbaron)

    
    

    
  
There is an additional problem with this.

The outline is rendered "in the back", meaning that text inside the element to
which the outline property is applied may overlap the outline if no sufficient
padding was given.

IF the problem with the positioning of the outline is fixed, I hope this
layering problem is taken into account as well; I wouldn't want to see the
outline disappear behind the background of one of the parent elements...

    
    

    
  
Now that the outline is painted outside the element's border-box, the element's
own content will never paint over it. Child elements can paint over it, though.
Putting outlines at the very top of the stacking context as per comment #62 is
something that's going to have to wait.

    
    

    
  
(In reply to comment #69)
> Now that the outline is painted outside the element's border-box, the 
element's
> own content will never paint over it.

That's a bit obvious, I'd say.  My concern was with the parent element.

Consider a DIV that has a 2-pixel #CCC border, containing another DIV to which a 
2-pixel #F00 border and a 2-pixel #000 outline is applied.  Question: should you 
see the 2-pixel border of the parent DIV, or the 2-pixel outline of the child 
DIV?

I'd say you should see the outline.  In the current "buggy" situation, the 
outline is rendered in the back; so if this layering problem isn't fixed, the 
outline will disappear behind the border of the parent element in my example.

    
    

    
  
You should see the outline. And with this patch, you do. No problem.

    
    

    
  
It seems like an inline

void FinishAndStoreOverflow(const nsHTMLReflowMetrics& aMetrics) {
  FinishAndStoreOverflow(aMetrics.mOverflowArea, nsSize(aMetrics.width,
aMetrics.height);
}

would simplify a bunch of the changes to callers...

    
    

    
  
Good idea! But it would have to be

void FinishAndStoreOverflow(nsHTMLReflowMetrics* aMetrics) {
  FinishAndStoreOverflow(&aMetrics->mOverflowArea, nsSize(aMetrics->width,
aMetrics->height);
}


    
    

    
  
(In reply to comment #71)
> You should see the outline. And with this patch, you do. No problem.

Thanks.  I'm sorry if I came across as a nitpicker, but I wanted to be sure. ;)

    
    

    
  
Comment on attachment 152223 [details] [diff] [review]
there

With that change (but why did you use a pointer rather than a reference?),
r+sr=dbaron.

        Attachment #152223 -
        Flags: superreview?(dbaron)

        Attachment #152223 -
        Flags: superreview+

        Attachment #152223 -
        Flags: review?(dbaron)

        Attachment #152223 -
        Flags: review+

    
    

    
  
I use a pointer so that it's clear in calling contexts that the parameter is
being modified.

    
  
Blocks: 251198

    
  

        Attachment #153181 -
        Attachment is obsolete: true

    
    

    
  


  
Roc, the DoesClipChildren() stuff is supposed to be in there, right?

    
    

    
  
Checking in content/base/src/nsStyleContext.cpp;
/cvsroot/mozilla/content/base/src/nsStyleContext.cpp,v  <--  nsStyleContext.cpp
new revision: 3.233; previous revision: 3.232
done
Checking in content/shared/src/nsStyleStruct.cpp;
/cvsroot/mozilla/content/shared/src/nsStyleStruct.cpp,v  <--  nsStyleStruct.cpp
new revision: 3.84; previous revision: 3.83
done
Checking in layout/base/public/nsIFrame.h;
/cvsroot/mozilla/layout/base/public/nsIFrame.h,v  <--  nsIFrame.h
new revision: 3.243; previous revision: 3.242
done
Checking in layout/html/base/src/nsAbsoluteContainingBlock.cpp;
/cvsroot/mozilla/layout/html/base/src/nsAbsoluteContainingBlock.cpp,v  <-- 
nsAbsoluteContainingBlock.cpp
new revision: 1.50; previous revision: 1.49
done
Checking in layout/html/base/src/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.632; previous revision: 3.631
done
Checking in layout/html/base/src/nsFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.489; previous revision: 3.488
done
Checking in layout/html/base/src/nsFrame.h;
/cvsroot/mozilla/layout/html/base/src/nsFrame.h,v  <--  nsFrame.h
new revision: 3.210; previous revision: 3.209
done
Checking in layout/html/base/src/nsImageFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsImageFrame.cpp,v  <--  nsImageFrame.cpp
new revision: 1.326; previous revision: 1.325
done
Checking in layout/html/base/src/nsInlineFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsInlineFrame.cpp,v  <--  nsInlineFrame.cpp
new revision: 3.225; previous revision: 3.224
done
Checking in layout/html/base/src/nsLineLayout.cpp;
/cvsroot/mozilla/layout/html/base/src/nsLineLayout.cpp,v  <--  nsLineLayout.cpp
new revision: 3.197; previous revision: 3.196
done
Checking in layout/html/base/src/nsPresShell.cpp;
/cvsroot/mozilla/layout/html/base/src/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.740; previous revision: 3.739
done
Checking in layout/html/forms/src/nsFieldSetFrame.cpp;
/cvsroot/mozilla/layout/html/forms/src/nsFieldSetFrame.cpp,v  <-- 
nsFieldSetFrame.cpp
new revision: 3.115; previous revision: 3.114
done
Checking in layout/html/style/src/nsCSSRendering.cpp;
/cvsroot/mozilla/layout/html/style/src/nsCSSRendering.cpp,v  <--  nsCSSRendering.cpp
new revision: 3.243; previous revision: 3.242
done
Checking in layout/html/table/src/nsTableCellFrame.cpp;
/cvsroot/mozilla/layout/html/table/src/nsTableCellFrame.cpp,v  <-- 
nsTableCellFrame.cpp
new revision: 3.334; previous revision: 3.333
done
Checking in layout/html/table/src/nsTableFrame.cpp;
/cvsroot/mozilla/layout/html/table/src/nsTableFrame.cpp,v  <--  nsTableFrame.cpp
new revision: 3.572; previous revision: 3.571
done
Checking in layout/html/table/src/nsTableOuterFrame.cpp;
/cvsroot/mozilla/layout/html/table/src/nsTableOuterFrame.cpp,v  <-- 
nsTableOuterFrame.cpp
new revision: 3.261; previous revision: 3.260
done
Checking in layout/html/table/src/nsTableRowFrame.cpp;
/cvsroot/mozilla/layout/html/table/src/nsTableRowFrame.cpp,v  <-- 
nsTableRowFrame.cpp
new revision: 3.351; previous revision: 3.350
done
Checking in layout/html/table/src/nsTableRowGroupFrame.cpp;
/cvsroot/mozilla/layout/html/table/src/nsTableRowGroupFrame.cpp,v  <-- 
nsTableRowGroupFrame.cpp
new revision: 3.329; previous revision: 3.328
done
Checking in layout/mathml/base/src/nsMathMLTokenFrame.cpp;
/cvsroot/mozilla/layout/mathml/base/src/nsMathMLTokenFrame.cpp,v  <-- 
nsMathMLTokenFrame.cpp
new revision: 1.24; previous revision: 1.23
done
Checking in layout/mathml/base/src/nsMathMLmpaddedFrame.cpp;
/cvsroot/mozilla/layout/mathml/base/src/nsMathMLmpaddedFrame.cpp,v  <-- 
nsMathMLmpaddedFrame.cpp
new revision: 1.31; previous revision: 1.30
done
Checking in layout/xul/base/src/nsBox.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsBox.cpp,v  <--  nsBox.cpp
new revision: 1.83; previous revision: 1.82
done
Checking in layout/xul/base/src/nsBox.h;
/cvsroot/mozilla/layout/xul/base/src/nsBox.h,v  <--  nsBox.h
new revision: 1.25; previous revision: 1.24
done
Checking in layout/xul/base/src/nsBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsBoxFrame.cpp,v  <--  nsBoxFrame.cpp
new revision: 1.240; previous revision: 1.239
done
Checking in layout/xul/base/src/nsLeafBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsLeafBoxFrame.cpp,v  <--  nsLeafBoxFrame.cpp
new revision: 1.35; previous revision: 1.34
done
Checking in layout/xul/base/src/nsScrollBoxFrame.h;
/cvsroot/mozilla/layout/xul/base/src/nsScrollBoxFrame.h,v  <--  nsScrollBoxFrame.h
new revision: 1.15; previous revision: 1.14
done
Checking in layout/xul/base/src/nsScrollbarFrame.h;
/cvsroot/mozilla/layout/xul/base/src/nsScrollbarFrame.h,v  <--  nsScrollbarFrame.h
new revision: 1.25; previous revision: 1.24
done
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED

    
    

    
  
Backed out because of bug 251510.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
I'm using Mozilla 1.8a2 build 2004071508 and regarding HTML button elements, I
still see focus outline inside the button borders while Opera 7.x draws outline
focus outside the button borders.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        additional fix
        — 
          — Splinter Review
      

    


  
Aaron, can you try applying this patch on top of the outline patch? I think it
fixes the regressions.

This is basically this morning's patch, except that this morning's patch
screwed up XUL trees because I missed changing the signature of
nsTreeBodyFrame::SetBounds. Fixing that seems to fix everything.

Please test this as thoroughly as you can :-)

    
    

    
  
I applied and tested these two patches together. They fix this bug as well the
issues with xul/html combos/lists/trees:
http://bugzilla.mozilla.org/attachment.cgi?id=153195&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=153368&action=view


    
    

    
  
Comment on attachment 153368 [details] [diff] [review]
additional fix

Here's an additional patch for the outline code, that fixes the smoketest
blocker problems we encountered before. There are two changes:

-- nsBox::SetBounds does not recompute the overflow area; that burden is pushed
to the caller (see http://bugzilla.mozilla.org/show_bug.cgi?id=251510#c5 )

-- we add a new box API to tell nsBox::SyncLayout that nsBoxToBlockAdaptors
compute their own overflow area and don't want it recomputed for them

        Attachment #153368 -
        Flags: superreview?(dbaron)

        Attachment #153368 -
        Flags: review?(dbaron)

        Attachment #153368 -
        Flags: superreview?(dbaron)

        Attachment #153368 -
        Flags: superreview+

        Attachment #153368 -
        Flags: review?(dbaron)

        Attachment #153368 -
        Flags: review+

    
    

    
  
Thanks David, that was quick! Okay Aaron, I think you can reland this now.

    
    

    
  
Checking in content/base/src/nsStyleContext.cpp;
/cvsroot/mozilla/content/base/src/nsStyleContext.cpp,v  <--  nsStyleContext.cpp
new revision: 3.235; previous revision: 3.234
done
Checking in content/shared/src/nsStyleStruct.cpp;
/cvsroot/mozilla/content/shared/src/nsStyleStruct.cpp,v  <--  nsStyleStruct.cpp
new revision: 3.90; previous revision: 3.89
done
Checking in layout/base/public/nsIFrame.h;
/cvsroot/mozilla/layout/base/public/nsIFrame.h,v  <--  nsIFrame.h
new revision: 3.245; previous revision: 3.244
done
Checking in layout/html/base/src/nsAbsoluteContainingBlock.cpp;
/cvsroot/mozilla/layout/html/base/src/nsAbsoluteContainingBlock.cpp,v  <-- 
nsAbsoluteContainingBlock.cpp
new revision: 1.52; previous revision: 1.51
done
Checking in layout/html/base/src/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.635; previous revision: 3.634
done
Checking in layout/html/base/src/nsFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.491; previous revision: 3.490
done
Checking in layout/html/base/src/nsFrame.h;
/cvsroot/mozilla/layout/html/base/src/nsFrame.h,v  <--  nsFrame.h
new revision: 3.212; previous revision: 3.211
done
Checking in layout/html/base/src/nsImageFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsImageFrame.cpp,v  <--  nsImageFrame.cpp
new revision: 1.328; previous revision: 1.327
done
Checking in layout/html/base/src/nsInlineFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsInlineFrame.cpp,v  <--  nsInlineFrame.cpp
new revision: 3.227; previous revision: 3.226
done
Checking in layout/html/base/src/nsLineLayout.cpp;
/cvsroot/mozilla/layout/html/base/src/nsLineLayout.cpp,v  <--  nsLineLayout.cpp
new revision: 3.200; previous revision: 3.199
done
Checking in layout/html/base/src/nsPresShell.cpp;
/cvsroot/mozilla/layout/html/base/src/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.742; previous revision: 3.741
done
Checking in layout/html/forms/src/nsFieldSetFrame.cpp;
/cvsroot/mozilla/layout/html/forms/src/nsFieldSetFrame.cpp,v  <-- 
nsFieldSetFrame.cpp
new revision: 3.118; previous revision: 3.117
done
Checking in layout/html/style/src/nsCSSRendering.cpp;
/cvsroot/mozilla/layout/html/style/src/nsCSSRendering.cpp,v  <--  nsCSSRendering.cpp
new revision: 3.245; previous revision: 3.244
done
Checking in layout/html/table/src/nsTableCellFrame.cpp;
/cvsroot/mozilla/layout/html/table/src/nsTableCellFrame.cpp,v  <-- 
nsTableCellFrame.cpp
new revision: 3.336; previous revision: 3.335
done
Checking in layout/html/table/src/nsTableFrame.cpp;
/cvsroot/mozilla/layout/html/table/src/nsTableFrame.cpp,v  <--  nsTableFrame.cpp
new revision: 3.574; previous revision: 3.573
done
Checking in layout/html/table/src/nsTableOuterFrame.cpp;
/cvsroot/mozilla/layout/html/table/src/nsTableOuterFrame.cpp,v  <-- 
nsTableOuterFrame.cpp
new revision: 3.263; previous revision: 3.262
done
Checking in layout/html/table/src/nsTableRowFrame.cpp;
/cvsroot/mozilla/layout/html/table/src/nsTableRowFrame.cpp,v  <-- 
nsTableRowFrame.cpp
new revision: 3.353; previous revision: 3.352
done
Checking in layout/html/table/src/nsTableRowGroupFrame.cpp;
/cvsroot/mozilla/layout/html/table/src/nsTableRowGroupFrame.cpp,v  <-- 
nsTableRowGroupFrame.cpp
new revision: 3.331; previous revision: 3.330
done
Checking in layout/mathml/base/src/nsMathMLTokenFrame.cpp;
/cvsroot/mozilla/layout/mathml/base/src/nsMathMLTokenFrame.cpp,v  <-- 
nsMathMLTokenFrame.cpp
new revision: 1.26; previous revision: 1.25
done
Checking in layout/mathml/base/src/nsMathMLmpaddedFrame.cpp;
/cvsroot/mozilla/layout/mathml/base/src/nsMathMLmpaddedFrame.cpp,v  <-- 
nsMathMLmpaddedFrame.cpp
new revision: 1.33; previous revision: 1.32
done
Checking in layout/xul/base/src/nsBox.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsBox.cpp,v  <--  nsBox.cpp
new revision: 1.85; previous revision: 1.84
done
Checking in layout/xul/base/src/nsBox.h;
/cvsroot/mozilla/layout/xul/base/src/nsBox.h,v  <--  nsBox.h
new revision: 1.27; previous revision: 1.26
done
Checking in layout/xul/base/src/nsBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsBoxFrame.cpp,v  <--  nsBoxFrame.cpp
new revision: 1.242; previous revision: 1.241
done
Checking in layout/xul/base/src/nsBoxToBlockAdaptor.h;
/cvsroot/mozilla/layout/xul/base/src/nsBoxToBlockAdaptor.h,v  <-- 
nsBoxToBlockAdaptor.h
new revision: 1.21; previous revision: 1.20
done
Checking in layout/xul/base/src/nsIBox.h;
/cvsroot/mozilla/layout/xul/base/src/nsIBox.h,v  <--  nsIBox.h
new revision: 1.31; previous revision: 1.30
done
Checking in layout/xul/base/src/nsLeafBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsLeafBoxFrame.cpp,v  <--  nsLeafBoxFrame.cpp
new revision: 1.37; previous revision: 1.36
done
Checking in layout/xul/base/src/nsScrollBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsScrollBoxFrame.cpp,v  <-- 
nsScrollBoxFrame.cpp
new revision: 1.49; previous revision: 1.48
done
Checking in layout/xul/base/src/nsScrollBoxFrame.h;
/cvsroot/mozilla/layout/xul/base/src/nsScrollBoxFrame.h,v  <--  nsScrollBoxFrame.h
new revision: 1.17; previous revision: 1.16
done
Checking in layout/xul/base/src/nsScrollbarFrame.h;
/cvsroot/mozilla/layout/xul/base/src/nsScrollbarFrame.h,v  <--  nsScrollbarFrame.h
new revision: 1.27; previous revision: 1.26
done
Checking in layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,v  <-- 
nsTreeBodyFrame.cpp
new revision: 1.223; previous revision: 1.222
done
Checking in layout/xul/base/src/tree/src/nsTreeBodyFrame.h;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.h,v  <-- 
nsTreeBodyFrame.h
new revision: 1.77; previous revision: 1.76
done
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED

    
  
Blocks: css2outline
No longer depends on: css2outline

    
    

    
  
FWIW, focus outline are drawn inside for input type="button", type="reset" and
type="submit" and HTML button elements. Mozilla 1.8a3 build 2004072008 under XP
Pro SP1a here.

    
    

    
  
Yeah, they have their own outline painting code. Aaron, try talking to bz, who
knows that code, to see if it can be removed.

    
    

    
  
We still want focus outlines to be drawn on the inside of buttons, at least for
Win/Lin dotted focus outlines. That matches platform appearance.

    
    

    
  
Well, it's a very minor bug, but I suspect that fixing this bug caused bug 257719.

    
    

    
  
Another bug that seemed to be caused by this is bug 255863.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: