cmtalbert Reporter
	Description • 16 years ago

The specification: http://dev.w3.org/csswg/css3-background/#the-border-radius states that percentage values are not allowed in CSS border radius rules.  However, our implementation takes percentage values.  The specification for CSS 3 is still in flux, so we shouldn't tie ourselves too tightly too it, but we probably shouldn't veer away from it on a whim either.  This is a bug to decide what we'll do about this issue. 

I'll attach a testcase.

	Jonathan Griffin (:jgriffin)
	Comment 1 • 15 years ago

Attaching Clint's reftests for this issue, so they won't get lost, and can be modified (or not) depending on the outcome of this issue before being checked in.

	Zack Weinberg (:zwol) Assignee
	Comment 2 • 15 years ago

The current (Oct 14) revision of css3-background allows percentages, but says that a percentage for the vertical dimension is relative to the *height*; our implementation takes both percentages relative to the *width*.  I'll bring this up with the committee.

	David Baron :dbaron:
	Comment 3 • 15 years ago

(In reply to comment #2)
> The current (Oct 14) revision of css3-background allows percentages, but says
> that a percentage for the vertical dimension is relative to the *height*; our
> implementation takes both percentages relative to the *width*.  I'll bring this
> up with the committee.

This has been discussed extensively; it is as-intended, and makes it easier to create ovals.

	Zack Weinberg (:zwol) Assignee
	Comment 4 • 15 years ago

(In reply to comment #3)
> This has been discussed extensively; it is as-intended, and makes it easier to
> create ovals.

Ok (bug retitled accordingly, and I'll come up with a patch), but we're going to have to audit themes for breakage.  I see a large handful of -moz-border-radius: xx% in {toolkit,browser}/themes.

	Gordon P. Hemsley [:GPHemsley]
	Comment 5 • 15 years ago

I can't figure out whether I'm missing something or if there is a bug in the layout engine—and if there is a bug, whether that bug is this one or a separate one.

But attached is a testcase that I created that applies 'border-radius' styling to li elements with the 'display' values of 'inline', 'inline-block', and 'block', using both percentages and em values.

The way I understand it, all of the 1% borders should be equal and all of the 10% should be equal, given the same contents of the box. (The spec says the percentage is of the border box, which appears to me to be the same size regardless of what unit is used.)

My explanation may be unclear, but the testcase should speak for itself. If this represents a separate bug, let me know and I'll pop it out.

	Markus Stange [:mstange]
	Comment 6 • 15 years ago

(In reply to comment #5)
> The way I understand it, all of the 1% borders should be equal and all of the
> 10% should be equal, given the same contents of the box.

No, only given the same sizes of the box. But your display:block boxes are wider.

	Zack Weinberg (:zwol) Assignee
	Comment 7 • 14 years ago

Comment on attachment 385404 [details] [diff] [review]
reftests

these are not the right tests, since percentages are allowed now.

	Zack Weinberg (:zwol) Assignee
	Comment 8 • 14 years ago

Here's a patch for the code + some tests.

I have NOT attempted to fix up any of the existing uses of percent border-radius in the themes.

	Zack Weinberg (:zwol) Assignee
	Comment 9 • 14 years ago

Gah.  I have no idea how the change to nsXREDirProvider.cpp got into this patch; it belongs in a different patch in my queue.  Will fix before landing.

	David Baron :dbaron:
	Comment 10 • 14 years ago

(In reply to comment #8)
> I have NOT attempted to fix up any of the existing uses of percent
> border-radius in the themes.

Note that this means you're leaving it to me, the reviewer, to figure that out... which means that this is a relatively low priority in my review queue because it's a review that requires a lot of work (relative to the amount it took to write the patch).

	Zack Weinberg (:zwol) Assignee
	Comment 11 • 14 years ago

(In reply to comment #10)
> Note that this means you're leaving it to me, the reviewer, to figure that
> out... which means that this is a relatively low priority in my review queue
> because it's a review that requires a lot of work (relative to the amount it
> took to write the patch).

Really I would rather it was up to someone who works on themes normally.

Here's all the suspect uses of border-radius in the tree:  http://mxr.mozilla.org/mozilla-central/search?string=border-radius%3A+*[0-9]*%25&regexp=on&tree=mozilla-central

In #ux Gavin said he could at least point out what each of these was used for.

	David Baron :dbaron:
	Comment 12 • 14 years ago

Comment on attachment 455767 [details] [diff] [review]
patch

Exclude the nsXREDirProvider changes that don't belong here, this looks fine.  (I didn't look closely at the tests.)

I guess the best way forward on this is to land after we reopen after beta4, and announce it widely (and explicitly make sure that Dão and Gavin know).

So r=dbaron; please land asap after we reopen after beta4, or, if you can't, let me know.  (Hopefully it won't be when I'm unreachable, although it looks like it might if we slip the beta a few days...)

	Zack Weinberg (:zwol) Assignee
	Comment 13 • 14 years ago

Same as before, but with the spurious nsXREDirProvider changes removed.  I can land this tomorrow if it gets a+ed, but after tomorrow I won't have time until next week.

	Zack Weinberg (:zwol) Assignee
	Comment 14 • 14 years ago

I no longer work for Mozilla, I am deassigning myself from bugs I have no intention of working on as a volunteer.

	Zack Weinberg (:zwol) Assignee
	Comment 15 • 14 years ago

Oops, didn't mean to deassign myself from this one.  It's practically done, and I will ensure it gets in for the next beta.

	Zack Weinberg (:zwol) Assignee
	Comment 16 • 14 years ago

IT doesn't seem to be in a hurry to reactivate my Hg access, so I need checkin help.  (Already added to ride-along list on http://wiki.mozilla.org/LandingQueue .)

	Mark Banner (:standard8)
	Comment 17 • 14 years ago

This was checked in, but backed out again due to reftest failures:

http://hg.mozilla.org/mozilla-central/rev/f600448ae7db
http://hg.mozilla.org/mozilla-central/rev/c78c4fda1325

REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/mozilla-central_snowleopard_test-reftest/build/reftest/tests/layout/reftests/box-properties/outline-radius-percent-1.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/mozilla-central_snowleopard_test-reftest/build/reftest/tests/layout/reftests/bugs/364861-1.html |

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282205369.1282205940.25736.gz#err0

	Daniel Holbert [:dholbert]
	Comment 18 • 14 years ago

(In reply to comment #17)
> This was checked in, but backed out again due to reftest failures:

Attaching reftest-failure-log with image URLs for convenience.

I glanced this in reftest-analyzer.xhtml -- in both failures, the testcase's border-corners stick out further than the reference case's border-corners.

	Zack Weinberg (:zwol) Assignee
	Comment 19 • 14 years ago

... I'm not sure how I managed to miss those reftest failures, which were real (the tests depended on the old semantics of a single percentage).  Fixed now.  Sorry about that.

	David Baron :dbaron:
	Comment 20 • 14 years ago

Landed:
http://hg.mozilla.org/mozilla-central/rev/3d42ac41a283

It turns out it causes some reasonably obvious UI breakage in at least the addons manager and about:home.  I audited themes; patch resulting from that audit coming shortly.

	David Baron :dbaron:
	Comment 21 • 14 years ago

This changes:
 * a large number of 100% to 10000px, since these just want large equal numbers to be affected by the clamping rules so we end up with fully rounded ends.  (the new % rules make them unequal)
 * two 1% in about:home code to 5.6px, since they were 1% of a 560px width container

	David Baron :dbaron:
	Comment 22 • 14 years ago

Landed theme audit fixes:
http://hg.mozilla.org/mozilla-central/rev/72b14a58afc0

	Eric Shepherd [:sheppy]
	Comment 23 • 14 years ago

This was previously documented.

	David Baron :dbaron:
	Comment 24 • 14 years ago

I realized I forgot to audit for outline-radius as well.  There were two uses of percentage outline-radius in themes.

	David Baron :dbaron:
	Comment 25 • 14 years ago

outline theme fixes landed:
http://hg.mozilla.org/mozilla-central/rev/ebabfab36a6f

I realized today we also need to fix nsComputedDOMStyle, though.

	j.j.
	Comment 26 • 14 years ago

https://developer.mozilla.org/en/CSS/-moz-outline-radius updated also

	David Baron :dbaron:
	Comment 27 • 14 years ago

(In reply to comment #25)
> I realized today we also need to fix nsComputedDOMStyle, though.

I filed bug 595650 on this.