480888 - outlines are drawn outside (i.e., expanded by) box-shadow and other visual overflow

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Lucas Sanders]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_6; en-us) AppleWebKit/528.16 (KHTML, like Gecko) Version/4.0 Safari/528.16
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090301 Shiretoko/3.1b3pre

When an outline and a box-shadow are applied to the same element, the outline is rendered outside the box-shadow (instead of immediately outside the border).

This is partly a specification problem, since neither the CSS 2.1 specification nor the CSS 3 Basic User Interface module drafts fully specify the desired rendering of 'outline' in the relevant section.
 - CSS 2.1, which pre-dates box-shadow, says that "The outline may be drawn starting just outside the border edge" <http://www.w3.org/TR/CSS21/ui.html#dynamic-outlines>, but doesn't *require* any particular rendering.
 - The CSS 3 draft is less helpful, defining the rendering of 'outline' thus:  "User agents should use an algorithm for determining the outline that encloses a region appropriate for conveying the concept of focus to the user."  <http://www.w3.org/TR/css3-ui/#propdef-outline-color>

However, CSS 3's specification of a new outline-offset property clarifies the intended rendering to be the one suggested by CSS 2.1:  "By default, the outline is drawn starting just outside the border edge."  <http://www.w3.org/TR/css3-ui/#outline-offset>

The WebKit team has implemented this with outlines immediately outside the box's border, as the specifications assume.

Reproducible: Always

Steps to Reproduce:
1. Visit testcase.
2. Observe incorrect rendering.  :)
Actual Results:  
The outline is rendered as a rectangle surrounding both the content box and its box-shadow.

Expected Results:  
A box's outline is rendered immediately surrounding its border.

Comment 1

[Lucas Sanders]

Attachment: simplified test case

Comment 2

[Emil Ivanov]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090829 Minefield/3.7a1pre

Comment 4

[Jon Raasch]

Confirmed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.0.8, Ant.com Toolbar 1.3

Comment 5

[j.j.]

Attachment: crossbrowser testcase

Comment 6

[matthias koplenig [:metasieben]]

google has started using this @ image-search: 

box-shadow: 0 4px 16px rgba(0, 0, 0, 0.2);
outline: 1px rgba(0, 0, 0, 0.2);

therefore it looks _broken_ in all gecko-browsers.

Comment 8

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

See the "FIXME (bug 599652)" comment in ComputeOutlineAndEffectsRect in layout/generic/nsFrame.cpp, plus the boundary accumulation code in nsCSSRendering::PaintOutline in layout/base/nsCSSRendering.cpp.

That said, switching from visual overflow to scrollable overflow probably is not the best solution, since scrollable overflow will become larger than visual overflow for some cases in bug 665597, in a way we wouldn't want to apply to outlines.

So, instead, I think we should not use an overflow area at all, but have a function for computing the area to draw outlines around.  This function would need to (hypothetically) walk all descendants of the frame with the outline (probably just including their border boxes); however, it could (to significantly optimize that walk) skip descendants where *either* the visual overflow or the scrollable overflow is contained entirely within the frame, since all border boxes are guaranteed to be contained in both visual and scrollable overflow.  It could be further optimized by using a conservative approximation of this when computing the visual overflow area (just increase the visual overflow by the outline, whether or not it's actually drawn at the edge of the current visual overflow) so that we only need to call this function when painting.  We could also, if needed, cache the result in a frame property that is deleted on Reflow.  This would make outlines a good bit more expensive; however, outlines are rare (in the sense that there's generally only one at once).

Comment 10

[Stephanie Sullivan Rewis]

Apparently I've just filed a dupe <sigh>... https://bugzilla.mozilla.org/show_bug.cgi?id=687311 Philippe recognized the similarity though I searched (and my problem cases are different).

Comment 11

[Fabien Ménager]

I confirm this very annoying bug, I hope it could be solved quickly.

Comment 12

[Palle Zingmark]

I second Fabien Ménage comment and hope it can be solved soon.

poc: http://jsfiddle.net/kolombiken/QRwqK/

Comment 13

[Ida Franceen]

+1 on Palle and Fabien. It's a truly annoying bug.

Comment 14

[Stephanie Sullivan Rewis]

Agree with the three comments above. Please see my notes added to the duplicate of this bug (which I filed): (https://bugzilla.mozilla.org/show_bug.cgi?id=687311 ) David Baron stated tonight that this is a feature, not a bug. I strongly disagree — with demo back up. 

Discuss?

Comment 15

[Stephanie Sullivan Rewis]

(In reply to Palle Zingmark from comment #12)
> poc: http://jsfiddle.net/kolombiken/QRwqK/

BTW, until this is fixed, you can likely get the effect you require using the spread radius in box-shadow:
http://jsfiddle.net/stefsull/nbQUC/

Comment 16

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

No, please do not discuss.  This is a bug.

Comment 17

[Fabien Ménager]

Thank you Stephanie Sullivan, this workaround works like a charm.

(In reply to Stephanie Sullivan from comment #15)
> (In reply to Palle Zingmark from comment #12)
> > poc: http://jsfiddle.net/kolombiken/QRwqK/
> 
> BTW, until this is fixed, you can likely get the effect you require using
> the spread radius in box-shadow:
> http://jsfiddle.net/stefsull/nbQUC/

Comment 18

[tim.erwin]

this has been a bug here for like three years. this is horrible. I was annoyed to see it working fine in chrome and then see firefox acting goofy. I'm glad chrome is my #1 browser.

Comment 19

[Leon Sorokin]

for 13 votes and 3 years in the pipes, it's certainly not getting much love or attention. it's also blocking a bug i opened 6 months back :(

Comment 20

[Tom Lee]

David, I'd be happy to pick this up if you could assign it to me & you're willing to tolerate the odd stupid question.

Comment 21

[Tom Lee]

(In reply to Palle Zingmark from comment #12)
> I second Fabien Ménage comment and hope it can be solved soon.
> 
> poc: http://jsfiddle.net/kolombiken/QRwqK/

Attaching a WIP that seems to address the bug as it manifests in this poc, but displays incorrect behavior in other cases. For example:

http://jsfiddle.net/QRwqK/20/

I think it's probably because I'm not correctly computing the border boxes of the child containers that David described.

So David, could you please address the following questions when you have a moment spare?

(In reply to David Baron [:dbaron] from comment #8)
> See the "FIXME (bug 599652)" comment in ComputeOutlineAndEffectsRect in
> layout/generic/nsFrame.cpp, plus the boundary accumulation code in
> nsCSSRendering::PaintOutline in layout/base/nsCSSRendering.cpp.
> 
> That said, switching from visual overflow to scrollable overflow probably is
> not the best solution, since scrollable overflow will become larger than
> visual overflow for some cases in bug 665597, in a way we wouldn't want to
> apply to outlines.
> 
> So, instead, I think we should not use an overflow area at all, but have a
> function for computing the area to draw outlines around.  This function
> would need to (hypothetically) walk all descendants of the frame with the
> outline (probably just including their border boxes); 

As you can see in the WIP patch, I'm currently calculating the border box using nsRect(frame->GetRelativeOffset(), frame->GetSize()) for each descendent child. This appears to be wrong per my example above when descendents are larger than their fixed-width parent.

What's the correct way to compute the border box you describe here?

> however, it could (to
> significantly optimize that walk) skip descendants where *either* the visual
> overflow or the scrollable overflow is contained entirely within the frame,
> since all border boxes are guaranteed to be contained in both visual and
> scrollable overflow.  

RE my patch: Am I doing this correctly?

> It could be further optimized by using a conservative
> approximation of this when computing the visual overflow area (just increase
> the visual overflow by the outline, whether or not it's actually drawn at
> the edge of the current visual overflow) so that we only need to call this
> function when painting.  We could also, if needed, cache the result in a
> frame property that is deleted on Reflow.  This would make outlines a good
> bit more expensive; however, outlines are rare (in the sense that there's
> generally only one at once).

I have more questions around these, but I won't bother attempting to implement either of these optimizations until I get the basics right. :)

Comment 22

[Tom Lee]

Attachment: WIP patch for #480888

Comment 23

[hsablonniere]

What's the status on this one? Just discovered it today :-(

Comment 25

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

Sorry for taking so long to get to this; in the future, it helps to use a feedback? request on the patch to get my attention.  (And while I made one myself, when I'm busy I try to prioritize requests from names I don't recognize, so that didn't help so much.)

(In reply to Tom Lee from comment #21)
> (In reply to David Baron [:dbaron] from comment #8)
> > See the "FIXME (bug 599652)" comment in ComputeOutlineAndEffectsRect in
> > layout/generic/nsFrame.cpp, plus the boundary accumulation code in
> > nsCSSRendering::PaintOutline in layout/base/nsCSSRendering.cpp.
> > 
> > That said, switching from visual overflow to scrollable overflow probably is
> > not the best solution, since scrollable overflow will become larger than
> > visual overflow for some cases in bug 665597, in a way we wouldn't want to
> > apply to outlines.
> > 
> > So, instead, I think we should not use an overflow area at all, but have a
> > function for computing the area to draw outlines around.  This function
> > would need to (hypothetically) walk all descendants of the frame with the
> > outline (probably just including their border boxes); 
> 
> As you can see in the WIP patch, I'm currently calculating the border box
> using nsRect(frame->GetRelativeOffset(), frame->GetSize()) for each
> descendent child. This appears to be wrong per my example above when
> descendents are larger than their fixed-width parent.
> 
> What's the correct way to compute the border box you describe here?

You don't want to use GetRelativeOffset.  Just GetRect() (and maybe GetPosition() or GetSize(), which are halves of GetRect()).

> > however, it could (to
> > significantly optimize that walk) skip descendants where *either* the visual
> > overflow or the scrollable overflow is contained entirely within the frame,
> > since all border boxes are guaranteed to be contained in both visual and
> > scrollable overflow.  
> 
> RE my patch: Am I doing this correctly?

No, in two ways:

 (1) you still need to include a frame if it doesn't have overflow, you can just skip walking its children.  So I'd put the test inside GetOutlineArea (in only one place rather than two)

 (2) what you need to test is whether either GetScrollableOverflowRect() or GetVisualOverflowRect() is the same as nsRect(nsPoint(0, 0), GetSize()).  (And you could possibly do this in a loop using GetOverflowRect() and NS_FOR_FRAME_OVERFLOW_TYPES, though that might make the code worse depending on what the test looks like.)

> > It could be further optimized by using a conservative
> > approximation of this when computing the visual overflow area (just increase
> > the visual overflow by the outline, whether or not it's actually drawn at
> > the edge of the current visual overflow) so that we only need to call this
> > function when painting.  We could also, if needed, cache the result in a
> > frame property that is deleted on Reflow.  This would make outlines a good
> > bit more expensive; however, outlines are rare (in the sense that there's
> > generally only one at once).
> 
> I have more questions around these, but I won't bother attempting to
> implement either of these optimizations until I get the basics right. :)

I think they're probably not needed.


However, I also think that the patch will need to modify other areas that compute where the outline goes, not just PaintOutline.  In particular, the function ComputeOutlineAndEffectsRect (in nsFrame.cpp) and *maybe* nsDisplayOutline::GetBounds (we should probably ask roc whether he thinks that's worth changing).

Comment 26

[Tom Lee]

Cool. Thanks for your feedback David, I'll take another look at this over the next few days.

Will be sure to flag my patches in the future, too.

Comment 28

[dominikmarczuk]

Is this ever going to be resolved?
http://jsfiddle.net/nNSjY/

Comment 29

[tim.erwin]

stop talking about it and just fix the damn bug

Comment 31

[Keith Pepin]

Is this ever going to be fixed?  It's been 4 years since it was initially reported.  We're actually working on a design now for an application and Firefox is the only major browser that doesn't render the design correctly (even IE passed) due to this bug.

Comment 32

[Josh Matthews [:jdm]]

Tom, could you confirm that you're still working on this?

Comment 33

[Tom Lee]

Josh, I've not looked at this in a while though I've been intending to.

I'm not particularly familiar with the code in question & it seems folks are keen for a fix. Happy to hand this off to somebody else with the time/experience to make it happen in short order.

That said, if nobody else addresses this prior to the weekend I'll try to get some assistance on IRC so we can move this forward.

Comment 34

[Stephanie Sullivan Rewis]

(In reply to Tom Lee from comment #33) 
> I'm not particularly familiar with the code in question & it seems folks are
> keen for a fix. Happy to hand this off to somebody else with the
> time/experience to make it happen in short order.
> 
> That said, if nobody else addresses this prior to the weekend I'll try to
> get some assistance on IRC so we can move this forward.

Tom, we would be forever in your debt if you can get to it or get someone that can. Seriously. :) It's a problem for many of us.

Comment 35

[Tom Lee]

Alright, David I've finally gotten around to taking another look at this.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #25)
> You don't want to use GetRelativeOffset.  Just GetRect() (and maybe
> GetPosition() or GetSize(), which are halves of GetRect()).
> 

Easily done.

> > > however, it could (to
> > > significantly optimize that walk) skip descendants where *either* the visual
> > > overflow or the scrollable overflow is contained entirely within the frame,
> > > since all border boxes are guaranteed to be contained in both visual and
> > > scrollable overflow.  
> > 
> > RE my patch: Am I doing this correctly?
> 
> No, in two ways:
> 
>  (1) you still need to include a frame if it doesn't have overflow, you can
> just skip walking its children.  So I'd put the test inside GetOutlineArea
> (in only one place rather than two)
> 

Ah, of course -- I need to include the frame's rect, but not its children. Again, easily done.

>  (2) what you need to test is whether either GetScrollableOverflowRect() or
> GetVisualOverflowRect() is the same as nsRect(nsPoint(0, 0), GetSize()). 

When you say "the same as" here you mean "fully contained by", right?

And you're talking about the Get{Visual,Scrollable}OverflowRect of a descendant, but do you mean the GetSize() associated with the "parent" frame with the outline, or the rect that we're in the process of computing?

> (And you could possibly do this in a loop using GetOverflowRect() and
> NS_FOR_FRAME_OVERFLOW_TYPES, though that might make the code worse depending
> on what the test looks like.)
> 

To avoid the recursion? I'll make another pass at this once I get something working. :)

> However, I also think that the patch will need to modify other areas that
> compute where the outline goes, not just PaintOutline.  In particular, the
> function ComputeOutlineAndEffectsRect (in nsFrame.cpp) and *maybe*
> nsDisplayOutline::GetBounds (we should probably ask roc whether he thinks
> that's worth changing).

I'll ping roc once I get a response from you, but does that mean we'll need to find a common home for what I'm currently calling GetOutlineArea(nsIFrame*)?

Comment 36

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

(In reply to Tom Lee from comment #35)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #25)
> >  (2) what you need to test is whether either GetScrollableOverflowRect() or
> > GetVisualOverflowRect() is the same as nsRect(nsPoint(0, 0), GetSize()). 
> 
> When you say "the same as" here you mean "fully contained by", right?

No, I meant "the same as".

> And you're talking about the Get{Visual,Scrollable}OverflowRect of a
> descendant, but do you mean the GetSize() associated with the "parent" frame
> with the outline, or the rect that we're in the process of computing?

No, I'm talking about the same frame.

And in particular, the above point was about the condition that would allow you to avoid walking the entire subtree for the common case.  Both overflow areas report an area covered by the frame and all its descendants (with the two overflow areas calulated differently; one for scrolling and one for painting).  If the overflow area is the same as the frame's area, then you know there's nothing in the descendants that's worth traversing.

> > (And you could possibly do this in a loop using GetOverflowRect() and
> > NS_FOR_FRAME_OVERFLOW_TYPES, though that might make the code worse depending
> > on what the test looks like.)
> > 
> 
> To avoid the recursion? I'll make another pass at this once I get something
> working. :)

Yes, I think this suggestion was also about how you might skip the recursion over the subtree if *either* overflow area matched the frame boundary.

> > However, I also think that the patch will need to modify other areas that
> > compute where the outline goes, not just PaintOutline.  In particular, the
> > function ComputeOutlineAndEffectsRect (in nsFrame.cpp) and *maybe*
> > nsDisplayOutline::GetBounds (we should probably ask roc whether he thinks
> > that's worth changing).
> 
> I'll ping roc once I get a response from you, but does that mean we'll need
> to find a common home for what I'm currently calling
> GetOutlineArea(nsIFrame*)?

probably yes.  A static method on nsCSSRendering (probably slightly preferable) or nsLayoutUtils would probably be fine.

Comment 37

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

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #25)
> However, I also think that the patch will need to modify other areas that
> compute where the outline goes, not just PaintOutline.  In particular, the
> function ComputeOutlineAndEffectsRect (in nsFrame.cpp) and *maybe*
> nsDisplayOutline::GetBounds (we should probably ask roc whether he thinks
> that's worth changing).

Actually ComputeOutlineAndEffectsRect and nsDisplayOutline::GetBounds can probably remain unchanged, as long as they continue to compute a rectangle that contains whatever it is you're actually drawing. You can just remove the "// FIXME". You may be able to remove all use of the OultineInnerRectProperty.

Comment 38

[Tom Lee]

David, I've got a patch that seems to render these test cases the same as in WebKit with the exception of the one I raised here:

http://jsfiddle.net/QRwqK/20/

Note that webkit seems to be using the display rect rather than the overflow rect of any descendants to paint the outline -- even if you explicitly set overflow. In contrast, with my patch we render our outline around the overflow.

roc's opinion is that if WebKit & IE10 render the outline using the display rect + a slight offset (which they do), perhaps we should do the same. Would you agree?

If so, I think the fix might be a lot more simple than we originally thought in that we can simply use nsRect(nsPoint(0, 0), GetSize()) of aFrame plus the offset we've already calculated in PaintOutline.

I'm going to attach my WIP for you to review to be sure it's not a bug in my code, but either way I think we're close. Let me know what you want to do wrt overflow & IE10/WebKit.

Comment 39

[Tom Lee]

Attachment: WIP #2 -- rendering outline around overflow instead of the display rect

Here's the updated WIP patch, renders outline around overflow rather than around the display rect, which is different to WebKit+IE10 but seems to fix all other test cases. (Refer to my comment above)

Comment 40

[Tom Lee]

Attachment: WIP #3 -- slight fix for WIP #2

Noticed a silly bug in my WIP #2 patch. Comments RE: overflow vs WebKit+IE10 still stand.

Going to try my hand at a patch that renders outlines with overflow in the same manner as WebKit+IE10.

Comment 41

[Tom Lee]

Attachment: ALT #1 -- render outline ignoring overflow

Here's an alternative patch that renders outlines for all PoCs in this bug report (including my overflow example) in a way that seems to be in line with WebKit+IE10 -- i.e. ignoring overflow.

Comment 42

[pondjon]

I need it fixed, too!  I noticed you have a potential fix for this, what is your ETA for getting it in the build?

Comment 43

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

Comment on attachment 742954 [details] [diff] [review]
WIP #3 -- slight fix for WIP #2

>diff -r 05533d50f2f7 layout/base/nsCSSRendering.cpp
>--- a/layout/base/nsCSSRendering.cpp	Sun Apr 28 18:27:00 2013 -0400
>+++ b/layout/base/nsCSSRendering.cpp	Sun Apr 28 19:01:11 2013 -0700
>@@ -757,16 +757,27 @@
> }
> 
> static nsRect
>-GetOutlineInnerRect(nsIFrame* aFrame)
>+GetOutlineArea(nsIFrame* aFrame)
> {
>-  nsRect* savedOutlineInnerRect = static_cast<nsRect*>
>-    (aFrame->Properties().Get(nsIFrame::OutlineInnerRectProperty()));
>-  if (savedOutlineInnerRect)
>-    return *savedOutlineInnerRect;
>-  // FIXME (bug 599652): We probably want something narrower than either
>-  // overflow rect here, but for now use the visual overflow in order to
>-  // be consistent with ComputeOutlineAndEffectsRect in nsFrame.cpp.
>-  return aFrame->GetVisualOverflowRect();

We should also remove the code in nsFrame.cpp that sets the OutlineInerRectProperty, and the declaration of OutlineInnerRectProperty, since it's now unused:
http://mxr.mozilla.org/mozilla-central/search?string=OutlineInnerRectProp

>+  nsRect outlineArea(nsPoint(0, 0), aFrame->GetSize());
>+  nsIFrame *currentFrame = aFrame->GetFirstPrincipalChild();
>+  while (currentFrame != NULL) {

This needs to traverse all child lists, not just the principal one.  You want something more like:

  for (nsIFrame::ChildListIterator lists(aFrame);
       !lists.IsDone(); lists.Next()) {
    for (nsFrameList::Enumerator childFrames(lists.CurrentList());
         !childFrames.AtEnd(); childFrames.Next()) {
      nsIFrame* child = childFrames.get();

>+    //
>+    // If the overflow area is the same as currentFrame's area,
>+    // we know there's nothing in the descendants that's worth traversing.
>+    //
>+    const nsRect testRect(nsPoint(0, 0), currentFrame->GetSize());
>+    if (testRect == currentFrame->GetScrollableOverflowRect() &&
>+        testRect == currentFrame->GetVisualOverflowRect()) {
>+      outlineArea.UnionRect(outlineArea, testRect);
>+    }
>+    else {
>+      outlineArea.UnionRect(outlineArea, GetOutlineArea(currentFrame));
>+    }

In both cases, this needs to add on currentFrame->GetPosition() so that the result is in the correct coordinate space.

I think it probably also ought to account for transforms, though I suppose that's gotten pretty complicated these days given preserve-3d.  (Maybe that means we should give up on this approach and switch to the other, though I'm not happy about that?)

>       // The outline has already been included in aForFrame's overflow
>       // area, but not in those of its descendants, so we have to
>       // include it.  Otherwise we'll end up drawing the outline inside
>       // the border.

I don't think this comment is relevant anymore (or was before this change, even), so it should be removed.



Otherwise this looks good, though it also needs a test (probably a reftest).

Comment 44

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

Comment on attachment 742954 [details] [diff] [review]
WIP #3 -- slight fix for WIP #2

>+    //
>+    // If the overflow area is the same as currentFrame's area,
>+    // we know there's nothing in the descendants that's worth traversing.
>+    //
>+    const nsRect testRect(nsPoint(0, 0), currentFrame->GetSize());
>+    if (testRect == currentFrame->GetScrollableOverflowRect() &&
>+        testRect == currentFrame->GetVisualOverflowRect()) {

Oh, and this could be || rather than &&.

Comment 45

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

Comment on attachment 742958 [details] [diff] [review]
ALT #1 -- render outline ignoring overflow

>diff -r 05533d50f2f7 layout/base/nsCSSRendering.cpp
>--- a/layout/base/nsCSSRendering.cpp	Sun Apr 28 18:27:00 2013 -0400
>+++ b/layout/base/nsCSSRendering.cpp	Sun Apr 28 19:19:48 2013 -0700
>@@ -756,19 +756,6 @@
>   SN();
> }
> 
>-static nsRect
>-GetOutlineInnerRect(nsIFrame* aFrame)
>-{
>-  nsRect* savedOutlineInnerRect = static_cast<nsRect*>
>-    (aFrame->Properties().Get(nsIFrame::OutlineInnerRectProperty()));
>-  if (savedOutlineInnerRect)
>-    return *savedOutlineInnerRect;

Again, you'd want to remove the rest of the code dealing with OutlineInnerRectProperty.

And, again, you'd want a reftest.

> void
> nsCSSRendering::PaintOutline(nsPresContext* aPresContext,
>                              nsRenderingContext& aRenderingContext,
>@@ -812,17 +799,18 @@
>     frameForArea = frameForArea->GetFirstPrincipalChild();
>     NS_ASSERTION(frameForArea, "anonymous block with no children?");
>   } while (frameForArea);
>+
>   nsRect innerRect; // relative to aBorderArea.TopLeft()
>   if (frameForArea == aForFrame) {
>-    innerRect = GetOutlineInnerRect(aForFrame);
>+      innerRect = nsRect(nsPoint(0, 0), aForFrame->GetSize());

The indent here is wrong.

>   } else {
>     for (; frameForArea; frameForArea = frameForArea->GetNextSibling()) {
>-      // The outline has already been included in aForFrame's overflow
>+      // The outline has already been included in aForFrame's 
>       // area, but not in those of its descendants, so we have to
>       // include it.  Otherwise we'll end up drawing the outline inside
>       // the border.

This whole comment should go.

>-      nsRect r(GetOutlineInnerRect(frameForArea) +
>-               frameForArea->GetOffsetTo(aForFrame));
>+      nsRect r(nsRect(nsPoint(0, 0), frameForArea->GetSize()) +
>+                frameForArea->GetOffsetTo(aForFrame));

This would be cleaner as
      nsRect r(frameForArea->GetOffsetTo(aFrFrame), frameForAea->GetSize());


Somebody should probably bring this issue up on www-style.  I suppose I can try to do that.

Comment 46

[Tom Lee]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #45)
> Somebody should probably bring this issue up on www-style.  I suppose I can
> try to do that.

Much appreciated, David! Once I get confirmation which direction we should be taking here, I'll clean up the appropriate patch & we can hopefully get this fixed once & for all :)

Comment 47

[Tom Lee]

(In reply to pondjon from comment #42)
> I need it fixed, too!  I noticed you have a potential fix for this, what is
> your ETA for getting it in the build?

Hard to give an ETA until we know what the correct way forward should be (which is why dbaron has offered to speak to www-style). Once I get confirmation, cleaning up the appropriate patch & sorting out a test shouldn't take long.

Comment 48

[tim.erwin]

this is the most ridiculously absurd process to fixing bugs that I've ever seen...get ahold of yourselves!!!!

I'm getting email after email, it was 2009 that it was opened and you guys are still talking about it. just read the last few comments and we can easily see why nothing is getting done here. "could be", "should be", "hopefully", "hard to give", "once I get...", "shouldn't take..." get this **** together!!!

Comment 49

[Keith Pepin]

Tom, David, et. al.

Unlike Mr. Erwin, I appreciate the efforts you are making and am glad you have the WIP fix for this.  I know it can be difficult to get these things "right" or to "spec."  I am anxious though, because as I mentioned before this fix is greatly needed for a software product my team and I are working on.  Please keep us posted with your progress, and we look forward to seeing these fixes in a new release for FF soon.

Keith

Comment 50

[Tom Lee]

Flagging this as needinfo? RE: your query to www-style, David.

Comment 51

[pondjon]

Is there any chance you can get this fixed and into the build by May 17th?  I'm working on a project right now that must be code complete by the 17th and we're using outlines.  All other browsers render this correctly, except Firefox.  Please let me know if you don't think it will be in by the 17th because we'll need to think of other alternatives for outlines.

Comment 52

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

Posted http://lists.w3.org/Archives/Public/www-style/2013May/0668.html

Comment 53

[pondjon]

Agreed! :)  What is your ETA for having it in the build?

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #52)
> Posted http://lists.w3.org/Archives/Public/www-style/2013May/0668.html

Comment 54

[Tom Lee]

Any news here David? Can't see any responses on www-style. In lieu of a decision there, my preference would be to just align with WebKit & IE10. Obviously your call, though.

Comment 55

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

We had some discussion in today's CSS WG telecon; minutes should be available within a few days.

Comment 56

[Stephanie Sullivan Rewis]

Great to hear, David. I'll be interested to see what you guys come up with.

Comment 57

[Stephanie Sullivan Rewis]

David, just wondering (after reading the minutes) if what you're considering is this:

smfr: In webkit we use different behaviour for focus ring(?) than with
         outlines.
smfr: if outline style is auto, we fall into focus ring behaviour which
         does go around overflow content

Is this the implementation you're evaluating?

(Even though I may use outline for things that you might consider decorative, I do always set :focus styles for keyboard navigators — whether it's outline, or another more appropriate style choice.)

(In reply to David Baron [:dbaron] (needinfo? me; away Aug 28 - Sep 3) from comment #55)
> We had some discussion in today's CSS WG telecon; minutes should be
> available within a few days.

Comment 58

[Stephanie Sullivan Rewis]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #55)
> We had some discussion in today's CSS WG telecon; minutes should be
> available within a few days.

Hoping to hear more about this this week? Thoughts, David?

Comment 59

[Stephanie Sullivan Rewis]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #55)
> We had some discussion in today's CSS WG telecon; minutes should be
> available within a few days.

Hoping to hear more about this this week? Thoughts, David?

Comment 60

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

The minutes of that meeting are at http://lists.w3.org/Archives/Public/www-style/2013Aug/0441.html .


The testing I've done so far of WebKit's behavior with outline-style: auto seems reasonable to me so far except for its merging-across-lines behavior being dependent on actual overlap; I've tested it in these examples:

http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cstyle%3E%0Adiv%20%7B%20display%3Ainline%3B%20outline-style%3A%20auto%3B%20outline-color%3A%20fuchsia%3B%20%7D%0Aspan%20%7B%20display%3Ainline-block%3Bbackground%3Ayellow%3Bwidth%3A100px%3Bheight%3A100px%3Bbox-shadow%3A%2020px%2020px%3Bmargin-bottom%3A%2040px%3B%20border%3A%205px%20solid%20green%3B%20vertical-align%3A%20middle%3B%20%7D%0A%3C%2Fstyle%3E%0A%0A%3Cdiv%3Ehello%20%3Cspan%3E%3C%2Fspan%3E%3C%2Fdiv%3E

http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cstyle%3E%0Ap%20%7B%20width%3A%206em%3B%20line-height%3A%201%20%7D%0A%23outline%20%7B%20display%3Ainline%3B%20outline-style%3A%20auto%3B%20outline-color%3A%20fuchsia%3B%20%7D%0A%3C%2Fstyle%3E%0A%0A%3Cp%3EThis%20is%20%3Cspan%20id%3D%22outline%22%3Esome%20text%20with%3C%2Fspan%3E%20outline.%3C%2Fp%3E
vs.
http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cstyle%3E%0Ap%20%7B%20width%3A%206em%3B%20line-height%3A%201.5%20%7D%0A%23outline%20%7B%20display%3Ainline%3B%20outline-style%3A%20auto%3B%20outline-color%3A%20fuchsia%3B%20%7D%0A%3C%2Fstyle%3E%0A%0A%3Cp%3EThis%20is%20%3Cspan%20id%3D%22outline%22%3Esome%20text%20with%3C%2Fspan%3E%20outline.%3C%2Fp%3E

I'm inclined to think we should emulate that behavior, except perhaps with the merging happening even if there is some line separation.  That said, the merging part doesn't need to happen in this bug; it's independent.

That, in turn, would mean not taking the patch here for now, but instead taking the slightly more complicated path described in comment 8.

Comment 61

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

Attachment: patch 1: Rename ComputeOutlineAndEffectsRect to ComputeEffectsRect.

Comment 62

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

Attachment: patch 2: Remove always-true aStoreRectProperties parameter to ComputeEffectsRect.

Comment 63

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

Attachment: patch 3: Draw outline around the union of border boxes rather than the visual overflow area.

Comment 64

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

https://tbpl.mozilla.org/?tree=Try&rev=02ef29236ac5

Comment 65

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

Comment on attachment 811479 [details] [diff] [review]
patch 3:  Draw outline around the union of border boxes rather than the visual overflow area.

Needs work given try results.

Comment 66

[philippe (part-time)]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #60)
 
> The testing I've done so far of WebKit's behavior with outline-style: auto
> seems reasonable to me so far except for its merging-across-lines behavior
> being dependent on actual overlap;

I'm curious why don't seem to like that. With my designer hat on (and given the input in comment 60, 3rd link, I would not expect any merging. The 2 lines are far apart (due to line-height). After all, if I change your span to a a[href], the space that is not included inside the outline is not clickable, focusable, etc.

Like this
http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=2545

(Presto based Opera agrees with WebKit on this, assuming I change outline-style to 'solid', auto not being supported).

Comment 67

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

https://tbpl.mozilla.org/?tree=Try&rev=b7d8b28dcadc

Comment 68

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

Attachment: patch 3: Draw outline around the union of border boxes rather than the visual overflow area.

At the same time, move the handling of outlines on inlines that contain
blocks earlier, so that we factor it into the stored frame property (and
thus have the stored frame property) and so that we include it
accurately in the overflow calculation.

Comment 69

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

And I'll put a non-breaking space before the "T" in the reftest so that it doesn't overlap the border/outline at all, and that should fix the reftest orange.

Comment 70

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

Actually, padding makes more sense.

Comment 71

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

Comment on attachment 813391 [details] [diff] [review]
patch 3:  Draw outline around the union of border boxes rather than the visual overflow area.

Review of attachment 813391 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +6880,5 @@
> +    nsFrameList children = childLists.CurrentList();
> +    for (nsFrameList::Enumerator e(children); !e.AtEnd(); e.Next()) {
> +      nsIFrame* child = e.get();
> +      aUnion.UnionRectEdges(aUnion,
> +                            UnionBorderBoxes(child) + child->GetPosition());

Is it correct to ignore CSS transforms?

Is it correct to traverse SVG frames and text frames?

@@ +6988,5 @@
> +  if (outline->GetOutlineStyle() != NS_STYLE_BORDER_STYLE_NONE) {
> +    nscoord width;
> +    DebugOnly<bool> result = outline->GetOutlineWidth(width);
> +    NS_ASSERTION(result, "GetOutlineWidth had no cached outline width");
> +    if (width > 0) {

Move everything under width > 0 to a helper function, I think.

@@ +7014,5 @@
> +        innerRect = bounds;
> +        if (!bounds.IsEqualEdges(aOverflowAreas.VisualOverflow()) &&
> +            !bounds.IsEqualEdges(aOverflowAreas.ScrollableOverflow())) {
> +          UnionDescendantBorderBoxes(this, innerRect);
> +        }

Just call UnionBorderBoxes here.

Comment 72

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #71)
> Is it correct to ignore CSS transforms?

Oops, no.

> Is it correct to traverse SVG frames and text frames?

Hmmm.  Should I just use their visual overflow?  (What's the issue with text frames?)

> > +  if (outline->GetOutlineStyle() != NS_STYLE_BORDER_STYLE_NONE) {
> > +    nscoord width;
> > +    DebugOnly<bool> result = outline->GetOutlineWidth(width);
> > +    NS_ASSERTION(result, "GetOutlineWidth had no cached outline width");
> > +    if (width > 0) {
> 
> Move everything under width > 0 to a helper function, I think.

Will do.

> @@ +7014,5 @@
> > +        innerRect = bounds;
> > +        if (!bounds.IsEqualEdges(aOverflowAreas.VisualOverflow()) &&
> > +            !bounds.IsEqualEdges(aOverflowAreas.ScrollableOverflow())) {
> > +          UnionDescendantBorderBoxes(this, innerRect);
> > +        }
> 
> Just call UnionBorderBoxes here.

I can't because this's new size isn't set, so we have to use aNewSize rather than this->GetSize().  This split seemed the easiest way around that inconvenience, but I suppose I should comment it.

Comment 73

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

(In reply to David Baron [:dbaron] (needinfo? me) from comment #72)
> > Is it correct to traverse SVG frames and text frames?
> 
> Hmmm.  Should I just use their visual overflow?  (What's the issue with text
> frames?)

You can use their frame size. For SVG that's their visual bounds, which makes sense. The issue here is that these nodes don't technically have CSS border-boxes (AFAIK) so if the CSS spec is just going to talk about "union of border boxes" then these frames would not be included. So either we need to exclude them, or we need to make sure the spec explicitly includes them.

> I can't because this's new size isn't set, so we have to use aNewSize rather
> than this->GetSize().  This split seemed the easiest way around that
> inconvenience, but I suppose I should comment it.

Makes sense. Thanks.

Comment 74

[Stephanie Sullivan Rewis]

So if I have pseudo-elements on a parent that are negatively margined outside the parent—and I put an outline around the parent—does it go around the pseudo-elements? Or does it stay only with the parent? I'm a little confused by what you're planning as the resolution here.

Comment 75

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

(In reply to Stephanie Sullivan Rewis from comment #74)
> So if I have pseudo-elements on a parent that are negatively margined
> outside the parent—and I put an outline around the parent—does it go around
> the pseudo-elements?

yes

> Or does it stay only with the parent?

no

Comment 76

[Stephanie Sullivan Rewis]

Well, that's a real shame. 

Since pseudo-elements, for the most part, are used for decorative reasons (they're not in the DOM, and they're not read by most screen readers, so they're not appropriate for important content) outlines around the parent are generally meant to be that — around the parent — not the decorative pseudo-elements. They don't need the focus. (And if they did, I'd give it to them manually.)

So in reality, this won't actually be like the webkit implementation — since that leaves the outline around the parent where I want it. (Or are you saying that only the focus ring will go around the whole kit and kaboodle?)

(In reply to David Baron [:dbaron] (needinfo? me) from comment #75)
> (In reply to Stephanie Sullivan Rewis from comment #74)
> > So if I have pseudo-elements on a parent that are negatively margined
> > outside the parent—and I put an outline around the parent—does it go around
> > the pseudo-elements?
> 
> yes
> 
> > Or does it stay only with the parent?
> 
> no

Comment 77

[Stephanie Sullivan Rewis]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #75)

Hi David - We're nearly 5 years from when this bug was filed and 4 months from any activity that those of us monitoring this bug can see. This greatly affects our app — and clearly many other folks projects too. Is there any hope of a resolution along the lines of webkits implementation? If so, can we expect it any time soon? 

Cheers

Comment 78

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

I've been working on it this week (among other things); getting it right in the presence of transforms is a little complicated.

Comment 79

[Stephanie Sullivan Rewis]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #78)
> I've been working on it this week (among other things); getting it right in
> the presence of transforms is a little complicated.

I'm sure! Thanks for all your hard work, David... we're chomping at the bit.

Comment 80

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

try push:  https://tbpl.mozilla.org/?tree=Try&rev=57fde013644b

Comment 81

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

And all over again, since I forgot we disabled 10.7 tests:
https://tbpl.mozilla.org/?tree=Try&rev=88b126671794

Comment 82

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

And again, with clipping:
https://tbpl.mozilla.org/?tree=Try&rev=573624c864d2

Comment 83

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

And without a leak-debugging patch applied too:
https://tbpl.mozilla.org/?tree=Try&rev=c7833a68f325

Comment 84

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

Attachment: patch 3: Refactor a common pattern into a FrameMaintainsOverflow helper function.

Comment 85

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

Attachment: patch 4: Draw outline around the union of border boxes, SVG, and text, rather than the visual overflow area.

At the same time, move the handling of outlines on inlines that contain
blocks earlier, so that we factor it into the stored frame property (and
thus have the stored frame property) and so that we include it
accurately in the overflow calculation.

Comment 86

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

Comment on attachment 8376042 [details] [diff] [review]
patch 4:  Draw outline around the union of border boxes, SVG, and text, rather than the visual overflow area.

Review of attachment 8376042 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +6860,5 @@
> +  // that we might need to, and if the frame doesn't clip its overflow
> +  // anyway.
> +  const nsStyleDisplay* disp = aFrame->StyleDisplay();
> +  nsIAtom* fType = aFrame->GetType();
> +  if (!u.IsEqualEdges(aFrame->GetVisualOverflowRect()) &&

I don't think we should check VisualOverflowRect here. The ScrollableOverflowRect check should be enough.

@@ +6922,5 @@
> +ComputeAndIncludeOutlineArea(nsIFrame* aFrame, nsOverflowAreas& aOverflowAreas,
> +                             const nsSize& aNewSize)
> +{
> +  const nsStyleOutline* outline = aFrame->StyleOutline();
> +  if (outline->GetOutlineStyle() != NS_STYLE_BORDER_STYLE_NONE) {

invert this condition and do an early return to save indenting.

@@ +6926,5 @@
> +  if (outline->GetOutlineStyle() != NS_STYLE_BORDER_STYLE_NONE) {
> +    nscoord width;
> +    DebugOnly<bool> result = outline->GetOutlineWidth(width);
> +    NS_ASSERTION(result, "GetOutlineWidth had no cached outline width");
> +    if (width > 0) {

Same here.

Comment 87

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #86)
> > +  // that we might need to, and if the frame doesn't clip its overflow
> > +  // anyway.
> > +  const nsStyleDisplay* disp = aFrame->StyleDisplay();
> > +  nsIAtom* fType = aFrame->GetType();
> > +  if (!u.IsEqualEdges(aFrame->GetVisualOverflowRect()) &&
> 
> I don't think we should check VisualOverflowRect here. The
> ScrollableOverflowRect check should be enough.

Not if we ever include margins in scrollable overflow, which we probably should at some point.  So I'd prefer to leave that, if that's ok.

Comment 88

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

Sure OK

Comment 89

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

Actually, that try run hit some assertions on Windows; looks like my new assertion about the frame property being present doesn't hold for XUL tree pseudo-elements.  (Now that I think about that case, it's not at all a surprise.)

Comment 90

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

The assertion being:

[4040] ###!!! ASSERTION: we should have saved a frame property: 'Not Reached', file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\layout\base\nsCSSRendering.cpp, line 578

nsCSSRendering::PaintOutline(nsPresContext *,nsRenderingContext &,nsIFrame *,nsRect const &,nsRect const &,nsStyleContext *) [layout/base/nsCSSRendering.cpp:609]

nsTreeBodyFrame::PaintBackgroundLayer(nsStyleContext *,nsPresContext *,nsRenderingContext &,nsRect const &,nsRect const &) [layout/xul/tree/nsTreeBodyFrame.cpp:3929]

nsTreeBodyFrame::PaintRow(int,nsRect const &,nsPresContext *,nsRenderingContext &,nsRect const &,nsPoint) [layout/xul/tree/nsTreeBodyFrame.cpp:2982]

nsTreeBodyFrame::PaintTreeBody(nsRenderingContext &,nsRect const &,nsPoint) [layout/xul/tree/nsTreeBodyFrame.cpp:2869]

PaintTreeBody [layout/xul/tree/nsTreeBodyFrame.cpp:2796]

nsDisplayGeneric::Paint(nsDisplayListBuilder *,nsRenderingContext *) [layout/base/nsDisplayList.h:1736]

Comment 91

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

https://tbpl.mozilla.org/?tree=Try&rev=fbea6e6ea0b1

Comment 92

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

Attachment: patch to be folded into patch 4: Preserve existing behavior for XUL tree pseudos (and avoid assertions), pending later fix.

Comment 93

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ff86655c1d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/d659d5d5f45f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ebf78b46aae
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd1f8adbfecc

Comment 94

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

Backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1feb66af4ff
for:
REFTEST TEST-UNEXPECTED-FAIL | http://10.26.84.13:30054/tests/layout/reftests/bugs/402338-1.html | image comparison (==), max difference: 255, number of differing pixels: 716
REFTEST TEST-UNEXPECTED-FAIL | http://10.26.84.13:30054/tests/layout/reftests/bugs/412352-2.html | image comparison (==), max difference: 255, number of differing pixels: 486
on Android and B2G only.

Comment 95

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

Try push for what I hope will fix that:
https://tbpl.mozilla.org/?tree=Try&rev=170f4b584992

Comment 96

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1ff86655c1d0
https://hg.mozilla.org/mozilla-central/rev/d659d5d5f45f
https://hg.mozilla.org/mozilla-central/rev/8ebf78b46aae

Comment 97

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

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #95)
> Try push for what I hope will fix that:
> https://tbpl.mozilla.org/?tree=Try&rev=170f4b584992

That didn't help, but I think the additions did fix some real mistakes in the patch.

Some further data:
https://tbpl.mozilla.org/?tree=Try&rev=de4325e08a3a and https://tbpl.mozilla.org/?tree=Try&rev=68e24cb0ee26 (the latter is more useful) show that the test and reference rendering are correct prior to the patch, so the change the patch introduces is making the rendering of the references incorrect.

Comment 98

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

Oh, wait, it probably didn't help because I forgot to pass the new parameter at the caller.

Comment 99

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

I'm hoping these should be in good shape:
https://tbpl.mozilla.org/?tree=Try&rev=d9b952d23a0e
https://tbpl.mozilla.org/?tree=Try&rev=f31c0d65f9cf

Comment 100

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

Attachment: second patch to be folded into patch 4: Fix checking of bounds vs. overflow area.

Comment 101

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0f0eabea26

Comment 102

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

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #92)
> Created attachment 8376614 [details] [diff] [review]
> patch to be folded into patch 4:  Preserve existing behavior for XUL tree
> pseudos (and avoid assertions), pending later fix.

That later fix is in bug 434102.

Comment 104

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d0f0eabea26

Comment 106

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

I landed some additional tests that depend on bug 709014:
https://hg.mozilla.org/integration/mozilla-inbound/rev/beff43e0a138

Comment 107

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/beff43e0a138

Comment 108

[Stephanie Sullivan Rewis]

We're less than a week from being 5 years on this bug. Does the fact that bug #709014 is marked fixed, and Comment 104 says Resolved, mean this is actually happening? A bit confused by the further comments.

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #106)
> I landed some additional tests that depend on bug 709014:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/beff43e0a138

Comment 109

[Kevin Brosnan [Ex-Mozilla]]

This bug and the other bug you mention have been marked as fixed for Firefox 30 as of this comment. Barring regressions that are unacceptable or some sort of intermittent test failure from the checkins the fixes will be in Firefox 30. 

If one of the scenarios were to happen a backout comment would be placed in the bugs. The comment would have the rational for the backout with a hg mozilla-central url would be posted pointing to the code change.

In the future questions about the development process at Mozilla are best asked on the mailing lists such as https://lists.mozilla.org/listinfo/dev-platform it reaches a wider audience than the cc list of a bug and provides a history that is more easily searchable.

Comment 110

[Sylvestre Ledru [:Sylvestre]]

David, What you would write to describe this bug/features for the release notes?
"Fix: Outlines were drawn outside box-shadow" ?
Thanks

Comment 111

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

Who's the audience for the release notes and why would they want to know about this bug in particular?

(I'd probably say "box-shadow and other visual overflow, since there's quite a bit of other visual overflow, including text-shadow, sometimes text, and also outlines themselves.)

Comment 112

[Sylvestre Ledru [:Sylvestre]]

I am talking about these release notes: http://www.mozilla.org/en-US/mobile/28.0beta/releasenotes/
and I am asking because Jean-Yves tagged it "relnote-firefox: ?".

Thanks

Comment 113

[yiorsi]

This bug has for several years, finally repaired!

Comment 114

[yiorsi]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #111)
> Who's the audience for the release notes and why would they want to know
> about this bug in particular?
> 
> (I'd probably say "box-shadow and other visual overflow, since there's quite
> a bit of other visual overflow, including text-shadow, sometimes text, and
> also outlines themselves.)

This bug has for several years, finally repaired!

Comment 116

[Louis Lazaris]

Firefox is still rendering outlines outside of overflowing child elements. Is this the expected/correct behaviour? For example:

http://codepen.io/impressivewebs/pen/ybKrQY

Is there another bug filed specifically for overflowing elements, rather than shadows? I see "Patch 4" seems to be addressing this issue, but I don't see it fixed.