Last Comment Bug 480888 - outlines are drawn outside (i.e., expanded by) box-shadow and other visual overflow
: outlines are drawn outside (i.e., expanded by) box-shadow and other visual ov...
Status: RESOLVED FIXED
[parity-webkit][parity-opera][parity-ie]
: testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal with 26 votes (vote)
: mozilla30
Assigned To: David Baron :dbaron: ⌚️UTC-10
:
: Jet Villegas (:jet)
Mentors:
: 287596 503719 599652 624732 674137 697899 782345 839797 987507 (view as bug list)
Depends on: 1064199 542595 978620
Blocks: 687311 287596 697899 824302 951440
  Show dependency treegraph
 
Reported: 2009-03-01 19:29 PST by Lucas Sanders
Modified: 2014-09-08 06:28 PDT (History)
49 users (show)
dbaron: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
30+


Attachments
simplified test case (533 bytes, text/html)
2009-03-01 19:30 PST, Lucas Sanders
no flags Details
crossbrowser testcase (409 bytes, text/html)
2010-09-04 16:55 PDT, j.j.
no flags Details
WIP patch for #480888 (2.08 KB, patch)
2012-06-07 02:31 PDT, Tom Lee
no flags Details | Diff | Splinter Review
WIP #2 -- rendering outline around overflow instead of the display rect (2.41 KB, patch)
2013-04-28 20:54 PDT, Tom Lee
no flags Details | Diff | Splinter Review
WIP #3 -- slight fix for WIP #2 (2.39 KB, patch)
2013-04-29 01:08 PDT, Tom Lee
dbaron: feedback+
Details | Diff | Splinter Review
ALT #1 -- render outline ignoring overflow (1.88 KB, patch)
2013-04-29 01:29 PDT, Tom Lee
dbaron: feedback+
Details | Diff | Splinter Review
patch 1: Rename ComputeOutlineAndEffectsRect to ComputeEffectsRect. (4.98 KB, patch)
2013-09-27 23:53 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 2: Remove always-true aStoreRectProperties parameter to ComputeEffectsRect. (4.18 KB, patch)
2013-09-27 23:54 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 3: Draw outline around the union of border boxes rather than the visual overflow area. (8.67 KB, patch)
2013-09-27 23:54 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch 3: Draw outline around the union of border boxes rather than the visual overflow area. (12.66 KB, patch)
2013-10-02 17:19 PDT, David Baron :dbaron: ⌚️UTC-10
dbaron: review-
Details | Diff | Splinter Review
patch 3: Refactor a common pattern into a FrameMaintainsOverflow helper function. (4.18 KB, patch)
2014-02-13 20:36 PST, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 4: Draw outline around the union of border boxes, SVG, and text, rather than the visual overflow area. (20.54 KB, patch)
2014-02-13 20:37 PST, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch to be folded into patch 4: Preserve existing behavior for XUL tree pseudos (and avoid assertions), pending later fix. (1.94 KB, patch)
2014-02-14 16:58 PST, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
second patch to be folded into patch 4: Fix checking of bounds vs. overflow area. (7.84 KB, patch)
2014-02-15 16:25 PST, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review

Description Lucas Sanders 2009-03-01 19:29:33 PST
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 2009-03-01 19:30:30 PST
Created attachment 364840 [details]
simplified test case
Comment 2 Emil Ivanov 2009-08-30 09:28:53 PDT
Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090829 Minefield/3.7a1pre
Comment 3 Emil Ivanov 2009-08-30 09:29:53 PDT
*** Bug 503719 has been marked as a duplicate of this bug. ***
Comment 4 Jon Raasch 2010-03-04 13:51:26 PST
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. 2010-09-04 16:55:23 PDT
Created attachment 472208 [details]
crossbrowser testcase
Comment 6 matthias koplenig 2011-07-21 02:57:30 PDT
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 7 Mats Palmgren (:mats) 2011-07-25 18:58:57 PDT
*** Bug 674137 has been marked as a duplicate of this bug. ***
Comment 8 David Baron :dbaron: ⌚️UTC-10 2011-08-27 09:52:58 PDT
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 9 David Baron :dbaron: ⌚️UTC-10 2011-08-27 09:53:59 PDT
*** Bug 599652 has been marked as a duplicate of this bug. ***
Comment 10 Stephanie Sullivan Rewis 2011-09-18 12:59:36 PDT
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 2011-10-26 01:10:26 PDT
I confirm this very annoying bug, I hope it could be solved quickly.
Comment 12 Palle Zingmark 2012-02-23 08:11:16 PST
I second Fabien Ménage comment and hope it can be solved soon.

poc: http://jsfiddle.net/kolombiken/QRwqK/
Comment 13 Ida Franceen 2012-02-23 08:18:01 PST
+1 on Palle and Fabien. It's a truly annoying bug.
Comment 14 Stephanie Sullivan Rewis 2012-02-23 21:33:08 PST
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 2012-02-23 22:01:38 PST
(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-10 2012-02-23 22:21:53 PST
No, please do not discuss.  This is a bug.
Comment 17 Fabien Ménager 2012-02-24 06:49:14 PST
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 2012-04-13 14:06:04 PDT
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 2012-04-13 15:52:47 PDT
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 2012-06-02 23:19:29 PDT
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 2012-06-07 02:30:01 PDT
(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 2012-06-07 02:31:11 PDT
Created attachment 630900 [details] [diff] [review]
WIP patch for #480888
Comment 23 hsablonniere 2012-07-20 05:50:30 PDT
What's the status on this one? Just discovered it today :-(
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2012-08-21 22:03:44 PDT
*** Bug 782345 has been marked as a duplicate of this bug. ***
Comment 25 David Baron :dbaron: ⌚️UTC-10 2012-09-06 13:38:10 PDT
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 2012-09-12 23:06:07 PDT
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 27 David Baron :dbaron: ⌚️UTC-10 2012-09-15 13:39:22 PDT
*** Bug 624732 has been marked as a duplicate of this bug. ***
Comment 28 dominikmarczuk 2012-10-02 07:32:21 PDT
Is this ever going to be resolved?
http://jsfiddle.net/nNSjY/
Comment 29 tim.erwin 2012-10-02 07:37:13 PDT
stop talking about it and just fix the damn bug
Comment 30 David Baron :dbaron: ⌚️UTC-10 2013-02-09 13:53:49 PST
*** Bug 839797 has been marked as a duplicate of this bug. ***
Comment 31 Keith Pepin 2013-04-22 11:06:05 PDT
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] (on vacation until Dec 5) 2013-04-24 12:32:11 PDT
Tom, could you confirm that you're still working on this?
Comment 33 Tom Lee 2013-04-24 23:31:40 PDT
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 2013-04-25 11:30:04 PDT
(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 2013-04-27 23:21:41 PDT
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-10 2013-04-28 07:19:50 PDT
(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) 2013-04-28 17:36:48 PDT
(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 2013-04-28 20:48:07 PDT
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 2013-04-28 20:54:56 PDT
Created attachment 742908 [details] [diff] [review]
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 2013-04-29 01:08:22 PDT
Created attachment 742954 [details] [diff] [review]
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 2013-04-29 01:29:45 PDT
Created attachment 742958 [details] [diff] [review]
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 2013-05-03 07:21:51 PDT
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-10 2013-05-04 12:14:17 PDT
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-10 2013-05-04 12:15:11 PDT
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-10 2013-05-04 12:18:28 PDT
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 2013-05-04 15:59:25 PDT
(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 2013-05-04 16:01:41 PDT
(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 2013-05-04 16:17:33 PDT
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 2013-05-06 06:23:42 PDT
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 2013-05-09 19:55:01 PDT
Flagging this as needinfo? RE: your query to www-style, David.
Comment 51 pondjon 2013-05-10 06:41:50 PDT
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-10 2013-05-27 21:48:25 PDT
Posted http://lists.w3.org/Archives/Public/www-style/2013May/0668.html
Comment 53 pondjon 2013-05-29 07:06:18 PDT
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 2013-06-17 23:03:34 PDT
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-10 2013-08-21 10:03:00 PDT
We had some discussion in today's CSS WG telecon; minutes should be available within a few days.
Comment 56 Stephanie Sullivan Rewis 2013-08-26 16:07:39 PDT
Great to hear, David. I'll be interested to see what you guys come up with.
Comment 57 Stephanie Sullivan Rewis 2013-08-28 14:43:18 PDT
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 2013-09-23 08:35:14 PDT
(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 2013-09-23 08:35:54 PDT
(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-10 2013-09-23 13:02:30 PDT
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-10 2013-09-27 23:53:54 PDT
Created attachment 811477 [details] [diff] [review]
patch 1:  Rename ComputeOutlineAndEffectsRect to ComputeEffectsRect.
Comment 62 David Baron :dbaron: ⌚️UTC-10 2013-09-27 23:54:19 PDT
Created attachment 811478 [details] [diff] [review]
patch 2:  Remove always-true aStoreRectProperties parameter to ComputeEffectsRect.
Comment 63 David Baron :dbaron: ⌚️UTC-10 2013-09-27 23:54:36 PDT
Created attachment 811479 [details] [diff] [review]
patch 3:  Draw outline around the union of border boxes rather than the visual overflow area.
Comment 64 David Baron :dbaron: ⌚️UTC-10 2013-09-27 23:57:01 PDT
https://tbpl.mozilla.org/?tree=Try&rev=02ef29236ac5
Comment 65 David Baron :dbaron: ⌚️UTC-10 2013-09-28 14:45:18 PDT
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) 2013-09-28 18:33:00 PDT
(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-10 2013-10-02 17:18:27 PDT
https://tbpl.mozilla.org/?tree=Try&rev=b7d8b28dcadc
Comment 68 David Baron :dbaron: ⌚️UTC-10 2013-10-02 17:19:39 PDT
Created attachment 813391 [details] [diff] [review]
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-10 2013-10-02 18:47:17 PDT
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-10 2013-10-02 18:49:29 PDT
Actually, padding makes more sense.
Comment 71 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-02 20:09:26 PDT
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-10 2013-10-02 22:20:34 PDT
(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) 2013-10-03 04:19:22 PDT
(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 2013-10-08 00:47:13 PDT
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-10 2013-10-08 12:43:29 PDT
(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 2013-10-08 22:06:08 PDT
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 2014-02-12 18:51:10 PST
(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-10 2014-02-12 18:59:41 PST
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 2014-02-12 19:08:48 PST
(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-10 2014-02-13 12:48:06 PST
try push:  https://tbpl.mozilla.org/?tree=Try&rev=57fde013644b
Comment 81 David Baron :dbaron: ⌚️UTC-10 2014-02-13 14:36:44 PST
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-10 2014-02-13 17:26:42 PST
And again, with clipping:
https://tbpl.mozilla.org/?tree=Try&rev=573624c864d2
Comment 83 David Baron :dbaron: ⌚️UTC-10 2014-02-13 18:16:32 PST
And without a leak-debugging patch applied too:
https://tbpl.mozilla.org/?tree=Try&rev=c7833a68f325
Comment 84 David Baron :dbaron: ⌚️UTC-10 2014-02-13 20:36:59 PST
Created attachment 8376040 [details] [diff] [review]
patch 3:  Refactor a common pattern into a FrameMaintainsOverflow helper function.
Comment 85 David Baron :dbaron: ⌚️UTC-10 2014-02-13 20:37:35 PST
Created attachment 8376042 [details] [diff] [review]
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) 2014-02-13 20:51:39 PST
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-10 2014-02-13 21:22:58 PST
(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) 2014-02-13 23:11:04 PST
Sure OK
Comment 89 David Baron :dbaron: ⌚️UTC-10 2014-02-13 23:12:07 PST
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-10 2014-02-14 10:16:29 PST
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-10 2014-02-14 10:48:43 PST
https://tbpl.mozilla.org/?tree=Try&rev=fbea6e6ea0b1
Comment 92 David Baron :dbaron: ⌚️UTC-10 2014-02-14 16:58:17 PST
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.
Comment 94 David Baron :dbaron: ⌚️UTC-10 2014-02-14 23:47:00 PST
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-10 2014-02-15 00:26:58 PST
Try push for what I hope will fix that:
https://tbpl.mozilla.org/?tree=Try&rev=170f4b584992
Comment 97 David Baron :dbaron: ⌚️UTC-10 2014-02-15 14:47:08 PST
(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-10 2014-02-15 15:00:17 PST
Oh, wait, it probably didn't help because I forgot to pass the new parameter at the caller.
Comment 99 David Baron :dbaron: ⌚️UTC-10 2014-02-15 15:06:33 PST
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-10 2014-02-15 16:25:31 PST
Created attachment 8376772 [details] [diff] [review]
second patch to be folded into patch 4: Fix checking of bounds vs. overflow area.
Comment 101 David Baron :dbaron: ⌚️UTC-10 2014-02-16 16:08:14 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0f0eabea26
Comment 102 David Baron :dbaron: ⌚️UTC-10 2014-02-16 22:38:05 PST
(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 103 David Baron :dbaron: ⌚️UTC-10 2014-02-16 23:07:48 PST
*** Bug 697899 has been marked as a duplicate of this bug. ***
Comment 104 Carsten Book [:Tomcat] 2014-02-17 03:54:38 PST
https://hg.mozilla.org/mozilla-central/rev/4d0f0eabea26
Comment 105 David Baron :dbaron: ⌚️UTC-10 2014-02-17 08:35:33 PST
*** Bug 287596 has been marked as a duplicate of this bug. ***
Comment 106 David Baron :dbaron: ⌚️UTC-10 2014-02-17 20:11:48 PST
I landed some additional tests that depend on bug 709014:
https://hg.mozilla.org/integration/mozilla-inbound/rev/beff43e0a138
Comment 107 Carsten Book [:Tomcat] 2014-02-18 05:03:42 PST
https://hg.mozilla.org/mozilla-central/rev/beff43e0a138
Comment 108 Stephanie Sullivan Rewis 2014-02-24 21:18:04 PST
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 [:kbrosnan] 2014-02-24 22:29:50 PST
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] 2014-03-05 02:39:03 PST
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-10 2014-03-05 18:18:29 PST
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] 2014-03-05 23:45:55 PST
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 2014-03-09 19:35:41 PDT
This bug has for several years, finally repaired!
Comment 114 yiorsi 2014-03-09 19:36:19 PDT
(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 115 Alice0775 White 2014-03-25 08:50:48 PDT
*** Bug 987507 has been marked as a duplicate of this bug. ***

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