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

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Alan Ogilvie]

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.

Comment 1

[Martijn Wargers (dead)]

You can use -moz-outline-radius to get rounded outline borders.

Comment 2

[Alan Ogilvie]

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

Comment 3

[Daniel.S]

I think CSS allows this, see http://www.w3.org/TR/css3-ui/#outline1

> Outlines may be non-rectangular.

Comment 4

[Emil Ivanov]

Outlines should follow border-radius like box-shadow.

Comment 5

[Emil Ivanov]

Attachment: testcase

Comment 6

[Sergiu Dumitriu]

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

Comment 7

[Albert Scheiner [:alberts]]

see also bug 593717

Comment 9

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/moz-outline-radius-will-be-removed/

Comment 10

[j.j.]

Attachment: testcase (unprefixed)

Comment 11

[Tantek Çelik]

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.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

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

Comment 14

[Abdallah Afify]

is this issue still of interest ?

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Screen Shot 2563-05-21 at 22.09.13.png

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?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Bug 315209 - Force outline radius to follow border-radius. r=emilio

Comment 18

[Wesley Johnston (:wesj)]

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.

Comment 19

[Wesley Johnston (:wesj)]

Attachment: outline with border-radius

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

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

Comment 20

[Release mgmt bot [:suhaib / :marco/ :calixte]]

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

Comment 21

[David Baron :dbaron:]

Might still be useful to provide feedback here.

Comment 22

[Emilio Cobos Álvarez (: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.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 315209 - Remove useless nsCSSBorderRenderer::mDocument. r=mstange

Drive-by cleanup.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 315209 - Add a pref to make non-auto outline follow border radius. r=mstange

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

Comment 25

[Emilio Cobos Álvarez (:emilio)]

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6997113c2a78
Tweak a fuzzy annotation.

Comment 28

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/acf63bfce848
https://hg.mozilla.org/mozilla-central/rev/fbe159e50e82
https://hg.mozilla.org/mozilla-central/rev/6997113c2a78

Comment 29

[Rachel Andrew]

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

Comment 30

[Emilio Cobos Álvarez (:emilio)]

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