Closed Bug 141715 Opened 21 years ago Closed 15 years ago

-moz-border-radius should round not bevel -moz-border-colors

Categories

(Core Graveyard :: GFX, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: neil, Unassigned)

References

Details

(Keywords: css-moz, helpwanted, testcase)

Attachments

(6 files, 2 obsolete files)

-moz-border-radius rounds borders, except if you use -moz-border-colors, in
which case it bevels them :-(
Attached file Test Case
This is as-designed.  -moz-border-colors was designed (by hyatt) to be used for
UI buttons, where the maximum border radius used is about 3px.  Making a
diagonal edge is sufficient for that radius and allows near-perfect imitation of
the color effects in real UI buttons.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → WONTFIX
I vote that this gets re-opened. It is a shame that such a useful feature
(-moz-border-colors) not be useable in cases where I want -moz-border-radius. 

For instance, I am trying to round the borders of large tabs, and the borders
are multiple pixels wide. It would suck to have to figure out how to surround my
tabs in multiple boxes so I can use -moz-border-radius. 

Another example, in our app my company has a number of rounded boxes that, in
our old release (based on 0.9.1), I had to use 3 boxes with 3 sets of
-moz-border-radius's to make a 3 pixel, gradiented, rounded border.
-moz-border-radius + -moz-border-colors would save me that work and look a lot
better. 

If this is by design, can someone point me to the source and I will try and
patch it myself?

It's in nsCSSRendering.cpp.  If you can fix it without:
 * slowing drawing down measurably, or
 * messing up the accuracy of the cases used in the UI
then the patch would be worth considering.  However, I suspect that's nearly
impossible to do.  I could be wrong, though.
*** Bug 200240 has been marked as a duplicate of this bug. ***
See bug 200240 for a way out:
If you define to boxes in each other with 1 pixel border (i.e. non-composite),
you can simulate the desired effect.

To fix the composite border with radius can thus be done by 
repeating PaintRoundedBorder for each composite border color, decrementing the
size by 1 for each color. This is probably the fastest way to do (beside special
hardware or OS calls).

Look at nsCSSRendering.cpp:
1650   // rounded version of the outline
1651   // check for any corner that is rounded
1652   for(i=0;i<4;i++){
1653     if(borderRadii[i] > 0 && !aBorderStyle.mBorderColors){
1654      
PaintRoundedBorder(aPresContext,aRenderingContext,aForFrame,aDirtyRect,aBorderArea,&aBorderStyle,nsnull,aStyleContext,aSkipSides,borderRadii,aGap,PR_FALSE);
1655       return;
1656     }
1657   }
1658 

Note, constructing boxes inside boxes (using XBL or XUL) is very slow, and was
the reason for existance of -moz-border-*-colors. Using the simple trick above,
it can then also be used for rounded borders.

Hoping that someone is willing to take the challenge, greetings Alfred
Can this bug be re-opened, and analyzed whether it is usefull
to include it in some release? The fix seems easy, while delivering
the nice round 'input' boxes, everybody now likes so much (see the new
themes for Phoenix). Right now, they are using fixed image-chunks, but
that's is not scaleable (for big fonts).
Re-opening as an enhancement request.
Severity: normal → enhancement
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Attached patch A tentative patch. (obsolete) — Splinter Review
This patch adds PaintCompositeRounderBorder functionality.
It should work. It does compile, but I have to rebuild
a whole tree, before I can test it.
As one can see from the patch, it only adds code, for the
specific Composite and Rounder border case, so it should not
impact the other code in any way.
Attached patch Working patch (obsolete) — Splinter Review
With this patch the test examples from this bug, and from
bug 200240 work correctly.

However, in the Modern theme it does create some undesired
effects in the rounded buttons, and the tabs.
But, it seems to be easily fixable.
Attachment #121567 - Attachment is obsolete: true
Final patch, for review and further testing
Attachment #121625 - Attachment is obsolete: true
This additional patch increases some radius values for the Modern
theme, to make it play nicer with the improve CompositeRounderBorder patch.
Note: the 'after' screenshots were taken with this patch also applied
Attachment #121663 - Attachment mime type: application/x-stuffit → application/zip
Attachment #121662 - Flags: superreview+
Attachment #121662 - Flags: review+
Attachment #121662 - Flags: superreview?(bzbarsky)
Attachment #121662 - Flags: review?(dbaron)
Attachment #121662 - Flags: superreview+
Attachment #121662 - Flags: review+
I'll try to get to this within the next two weeks, but that's about all I can
promise right now.
Comment on attachment 121662 [details] [diff] [review]
Patch from mozilla\layout\html\style\src

I'd think if you wanted to do this (and I'm not even convinced of the need for
the patch in the first place, so it's probably not a good idea to put too much
work into that until we agree on the need for it):
 * you'd want to remove the current code that draws the beveled borders
 * you'd want to avoid copying code (was some of the new code here copied from
elsewhere?), but instead refactor existing code.
Comment on attachment 121662 [details] [diff] [review]
Patch from mozilla\layout\html\style\src

Please ask me for sr once the reviewer (dbaron) is happy with the patch.
Attachment #121662 - Flags: superreview?(bz-bugspam)
Comment on attachment 121662 [details] [diff] [review]
Patch from mozilla\layout\html\style\src

Marking review- based on my previous comment, although feel free to re-request
if you think I was wrong, since I didn't look too closely.
Attachment #121662 - Flags: review?(dbaron) → review-
I agree with the review-, the code is not finished yet.
Just wanted to have some look at it from the experts.
The following things need to be done:
* Assess importance of this feature (only 2 votes, and 3 CC'ers)
* Figure out, how to prevent holes 
* Remove redundancy
  * Remove or 'bevel composite radius' or make it a new style?
  * Refactor code to reduce code
* More testing, also on other platforms

So, as soon I've got some sparetime to waste, and I
want it to waste it on this feature, I will have another
go it. So no promises yet.
note: please don't attach stuff as .zip unless the files are too large to attach
to bugzilla. (And if screenshots are too big, you should reencode them using a
more optimal file format!) Thanks.
Keywords: testcase
Keywords: css-moz
OS: Windows 95 → All
Hardware: PC → All
Keywords: helpwanted
Blocks: 368247
Fixed by patch for bug 368247.
Status: REOPENED → RESOLVED
Closed: 21 years ago15 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Let me re-open this bug, after the patch for bug 368247.

While the result is much better, running the test case (attachment #1 [details] [diff] [review]) still clearly shows a big difference between normal border and -moz-border-colors rounding...  The difference is so clear, that one can easily conclude that for -moz-border-colors radius drawing, the radius is two times as big as it should be.

See also bug 380601, bug 379505, bug 379436 and attachment 7 [details] (testcase 2).
Making therefor this bug blocking the border rewrite bug 368247.

Testcase 3 shows all border radius issues between background, border and border-colors drawing
Status: RESOLVED → REOPENED
No longer depends on: 368247
Resolution: FIXED → ---
Bug 379474 as another testcase showing the radius calculation problem.
Blocks: 379474, 380608
Assignee: dbaron → general
Status: REOPENED → NEW
Component: Style System (CSS) → GFX
Fixed by checked-in patch from bug 368247.
Status: NEW → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Depends on: spirograph
Well, not exactly, the background is still strange, but that's bug 382613. It will get fixed soon.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.