Closed
Bug 427928
Opened 17 years ago
Closed 10 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)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: gkw, Assigned: MatsPalmgren_bugz)
References
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
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
7.23 KB,
patch
|
MatsPalmgren_bugz
:
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
Reporter | ||
Comment 1•17 years ago
|
||
testcase
Assignee | ||
Updated•17 years ago
|
Component: Layout: Tables → Layout
OS: Mac OS X → All
QA Contact: traversal-range → layout
Hardware: PC → All
Assignee | ||
Comment 2•17 years ago
|
||
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)
Assignee | ||
Comment 3•17 years ago
|
||
FWIW, the testcase paints garbage when resizing the window on Linux.
Comment 4•17 years ago
|
||
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.
Assignee | ||
Comment 5•17 years ago
|
||
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.
Assignee | ||
Comment 6•17 years ago
|
||
(filed bug 428278 for the expanding outline rect)
Assignee | ||
Comment 7•17 years ago
|
||
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-
Reporter | ||
Comment 9•16 years ago
|
||
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 | ||
Updated•12 years ago
|
Assignee: matspal → nobody
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #314802 -
Attachment is obsolete: true
Attachment #314818 -
Attachment is obsolete: true
Attachment #8436371 -
Flags: review?(roc)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8436372 -
Flags: review?(roc)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8436373 -
Flags: review?(smichaud)
Assignee | ||
Comment 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8436371 -
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]:
-----------------------------------------------------------------
::: 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 20•10 years ago
|
||
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)
Assignee | ||
Comment 21•10 years ago
|
||
(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).
Assignee | ||
Comment 22•10 years ago
|
||
(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•10 years ago
|
||
(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.
Assignee | ||
Comment 24•10 years ago
|
||
(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.)
Assignee | ||
Comment 25•10 years ago
|
||
(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•10 years ago
|
||
Exactly.
Assignee | ||
Comment 27•10 years ago
|
||
Sure, we can do that in ComputeAndIncludeOutlineArea I think.
Assignee | ||
Comment 28•10 years ago
|
||
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)
Assignee | ||
Comment 29•10 years ago
|
||
I made GetWidgetOverflow return 'focus_width' when 'interior_focus'
is false.
Attachment #8436374 -
Attachment is obsolete: true
Attachment #8438365 -
Flags: review?(roc)
Assignee | ||
Comment 30•10 years ago
|
||
Here's a wdiff version of part 4 to help reviewing.
Assignee | ||
Comment 31•10 years ago
|
||
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+
Attachment #8438365 -
Flags: review?(roc) → review+
Attachment #8436375 -
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 34•10 years ago
|
||
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•10 years ago
|
||
Roc's suggestion works too.
Assignee | ||
Comment 36•10 years ago
|
||
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)
Assignee | ||
Comment 37•10 years ago
|
||
s/border/outline/
Attachment #8436373 -
Attachment is obsolete: true
Attachment #8436373 -
Flags: review?(mstange)
Attachment #8438627 -
Flags: review?(mstange)
Assignee | ||
Comment 38•10 years ago
|
||
s/border/outline/
Attachment #8438365 -
Attachment is obsolete: true
Attachment #8438367 -
Attachment is obsolete: true
Attachment #8438630 -
Flags: review+
Assignee | ||
Comment 39•10 years ago
|
||
s/border/outline/
Attachment #8436375 -
Attachment is obsolete: true
Attachment #8438631 -
Flags: review+
Attachment #8438626 -
Flags: review?(roc) → review+
Comment 40•10 years ago
|
||
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?
Assignee | ||
Comment 41•10 years ago
|
||
(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•10 years ago
|
||
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.
Assignee | ||
Comment 43•10 years ago
|
||
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 44•10 years ago
|
||
Comment on attachment 8439973 [details] [diff] [review]
addendum - pass down inner rect
Thanks!
Attachment #8439973 -
Flags: review?(mstange) → review+
Updated•10 years ago
|
Attachment #8438627 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 45•10 years ago
|
||
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
Flags: in-testsuite+
Comment 46•10 years ago
|
||
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
Assignee: nobody → mats
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 47•10 years ago
|
||
I still hit the assertions mentioned in comment 0, but I filed bug 1033348 rather than reopening this bug.
You need to log in
before you can comment on or make changes to this bug.
Description
•