Closed Bug 315209 Opened 19 years ago Closed 4 years ago

outline should follow shape of border-radius (remove -moz-outline-radius)

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
87 Branch
Tracking Status
firefox87 --- fixed

People

(Reporter: alan.ogilvie, Assigned: emilio, Mentored)

References

(Depends on 1 open bug, Blocks 3 open bugs, )

Details

(4 keywords, Whiteboard: [layout:backlog:quality] [lang=c++])

Attachments

(6 files, 1 obsolete file)

User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051025 Firefox/1.5 Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051025 Firefox/1.5 It seems that if a box has a border radius (for rounding the corners) that if an outline is applied to the same box, the outline does not follow the defined radius. Resulting in a round-edged box with an outline that is rectangular. Not sure if this is a CSS-spec bug (W3C), or a rendering bug (Moz). On the example URL given - mouse over the main boxes on the page to see the effect. Reproducible: Always Steps to Reproduce: 1. Visit example URL 2. Mouse over the main white boxes on the page 3. Brown coloured outline should appear around the rounded edged boxes (border-radius) Actual Results: Outline does not follow border-radius. Expected Results: Outline should follow border-radius. (? or should it ?) Other browsers: Opera 8 doesn't support radius anyway, Safari 2.0.2 doesn't support radius, IE doesn't support [anything] radius - so not much to compare it with really. I suppose this is CSS3 related, although obviously the -moz-border-radius CSS extension also demonstrates the problem.
You can use -moz-outline-radius to get rounded outline borders.
(In reply to comment #1) > You can use -moz-outline-radius to get rounded outline borders. > Hmmm... ok, so that's the mozilla extension. So it's possible to achieve it. But should the outline follow the border shape if defined?
I think CSS allows this, see http://www.w3.org/TR/css3-ui/#outline1 > Outlines may be non-rectangular.
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Summary: CSS: radius defined box edges and outline combination - outline not following radius 'shape' → outline should follow shape of border-radius
Outlines should follow border-radius like box-shadow.
Attached file testcase (obsolete) (deleted) —
Keywords: testcase
(In reply to Daniel.S (mostly offline during the week) from comment #3) > I think CSS allows this, see http://www.w3.org/TR/css3-ui/#outline1 > > > Outlines may be non-rectangular. That sentence is longer in CSS2, see http://www.w3.org/TR/CSS2/ui.html#propdef-outline : > Outlines may be non-rectangular. For example, if the element is broken across several lines, the outline is the minimum outline that encloses all the element's boxes. In contrast to borders, the outline is not open at the line box's end or start, but is always fully connected if possible. I read this not as "outlines can be rounded, while borders can't", which is obviously false (the idea of the issue is that the outline should be rounded like the border), but that the outline follows the element's real shape closer, especially valid for inline elements spread over several lines.
Summary: outline should follow shape of border-radius → outline should follow shape of border-radius (remove -moz-outline-radius)
Blocks: unprefix
Attached file testcase (unprefixed) (deleted) —
Attachment #404342 - Attachment is obsolete: true
Priority: -- → P5
Priority: P5 → P3
CSS3 UI requires this behavior: https://www.w3.org/TR/css-ui-3/#ref-for-border-edge "To the extent that the outline follows the border edge, it should follow the border-radius curve." Updated URL accordingly. Old "URL" in this bug (http://www.kollektive.com/prodsandservs.asp) appears to be offline.
Depends on: 1479623
Depends on: 1609195
Whiteboard: [layout:backlog:quality]

We could prototype this behind a flag pretty easily. It'd be a matter of replacing the users of mOutlineRadius with the border-radius instead, and adding a pref.

So, steps:

  • Adding a layout.css.outline-follows-border-radius.enabled pref to StaticPrefList.yaml (probably enabled on nightly-only or disabled by default for now).
  • Add a comment here, saying that if we ever optimize this we need to account for outline too.
  • Instead of using mOutlineRadius here, check the pref and do something like:
const auto& radius = StaticPrefs::layout_css_outline_follows_border_radius_enabled()
    ? aStyle->StyleBorder()->mBorderRadius
    : ourOutline->mOutlineRadius;

// .. carry-on
  • Same here (maybe add a const StyleBorderRadius& GetOutlineRadius() const helper to nsDisplayOutline?

That'd be all for now, we wouldn't disable -moz-outline-radius for now, because we don't have prefs that disable css properties. The property would just become silently useless, and then when this ships we can remove it altogether.

Mentor: emilio
Whiteboard: [layout:backlog:quality] → [layout:backlog:quality] [lang=c++] [good-first-bug]
Keywords: good-first-bug

Oh, forgot a step of course. Write a reftest in layout/reftests/outline like:

test-pref(layout.css.outline-follows-border-radius.enabled,true) == border-radius.html outline-radius.html

Where the border-radius.html file uses border-radius and an outline, and the outline radius one uses -moz-outline-radius. That would check that the feature works :)

Whiteboard: [layout:backlog:quality] [lang=c++] [good-first-bug] → [layout:backlog:quality] [lang=c++]

is this issue still of interest ?

Attached image Screen Shot 2563-05-21 at 22.09.13.png (deleted) —

I don't think the approach you posted works quite right. i.e. it doesn't take into account the border thickness. Adding the outline-width onto its radius looks right to me. Is that what we want? Are there other scenarios to take into account?

EDIT: Also need to take into account outline-offset?

Ah, you're totally right of course. Other interesting scenario is what happens when the element is broken into lines or columns, etc. But I think our behavior there may not be ideal already.

Assignee: nobody → wjohnston2000
Status: NEW → ASSIGNED

Patch wfm, but needs reftests. I'm not sure the right way to do all the unit conversions/addition here, so looking for feedback first.

Attached file outline with border-radius (deleted) —

Another set of test cases with some different sizes, offsets, animations, etc.

Rendering borders on elements with columns seems broken in basically every browser.

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Assignee: wjohnston2000 → nobody
Status: ASSIGNED → NEW

Might still be useful to provide feedback here.

Flags: needinfo?(emilio)

I provided feedback on the phabricator revision a while ago, though maybe they were missed?

Yeah, the multicol case seems harder, but for now keeping our moz-outline-radius first to get something landed here sounds reasonable to me. We can flesh out the edge cases in other bug.

Flags: needinfo?(emilio)
Assignee: nobody → emilio
Status: NEW → ASSIGNED

This makes -moz-outline-radius a no-op, but keep it for now.

If/when we make this the default in release, we can remove it.

Depends on D104323

I was looking at related code as part of bug 1691064, so I think we should try to do this too.

Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/acf63bfce848 Remove useless nsCSSBorderRenderer::mDocument. r=mstange https://hg.mozilla.org/integration/autoland/rev/fbe159e50e82 Add a pref to make non-auto outline follow border radius. r=mstange
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch

I was given this to document but it looks like it is just in Nightly right now, is that correct?

Flags: needinfo?(emilio)
Blocks: 1694146

Yes, though I think we should turn it on everywhere, now that the merge is done. I filed bug 1694146 for that.

Flags: needinfo?(emilio)
QA Whiteboard: [qa-87b-p2]
No longer regressions: 1701910
Depends on: 1783629
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: