Closed
Bug 471643
Opened 16 years ago
Closed 14 years ago
-moz-border-radius vertical percentages should be relative to box height, not width
Categories
(Core :: Layout: Block and Inline, defect)
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)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
zwol
:
review-
|
Details | Diff | Splinter Review |
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
zwol
:
review+
dbaron
:
approval2.0+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
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•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.
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.
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.
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.
Summary: -moz-border-radius allows percentage values → -moz-border-radius vertical percentages should be relative to box height, not width
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.
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.
Assignee | ||
Updated•15 years ago
|
Blocks: border-radius
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.
Attachment #385404 -
Flags: review-
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.
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.
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).
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®exp=on&tree=mozilla-central In #ux Gavin said he could at least point out what each of these was used for.
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...)
Attachment #455767 -
Flags: review?(dbaron) → review+
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.
Attachment #455767 -
Attachment is obsolete: true
Attachment #466916 -
Flags: review+
Attachment #466916 -
Flags: approval2.0?
Updated•14 years ago
|
Attachment #466916 -
Flags: approval2.0? → approval2.0+
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.
Assignee: zackw → nobody
Status: ASSIGNED → NEW
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.
Assignee: nobody → zackw
Status: NEW → ASSIGNED
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 .)
Keywords: checkin-needed
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
Keywords: checkin-needed
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.
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.
Attachment #466916 -
Attachment is obsolete: true
Attachment #468551 -
Flags: review+
Attachment #468551 -
Flags: approval2.0?
Updated•14 years ago
|
blocking2.0: --- → beta6+
Updated•14 years ago
|
Attachment #468551 -
Flags: approval2.0? → approval2.0+
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.
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
Attachment #470834 -
Flags: review?(dtownsend)
Updated•14 years ago
|
Attachment #470834 -
Flags: review?(dtownsend) → review+
Updated•14 years ago
|
Keywords: dev-doc-needed
Comment 22•14 years ago
|
||
Landed theme audit fixes: http://hg.mozilla.org/mozilla-central/rev/72b14a58afc0
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Comment 23•14 years ago
|
||
This was previously documented.
Keywords: dev-doc-needed → dev-doc-complete
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.
Attachment #472300 -
Flags: review?(dtownsend)
Updated•14 years ago
|
Attachment #472300 -
Flags: review?(dtownsend) → review+
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.
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•