outline should follow shape of border-radius (remove -moz-outline-radius)
Categories
(Core :: Layout, enhancement, P3)
Tracking
()
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)
Comment 1•19 years ago
|
||
Reporter | ||
Comment 2•19 years ago
|
||
Comment 4•15 years ago
|
||
Comment 5•15 years ago
|
||
Updated•15 years ago
|
Comment 6•12 years ago
|
||
Comment 7•10 years ago
|
||
Updated•10 years ago
|
Updated•10 years ago
|
Comment 9•9 years ago
|
||
Comment 10•8 years ago
|
||
Updated•8 years ago
|
Updated•8 years ago
|
Comment 11•6 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 12•5 years ago
|
||
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 toStaticPrefList.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 tonsDisplayOutline
?
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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 13•5 years ago
|
||
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 :)
Updated•5 years ago
|
Comment 14•5 years ago
|
||
is this issue still of interest ?
Comment 15•5 years ago
|
||
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?
Assignee | ||
Comment 16•5 years ago
|
||
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•5 years ago
|
||
Updated•5 years ago
|
Comment 18•5 years ago
|
||
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•5 years ago
|
||
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•4 years ago
|
||
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 | ||
Comment 22•4 years ago
|
||
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.
Assignee | ||
Comment 23•4 years ago
|
||
Drive-by cleanup.
Updated•4 years ago
|
Assignee | ||
Comment 24•4 years ago
|
||
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
Assignee | ||
Comment 25•4 years ago
|
||
I was looking at related code as part of bug 1691064, so I think we should try to do this too.
Comment 26•4 years ago
|
||
Comment 27•4 years ago
|
||
Comment 28•4 years ago
|
||
bugherder |
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•4 years ago
|
||
I was given this to document but it looks like it is just in Nightly right now, is that correct?
Assignee | ||
Comment 30•4 years ago
|
||
Yes, though I think we should turn it on everywhere, now that the merge is done. I filed bug 1694146 for that.
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Description
•