151375 - focus outline should be drawn outside of element

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

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."

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Greg K.]

Should be All/All, shouldn't it?

Comment 3

[Aaron Leventhal]

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]

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[Aaron Leventhal]

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.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 7

[Aaron Leventhal]

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Taking. This is really a duplicate of bug 9809, I think.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.)

Comment 11

[Aaron Leventhal]

I think this is a major problem with our rendering.

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

Comment 12

[Rafael Ebron (:rebron)]

adt: nsbeta1-

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Aaron Leventhal]

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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 16

[Aaron Leventhal]

(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.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 18

[Aaron Leventhal]

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

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 :-)

Comment 20

[Aaron Leventhal]

Attachment: Initiial patch to experiment with and talk about, contains flaws

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

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    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.

Comment 22

[Aaron Leventhal]

Attachment: Fixes problems with selection, img and dynamic changes

Comment 23

[Aaron Leventhal]

Attachment: More test cases with img and block frames

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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.

Comment 25

[Aaron Leventhal]

Attachment: Clipping/overflow test cases

Comment 26

[Aaron Leventhal]

Attachment: Addresses comments, fixes issues with clipping and overflow

Comment 27

[Aaron Leventhal]

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.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+      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.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: work in progress

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.

Comment 32

[Aaron Leventhal]

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

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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).

Comment 34

[Aaron Leventhal]

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?

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 :-)

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch

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 37

[Aaron Leventhal]

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.

Comment 38

[Aaron Leventhal]

(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. 

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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?

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.)

Comment 41

[Aaron Leventhal]

(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?

Comment 42

[Aaron Leventhal]

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

Comment 43

[Aaron Leventhal]

Attachment: Calculate correct outline size during reflow

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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'.

Comment 45

[Aaron Leventhal]

(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. 

Comment 46

[Aaron Leventhal]

Attachment: Screenshot - poor results without 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.

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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.

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 49

[Aaron Leventhal]

(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.

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 51

[Aaron Leventhal]

Attachment: Viewless outlines

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Er ... '400'?

Comment 53

[Aaron Leventhal]

Attachment: Viewless outlines, always use aNewSize for outline rect in FinishAndStoreOverflow()

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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.

Comment 55

[Aaron Leventhal]

Attachment: Remove '400' debugging line

Comment 56

[Aaron Leventhal]

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

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+      { 
+        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.

Comment 58

[Aaron Leventhal]

Attachment: Addresses more comments

Comment 59

[Hixie (not reading bugmail)]

> 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.

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Even though it gives you the maybe unintuitive rendering shown?

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

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: nearly there!

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.

Comment 62

[Hixie (not reading bugmail)]

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.

Comment 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 64

[Hixie (not reading bugmail)]

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.

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: there

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 67

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 :-)

Comment 68

[Thomas Verelst]

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...

Comment 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 70

[Thomas Verelst]

(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.

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 72

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Good idea! But it would have to be

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

Comment 74

[Thomas Verelst]

(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 75

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 76

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 77

[Aaron Leventhal]

Attachment: Final patch ready for checkin, using FinishAndStoreOverflow inlin

Comment 78

[Aaron Leventhal]

Attachment: Patch with simple FinishAndStoreOverflow() inline wrapper

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

Comment 79

[Aaron Leventhal]

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

Comment 80

[Aaron Leventhal]

Backed out because of bug 251510.

Comment 81

[Gérard Talbot]

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.

Comment 82

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: additional fix

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 :-)

Comment 83

[Aaron Leventhal]

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 84

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 85

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 86

[Aaron Leventhal]

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

Comment 87

[Gérard Talbot]

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.

Comment 88

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 89

[Aaron Leventhal]

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.

Comment 90

[Martijn Wargers (dead)]

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

Comment 91

[Martijn Wargers (dead)]

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