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

Status: RESOLVED FIXED

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

Description

[cmtalbert]

Attachment: Testcase - all the divs should have square borders

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.

Comment 1

[Jonathan Griffin (:jgriffin)]

Attachment: reftests

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.

Comment 2

[Zack Weinberg (:zwol)]

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.

Comment 3

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

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

Comment 4

[Zack Weinberg (:zwol)]

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

Comment 5

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Testcase for 'border-radius' on "display: inline" and "display: inline-block" elements

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.

Comment 6

[Markus Stange [:mstange]]

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

[Zack Weinberg (:zwol)]

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

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

Comment 8

[Zack Weinberg (:zwol)]

Attachment: patch

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.

Comment 9

[Zack Weinberg (:zwol)]

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.

Comment 10

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

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

Comment 11

[Zack Weinberg (:zwol)]

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

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

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

Comment 13

[Zack Weinberg (:zwol)]

Attachment: corrected patch

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.

Comment 14

[Zack Weinberg (:zwol)]

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

Comment 15

[Zack Weinberg (:zwol)]

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.

Comment 16

[Zack Weinberg (:zwol)]

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

Comment 17

[Mark Banner (:standard8)]

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

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: reftest failure log, from first attempted landing

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

Comment 19

[Zack Weinberg (:zwol)]

Attachment: patch with reftests fixed

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

Comment 20

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

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.

Comment 21

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

Attachment: theme fixes

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

Comment 22

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

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

Comment 23

[Eric Shepherd [:sheppy]]

This was previously documented.

Comment 24

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

Attachment: theme fixes for -moz-outline-radius

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

Comment 25

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

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

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

Comment 26

[j.j.]

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

Comment 27

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

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

I filed bug 595650 on this.