Closed Bug 427928 Opened 12 years ago Closed 5 years ago

"ASSERTION: Non-border-colors case with borderColorStyleCount < 1 or > 3" and "ASSERTION: Unhandled border style" with <table style="outline: auto;"></table>

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: gkw, Assigned: mats)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(7 files, 9 obsolete files)

101 bytes, text/html
Details
3.07 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.33 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.58 KB, patch
mstange
: review+
Details | Diff | Splinter Review
12.68 KB, patch
mats
: review+
Details | Diff | Splinter Review
7.23 KB, patch
mats
: review+
Details | Diff | Splinter Review
9.07 KB, patch
mstange
: review+
Details | Diff | Splinter Review
I get the following assertions when running the testcase on the latest nightly, Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008040812 Minefield/3.0pre:

###!!! ASSERTION: Unhandled border style!!: 'Not Reached', file /Users/skywalker/Desktop/Mozilla/cvs/mozilla/layout/base/nsCSSRendering.cpp, line 1998

###!!! ASSERTION: Non-border-colors case with borderColorStyleCount < 1 or > 3; what happened?: 'Error', file /Users/skywalker/Desktop/Mozilla/cvs/mozilla/layout/base/nsCSSRendering.cpp, line 2182
Blocks: stirtable
Attached file testcase
testcase
Component: Layout: Tables → Layout
OS: Mac OS X → All
QA Contact: traversal-range → layout
Hardware: PC → All
Attached patch Patch rev. 1 (obsolete) — Splinter Review
Not sure how we should handle outline-style:auto until we implement it
properly.  It's implemented in the style system but we haven't really
implemented the proper painting for it.
http://www.w3.org/TR/css3-ui/#outline-style0
I think the spec wants us to render it like the native focus outline
for the platform, which means the drawing should be done by the widget
theme code(?).

This patch treats it like 'none'. An alternative would be to treat it
like 'solid'.  David, what do you think we should do for Fx3?
(assuming there's no time to implement it the correct way)
Assignee: nobody → mats.palmgren
Status: NEW → ASSIGNED
Attachment #314802 - Flags: superreview?(dbaron)
Attachment #314802 - Flags: review?(dbaron)
FWIW, the testcase paints garbage when resizing the window on Linux.
I don't see garbage pixels on Mac, but I do see a square that expands slightly every time I resize the window in any direction.
Yeah, that's what I see on Linux too.  Since there's nothing in the
testcase to explain that I though of it as garbage, sorry for my
misleading description.  Maybe that's a separate bug actually.
(filed bug 428278 for the expanding outline rect)
I think PaintOutline should assert that the outline style is not 'none' rather than checking for it.  The assert should probably say "shouldn't have created nsDisplayOutline display item".

Handling 'auto' as 'none' is wrong; handling it as solid might be OK (since the spec says so), but I'm not sure why we'd want to handle it at all.  Why not just stop supporting it in the CSS parser?
Attachment #314802 - Flags: superreview?(dbaron)
Attachment #314802 - Flags: superreview-
Attachment #314802 - Flags: review?(dbaron)
Attachment #314802 - Flags: review-
Nominating wanted-1.9.1 since there has been some progress by way of patches in this bug.
Flags: wanted1.9.1?
Flags: wanted1.9.1? → wanted1.9.1-
Assignee: matspal → nobody
Attachment #314802 - Attachment is obsolete: true
Attachment #314818 - Attachment is obsolete: true
Attachment #8436371 - Flags: review?(roc)
Attachment #8436373 - Flags: review?(smichaud)
I've tested the GTK2 part and it's working fine.  I don't have a GTK3
build so that part is untested.
Attachment #8436374 - Flags: review?(roc)
Only tested on Win7 and it doesn't quite work as expected there.
It draws the right type and size of border, but it's doesn't have
the blue-ish color that focused borders should have.  Drawing it
using TFP_EDITBORDER_NOSCROLL and TFS_EDITBORDER_FOCUSED does
give it the right color but then it also draws the white background
and thus overwrites the content.  It looks like Windows ignores
the DTBG_OMITCONTENT option in this case.

I think we should land it anyway and file a bug on getting the
color right in a follow-up bug.  The current result seems better
than outline:solid.
Attachment #8436375 - Flags: review?(roc)
Comment on attachment 8436372 [details] [diff] [review]
part 2, Add NS_THEME_FOCUS_BORDER to support drawing a native themed focus border

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

::: layout/base/nsCSSRendering.cpp
@@ +838,5 @@
> +      theme->GetWidgetOverflow(aPresContext->DeviceContext(), aForFrame,
> +                               NS_THEME_FOCUS_BORDER, &dirtyRect);
> +      theme->DrawWidgetBackground(&aRenderingContext, aForFrame,
> +                                  NS_THEME_FOCUS_BORDER, drawing,
> +                                  dirtyRect);

Why are we calling GetWidgetOverflow here? Can't we just pass aDirtyRect to DrawWidgetBackground as the dirtyrect and pass innerRect for the widget rect?
Attachment #8436372 - Flags: review?(roc) → review-
Comment on attachment 8436372 [details] [diff] [review]
part 2, Add NS_THEME_FOCUS_BORDER to support drawing a native themed focus border

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

::: gfx/src/nsThemeConstants.h
@@ +18,5 @@
>  // like images (e.g. HTML <button> elements)
>  #define NS_THEME_BUTTON_BEVEL                              7
>  
> +// A themed focus border (for outline:auto)
> +#define NS_THEME_FOCUS_BORDER                              8

Why not call this NS_THEME_FOCUS_OUTLINE since that is more properly what it is?
Comment on attachment 8436374 [details] [diff] [review]
part 4, Add NS_THEME_FOCUS_BORDER support for GTK widgets

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

::: widget/gtk/gtk3drawing.c
@@ +758,5 @@
> +                         NULL);
> +    if (interior_focus) {
> +        *focus_h_width = XTHICKNESS(w->style) + focus_width;
> +        *focus_v_width = YTHICKNESS(w->style) + focus_width;
> +    } else {

I don't understand this. Shouldn't the focus border widths be zero in the interior_focus case, but non-zero when !interior_focus?
Attachment #8436374 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> Comment on attachment 8436372 [details] [diff] [review]
> part 2, Add NS_THEME_FOCUS_BORDER to support drawing a native themed focus
> border
> 
> Review of attachment 8436372 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsCSSRendering.cpp
> @@ +838,5 @@
> > +      theme->GetWidgetOverflow(aPresContext->DeviceContext(), aForFrame,
> > +                               NS_THEME_FOCUS_BORDER, &dirtyRect);
> > +      theme->DrawWidgetBackground(&aRenderingContext, aForFrame,
> > +                                  NS_THEME_FOCUS_BORDER, drawing,
> > +                                  dirtyRect);
> 
> Why are we calling GetWidgetOverflow here? Can't we just pass aDirtyRect to
> DrawWidgetBackground as the dirtyrect and pass innerRect for the widget rect?

I guess the question is, what is the "widget area" (what normally corresponds to the frame's border-box) be for a NS_THEME_FOCUS_BORDER/OUTLINE? Should the outline be drawn entirely outside the widget area? I think so, but this should be documented somewhere.
Comment on attachment 8436373 [details] [diff] [review]
part 3, Add NS_THEME_FOCUS_BORDER support for Cocoa widgets

Markus is probably the best one to review this.  He knows this code better than I do.
Attachment #8436373 - Flags: review?(smichaud) → review?(mstange)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> ::: layout/base/nsCSSRendering.cpp
> @@ +838,5 @@
> > +      theme->GetWidgetOverflow(aPresContext->DeviceContext(), aForFrame,
> > +                               NS_THEME_FOCUS_BORDER, &dirtyRect);
> > +      theme->DrawWidgetBackground(&aRenderingContext, aForFrame,
> > +                                  NS_THEME_FOCUS_BORDER, drawing,
> > +                                  dirtyRect);
> 
> Why are we calling GetWidgetOverflow here? Can't we just pass aDirtyRect to
> DrawWidgetBackground as the dirtyrect and pass innerRect for the widget rect?

It's to ensure that the dirtyRect is large enough to include
the width of a native focus border.  The frame isn't aware of
this overflow so it's not included in its overflow areas.
The overflow areas for the frame is calculated based on the
CSS outline-width, which isn't really used for outline:auto
since it draws a native size focus border regardless of the
specified outline-width (this is intentional).
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> Why not call this NS_THEME_FOCUS_OUTLINE since that is more properly what it
> is?

That's what I called it at first actually, then I changed it because
I thought nsITheme and its constants are "widget things" and on the 
widget layer (and in native APIs) the focus ring is a border, which
can be "interior" or not, but it's not called an outline there.

I don't feel strongly about the name though, either of
NS_THEME_FOCUS_BORDER/OUTLINE/RING is fine with me.
I'm happy to change it if you have a preference.
(In reply to Mats Palmgren (:mats) from comment #21)
> The overflow areas for the frame is calculated based on the
> CSS outline-width

Can you change that? Respecting outline-width by changing the area inside the focus outline doesn't make all that much sense.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> ::: widget/gtk/gtk3drawing.c
> @@ +758,5 @@
> > +                         NULL);
> > +    if (interior_focus) {
> > +        *focus_h_width = XTHICKNESS(w->style) + focus_width;
> > +        *focus_v_width = YTHICKNESS(w->style) + focus_width;
> > +    } else {
> 
> I don't understand this. Shouldn't the focus border widths be zero in the
> interior_focus case, but non-zero when !interior_focus?

No, if 'interior_focus' is true the native code will draw a border
*inside* the area you give it and since we want to use it as an
outline we need to inflate our frame rect by that size.

If 'interior_focus' is false, I assume the native code will draw
the border around the area you give it, so zero should give the
correct result in that case. (I haven't tested this since I don't
have any GTK themes where 'interior_focus' false, so I'm just
guessing that's what it does.)
(In reply to Markus Stange [:mstange] from comment #23)
> (In reply to Mats Palmgren (:mats) from comment #21)
> > The overflow areas for the frame is calculated based on the
> > CSS outline-width
> 
> Can you change that? Respecting outline-width by changing the area inside
> the focus outline doesn't make all that much sense.

Do you mean ignoring outline-width for the purpose of calculating
the frame's overflow areas and instead use the result of
GetWidgetOverflow in the outline:auto case?
Sure, we can do that in ComputeAndIncludeOutlineArea I think.
Added code to update the frame's overflow area with the output from
GetWidgetOverflow rather than outline-width, as Markus suggested.
Which means we can now pass aDirtyRect as is.
Attachment #8436372 - Attachment is obsolete: true
Attachment #8438358 - Flags: review?(roc)
I made GetWidgetOverflow return 'focus_width' when 'interior_focus'
is false.
Attachment #8436374 - Attachment is obsolete: true
Attachment #8438365 - Flags: review?(roc)
Attached patch part 4, wdiff (obsolete) — Splinter Review
Here's a wdiff version of part 4 to help reviewing.
Comment on attachment 8438358 [details] [diff] [review]
part 2, Add NS_THEME_FOCUS_BORDER to support drawing a native themed focus border

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

r+ with that

::: gfx/src/nsThemeConstants.h
@@ +18,5 @@
>  // like images (e.g. HTML <button> elements)
>  #define NS_THEME_BUTTON_BEVEL                              7
>  
> +// A themed focus border (for outline:auto)
> +#define NS_THEME_FOCUS_BORDER                              8

Let's call this NS_THEME_FOCUS_OUTLINE. It's definitely an outline from Gecko's point of view. Whether the platform calls it an "outline" or "border" is rather arbitrary (and different platforms could legitimately call it either).

::: layout/generic/nsFrame.cpp
@@ +7131,5 @@
> +    nsITheme* theme = presContext->GetTheme();
> +    if (theme && theme->ThemeSupportsWidget(presContext, aFrame,
> +                                            NS_THEME_FOCUS_BORDER)) {
> +      nsRect r;
> +      theme->GetWidgetOverflow(presContext->DeviceContext(), aFrame,

I think we should do what GetWidgetOverflow expects and set r to have size aNewSize here. It makes the code below a little more complicated, but a lot less confusing!

@@ +7133,5 @@
> +                                            NS_THEME_FOCUS_BORDER)) {
> +      nsRect r;
> +      theme->GetWidgetOverflow(presContext->DeviceContext(), aFrame,
> +                               NS_THEME_FOCUS_BORDER, &r);
> +      if (r.x == 0 && r.y == 0 && r.width == 0 && r.height == 0) {

Then this would check r.IsEqualInterior(nsRect(nsPoint(0,0), aNewSize))

@@ +7137,5 @@
> +      if (r.x == 0 && r.y == 0 && r.width == 0 && r.height == 0) {
> +        return;
> +      }
> +      outlineMargin.top = std::max(-r.y + offset, 0);
> +      outlineMargin.right = std::max(r.width + offset, 0);

Then this would be std::max(r.XMost() - aFrame->GetSize().width + offset, 0)
Attachment #8438358 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> I think we should do what GetWidgetOverflow expects and set r to have size
> aNewSize here. It makes the code below a little more complicated, but a lot
> less confusing!

Actually it would be much better to just treat "auto" as width 0, go all the way down until after we've inflated outerRect, and then if we have outline:auto call GetWidgetOverflow on outerRect.
Comment on attachment 8438358 [details] [diff] [review]
part 2, Add NS_THEME_FOCUS_BORDER to support drawing a native themed focus border

Looking at this code:

-  nscoord offset = outline->mOutlineOffset;
-  nscoord inflateBy = std::max(width + offset, 0);
-
-  // Keep this code (and the storing of properties just above) in
-  // sync with GetOutlineInnerRect in nsCSSRendering.cpp.
   nsRect outerRect(innerRect);
-  outerRect.Inflate(inflateBy, inflateBy);

I think it would have been simpler to instead write:

nsRect outerRect = innerRect.Union(nsRect(innerRect).Inflate(width + offset));

You could do something similar in the new code and get rid of the confusing min/max expressions.

And I think the rect you pass to both GetWidgetOverflow and to DrawWidgetBackground should be nsRect(innerRect).Inflate(offset).
Roc's suggestion works too.
I've incorporated the suggestions from both of you.
I changed s/border/outline/ everywhere, moved the calculations
for outline:auto to the end (the reason I had it at the top
was that "outline:auto; outline-width:0" might result in
falling back to the non-auto path for platforms that don't
support it and thus take the width==0 early return --
I guess it's not worth optimizing this edge case though).
I removed the std::max stuff and use innerRect.Union(...)
instead (which is a tiny bit slower I guess, but clearer).
Attachment #8438358 - Attachment is obsolete: true
Attachment #8438626 - Flags: review?(roc)
s/border/outline/
Attachment #8436373 - Attachment is obsolete: true
Attachment #8436373 - Flags: review?(mstange)
Attachment #8438627 - Flags: review?(mstange)
s/border/outline/
Attachment #8438365 - Attachment is obsolete: true
Attachment #8438367 - Attachment is obsolete: true
Attachment #8438630 - Flags: review+
s/border/outline/
Attachment #8436375 - Attachment is obsolete: true
Attachment #8438631 - Flags: review+
Comment on attachment 8438627 [details] [diff] [review]
part 3, Add NS_THEME_FOCUS_OUTLINE support for Cocoa widgets

So you're adding the focus outline width before calling DrawWidgetBackground, and then you're removing it again in DrawFocusOutline. Why not pass the inner rect to DrawWidgetBackground? Does that not map that well to the platform APIs on the other platforms?
(In reply to Markus Stange [:mstange] from comment #40)
> Comment on attachment 8438627 [details] [diff] [review]
> part 3, Add NS_THEME_FOCUS_OUTLINE support for Cocoa widgets
> 
> So you're adding the focus outline width before calling
> DrawWidgetBackground, and then you're removing it again in DrawFocusOutline.
> Why not pass the inner rect to DrawWidgetBackground? Does that not map that
> well to the platform APIs on the other platforms?

The current code seems to suit GTK and Windows better since the rect
can be used as is.  We could inflate the rect in the widget layer
instead on those platforms of course.
I think that would be better. It would at least be more consistent with -moz-appearance because there the rect that gets passed as aRect to DrawWidgetBackground is the same as the one passed to GetWidgetOverflow, i.e. the frame's border rect without overflow.
No problem.  This change might be easier to review as a separate
patch on top of the previous.  (I'll fold it into the relevant
patches before landing).

https://tbpl.mozilla.org/?tree=Try&rev=6929fe3e1e1e
Attachment #8439973 - Flags: review?(mstange)
Comment on attachment 8439973 [details] [diff] [review]
addendum - pass down inner rect

Thanks!
Attachment #8439973 - Flags: review?(mstange) → review+
Attachment #8438627 - Flags: review?(mstange) → review+
Depends on: 1025874
Depends on: 1026641
Blocks: 1031664
I still hit the assertions mentioned in comment 0, but I filed bug 1033348 rather than reopening this bug.
No longer blocks: 1031664
Depends on: 1031662
You need to log in before you can comment on or make changes to this bug.