251498 - Implement outline-offset

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Aaron Leventhal]

The problem with using -moz-outline with -moz-outline-radius is that the rounded
borders cut through the frame.

We can implement outline-offset to get around this problem:
http://www.w3.org/TR/2004/CR-css3-ui-20040511/#outline-offset

Comment 1

[Aaron Leventhal]

I think it's reasonable to allow a negative outline-offset. This would allow
focus to appear correctly for buttons and tab panels.

Comment 2

[Aaron Leventhal]

Attachment: Implement outline offset. Negative offsets work as long as outline.width >0 && outline.height > 0

Comment 3

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

Comment on attachment 162184 [details] [diff] [review]
Implement outline offset. Negative offsets work as long as outline.width >0  && outline.height > 0

   nsStyleSides  mOutlineRadius;    // [reset] length, percent, inherit
									       
						// (top=topLeft,
right=topRight, bottom=bottomRight, left=bottomLeft)

+  nsStyleCoord  mOutlineOffset;   // [reset] length
+
+  PRBool GetOutlineOffset(nscoord& aOffset) const

Lose the unnecessary blank line.

+      nscoord offset;
+      outline->GetOutlineOffset(offset);
+      r.Inflate(width + offset, width + offset);
+      if (r.width < 0 || r.height < 0) {
+	 return aOverflowRect; // Protect from negative outline offset that's
too large
+      }

This is not right. You should never allow the overflow area to shrink because
of an outline with negative outline-offset; consider the situation where frame
A has an overflowing child B and also has outline with negative offset, you
could lose the information about the overflow of B. I think what you want to do
here is Inflate r by PR_MAX(0, width + offset) in each direction.

+static nsRect ComputeOutlineRect(const nsIFrame* aFrame, PRBool* aAnyOutline,
+				  const nsRect& aOverflowRect);
+

This is wrong and shouldn't be here.

Changing the meaning of the rect passed to PaintOutline isn't a good idea,
certainly not if it's not documented...

I think you can avoid doing ComputeOutlineRect and leave the PaintOutline
interface as is, and compute the correct outline position like this:

  // The overflowArea edge is the outside

Comment 4

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

ugh, I lost the code. It looked like this:

nsRect outside(*overflowArea);
nsRect inside(outside);
if (outlineWidth + outlineOffset >= 0) {
  // the overflow area is exactly the outside edge of the outline
  inside.Deflate(outlineWidth, outlineWidth);
} else {
  // the overflow area is exactly the rectangle containing the frame and its
  // children; we can compute the outline directionly
  inside.Deflate(-outlineOffset, -outlineOffset);
  outside = inside;
  outside.Inflate(outlineWidth, outlineWidth);
}

Comment 5

[Aaron Leventhal]

Attachment: Address roc's comments

Comment 6

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

Comment on attachment 163316 [details] [diff] [review]
Address roc's comments

+      if (offset < 0) {
+	 offset = 0;
+      }

This isn't right. You need to compute PR_MAX(0, width + offset).

+  // get the offset for our outline
+  aOutlineStyle.GetOutlineOffset(offset);
   nsRect outside(*overflowArea);
+  outside.Inflate(offset, offset); // Note, it's intentional that offset < 0
causes a deflation
   nsRect inside(outside);
   inside.Deflate(width, width);
+  if (inside.width <= 0 || inside.height <= 0) {
+    return;  // Protect against outline-offset that is too small
+  }

This isn't equivalent to what I suggested and it's not right either. This will
effectively give you double the offset (because we add the offset once when
computing the overflow area and again here when rendering).

Comment 7

[Aaron Leventhal]

Attachment: My bad -- uses roc's code for inside/outside computation, but also checks for negative outline sizes

Comment 8

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

Comment on attachment 163328 [details] [diff] [review]
My bad -- uses roc's code for inside/outside computation, but also checks for negative outline sizes

+    // children; we can compute the outline directionally

"directly"

Comment 9

[Aaron Leventhal]

Checking in content/base/src/nsRuleNode.cpp;
/cvsroot/mozilla/content/base/src/nsRuleNode.cpp,v  <--  nsRuleNode.cpp
new revision: 1.137; previous revision: 1.136
done
Checking in content/html/style/src/nsCSSParser.cpp;
/cvsroot/mozilla/content/html/style/src/nsCSSParser.cpp,v  <--  nsCSSParser.cpp
new revision: 3.289; previous revision: 3.288
done
Checking in content/html/style/src/nsCSSStruct.cpp;
/cvsroot/mozilla/content/html/style/src/nsCSSStruct.cpp,v  <--  nsCSSStruct.cpp
new revision: 3.133; previous revision: 3.132
done
Checking in content/html/style/src/nsCSSStruct.h;
/cvsroot/mozilla/content/html/style/src/nsCSSStruct.h,v  <--  nsCSSStruct.h
new revision: 3.36; previous revision: 3.35
done
Checking in content/html/style/src/nsComputedDOMStyle.cpp;
/cvsroot/mozilla/content/html/style/src/nsComputedDOMStyle.cpp,v  <-- 
nsComputedDOMStyle.cpp
new revision: 1.131; previous revision: 1.130
done
Checking in content/html/style/src/nsComputedDOMStyle.h;
/cvsroot/mozilla/content/html/style/src/nsComputedDOMStyle.h,v  <-- 
nsComputedDOMStyle.h
new revision: 1.24; previous revision: 1.23
done
Checking in content/shared/public/nsCSSPropList.h;
/cvsroot/mozilla/content/shared/public/nsCSSPropList.h,v  <--  nsCSSPropList.h
new revision: 3.57; previous revision: 3.56
done
Checking in content/shared/public/nsStyleStruct.h;
/cvsroot/mozilla/content/shared/public/nsStyleStruct.h,v  <--  nsStyleStruct.h
new revision: 3.76; previous revision: 3.75
done
Checking in content/shared/src/nsStyleStruct.cpp;
/cvsroot/mozilla/content/shared/src/nsStyleStruct.cpp,v  <--  nsStyleStruct.cpp
new revision: 3.107; previous revision: 3.106
done
Checking in dom/public/idl/css/nsIDOMCSS2Properties.idl;
/cvsroot/mozilla/dom/public/idl/css/nsIDOMCSS2Properties.idl,v  <-- 
nsIDOMCSS2Properties.idl
new revision: 1.25; previous revision: 1.24
done
Checking in layout/html/base/src/nsFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.523; previous revision: 3.522
done
Checking in layout/html/style/src/nsCSSRendering.cpp;
/cvsroot/mozilla/layout/html/style/src/nsCSSRendering.cpp,v  <--  nsCSSRendering.cpp
new revision: 3.254; previous revision: 3.253
done

Comment 10

[[e-mail]]

Excellent! Fixes my theme up nicely, except for trees :-/

Comment 11

[[e-mail]]

Ah, for treerows border works just as well as outline used to :-)

Comment 12

[Boris Zbarsky [:bzbarsky]]

So... the CSSParser changes checked in here are pretty wrong, imo.  See bug 281972.

Once that's fixed, the nsRuleNode changes are wrong as well (outline-offset
should be an LH, not an LEH, and should not pass a keyword table), the changes
to nsStyleStruct make no sense (since outline-offset is just a length there is
no need to cache it, etc), and the nsComputedDOMStyle changes are wrong (there
should be no enumerated unit).

Comment 13

[Boris Zbarsky [:bzbarsky]]

And please, if you decide to copy-paste CSS code in the future, at least make
sure to copy-paste code for a property that takes the same exact set of values?

Comment 14

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

I didn't check against the spec whether it took a keyword ... and in this case
there is no other property that takes "length, inherit".