Closed Bug 471643 Opened 11 years ago Closed 9 years ago

-moz-border-radius vertical percentages should be relative to box height, not width

Categories

(Core :: Layout: Block and Inline, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: cmtalbert, Assigned: zwol)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(7 files, 2 obsolete files)

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.
Attached patch reftestsSplinter Review
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.
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.
(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.
(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.
Summary: -moz-border-radius allows percentage values → -moz-border-radius vertical percentages should be relative to box height, not width
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.
(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.
Comment on attachment 385404 [details] [diff] [review]
reftests

these are not the right tests, since percentages are allowed now.
Attachment #385404 - Flags: review-
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
Attachment #455767 - Flags: review?(dbaron)
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.
(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).
(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.
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...)
Attachment #455767 - Flags: review?(dbaron) → review+
Attached patch corrected patch (obsolete) — Splinter Review
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.
Attachment #455767 - Attachment is obsolete: true
Attachment #466916 - Flags: review+
Attachment #466916 - Flags: approval2.0?
Blocks: 451134
Attachment #466916 - Flags: approval2.0? → approval2.0+
I no longer work for Mozilla, I am deassigning myself from bugs I have no intention of working on as a volunteer.
Assignee: zackw → nobody
Status: ASSIGNED → NEW
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.
Assignee: nobody → zackw
Status: NEW → ASSIGNED
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 .)
Keywords: checkin-needed
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
Keywords: checkin-needed
(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.
... 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.
Attachment #466916 - Attachment is obsolete: true
Attachment #468551 - Flags: review+
Attachment #468551 - Flags: approval2.0?
blocking2.0: --- → beta6+
Attachment #468551 - Flags: approval2.0? → approval2.0+
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.
Attached patch theme fixesSplinter Review
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
Attachment #470834 - Flags: review?(dtownsend)
Attachment #470834 - Flags: review?(dtownsend) → review+
Landed theme audit fixes:
http://hg.mozilla.org/mozilla-central/rev/72b14a58afc0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
This was previously documented.
I realized I forgot to audit for outline-radius as well.  There were two uses of percentage outline-radius in themes.
Attachment #472300 - Flags: review?(dtownsend)
Attachment #472300 - Flags: review?(dtownsend) → review+
outline theme fixes landed:
http://hg.mozilla.org/mozilla-central/rev/ebabfab36a6f

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

I filed bug 595650 on this.
You need to log in before you can comment on or make changes to this bug.