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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: testcase

testcase

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

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)

Comment 3

[Mats Palmgren (inactive)]

FWIW, the testcase paints garbage when resizing the window on Linux.

Comment 4

[Jesse Ruderman]

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.

Comment 5

[Mats Palmgren (inactive)]

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.

Comment 6

[Mats Palmgren (inactive)]

(filed bug 428278 for the expanding outline rect)

Comment 7

[Mats Palmgren (inactive)]

Attachment: Alternative patch, paint 'auto' as 'solid'

Comment 8

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

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?

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Nominating wanted-1.9.1 since there has been some progress by way of patches in this bug.

Comment 10

[Mats Palmgren (inactive)]

Attachment: part 1, Treat outline:auto as outline:solid (as suggested by spec)

Comment 11

[Mats Palmgren (inactive)]

Attachment: part 2, Add NS_THEME_FOCUS_BORDER to support drawing a native themed focus border

Comment 12

[Mats Palmgren (inactive)]

Attachment: part 3, Add NS_THEME_FOCUS_BORDER support for Cocoa widgets

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 4, Add NS_THEME_FOCUS_BORDER support for GTK widgets

I've tested the GTK2 part and it's working fine.  I don't have a GTK3
build so that part is untested.

Comment 14

[Mats Palmgren (inactive)]

Attachment: part 5, Add NS_THEME_FOCUS_BORDER support for Windows widgets

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.

Comment 15

[Mats Palmgren (inactive)]

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

Comment 16

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

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?

Comment 17

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

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 18

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

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?

Comment 19

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

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

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 21

[Mats Palmgren (inactive)]

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

Comment 22

[Mats Palmgren (inactive)]

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

Comment 23

[Markus Stange [:mstange]]

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

Comment 24

[Mats Palmgren (inactive)]

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

Comment 25

[Mats Palmgren (inactive)]

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

Comment 26

[Markus Stange [:mstange]]

Exactly.

Comment 27

[Mats Palmgren (inactive)]

Sure, we can do that in ComputeAndIncludeOutlineArea I think.

Comment 28

[Mats Palmgren (inactive)]

Attachment: part 2, Add NS_THEME_FOCUS_BORDER to support drawing a native themed focus border

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.

Comment 29

[Mats Palmgren (inactive)]

Attachment: part 4, Add NS_THEME_FOCUS_BORDER support for GTK widgets

I made GetWidgetOverflow return 'focus_width' when 'interior_focus'
is false.

Comment 30

[Mats Palmgren (inactive)]

Attachment: part 4, wdiff

Here's a wdiff version of part 4 to help reviewing.

Comment 31

[Mats Palmgren (inactive)]

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

Comment 32

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

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)

Comment 33

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

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

[Markus Stange [:mstange]]

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

Comment 35

[Markus Stange [:mstange]]

Roc's suggestion works too.

Comment 36

[Mats Palmgren (inactive)]

Attachment: part 2, Add NS_THEME_FOCUS_OUTLINE to support drawing a native themed focus outline

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

Comment 37

[Mats Palmgren (inactive)]

Attachment: part 3, Add NS_THEME_FOCUS_OUTLINE support for Cocoa widgets

s/border/outline/

Comment 38

[Mats Palmgren (inactive)]

Attachment: part 4, Add NS_THEME_FOCUS_OUTLINE support for GTK widgets

s/border/outline/

Comment 39

[Mats Palmgren (inactive)]

Attachment: part 5, Add NS_THEME_FOCUS_OUTLINE support for Windows widgets

s/border/outline/

Comment 40

[Markus Stange [:mstange]]

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?

Comment 41

[Mats Palmgren (inactive)]

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

Comment 42

[Markus Stange [:mstange]]

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.

Comment 43

[Mats Palmgren (inactive)]

Attachment: addendum - pass down inner rect

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

Comment 44

[Markus Stange [:mstange]]

Comment on attachment 8439973 [details] [diff] [review]
addendum - pass down inner rect

Thanks!

Comment 45

[Mats Palmgren (inactive)]

I added a basic reftest as well:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1d28cf0b805
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b66757843a
https://hg.mozilla.org/integration/mozilla-inbound/rev/dde12de3eedf
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ad397becd2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/99ddf198e0e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/eda22df589d2

Comment 46

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c1d28cf0b805
https://hg.mozilla.org/mozilla-central/rev/f0b66757843a
https://hg.mozilla.org/mozilla-central/rev/dde12de3eedf
https://hg.mozilla.org/mozilla-central/rev/3ad397becd2b
https://hg.mozilla.org/mozilla-central/rev/99ddf198e0e9
https://hg.mozilla.org/mozilla-central/rev/eda22df589d2

Comment 47

[Jesse Ruderman]

I still hit the assertions mentioned in comment 0, but I filed bug 1033348 rather than reopening this bug.