CSS Outline and border-radius Topic is solved

Discussions about the development and maturation of the platform code (UXP).
Warning: may contain highly-technical topics.

Moderators: trava90, athenian200

User avatar
RealityRipple
Keeps coming back
Keeps coming back
Posts: 763
Joined: 2018-05-17, 02:34
Location: Los Berros Canyon, California
Contact:

CSS Outline and border-radius

Unread post by RealityRipple » 2024-10-09, 00:15

The spec on outline was altered a bit ago to say it should shape to the border edge, and thus be affected by border-radius. bug #315209 covers the changes, but I wanted to check one thing first. Does the Drive-by cleanup apply to us? It looks like it's no longer expecting the possibility of a document-less entry, getting context from mPresContext->Document() instead. Is this going to differ for us because of our non-document contexts?

User avatar
Moonchild
Pale Moon guru
Pale Moon guru
Posts: 36627
Joined: 2011-08-28, 17:27
Location: Motala, SE
Contact:

Re: CSS Outline and border-radius

Unread post by Moonchild » 2024-10-09, 09:29

I honestly don't know off the top of my head and would have to research if we have document-less contexts to contend with. It does seem to just be to be able to report issues with the border radius though, so I'm not sure it harms anything. The BZ bug (as often the case) doesn't at all give a reason this drive-by-cleanup is safe, anywhere -- and this has caused sec bugs in the past when assumptions were incorrect and "cleanup" happened.

We should probably keep this in place for now. It may be safe to remove, but it may not be, and I'd prefer not to have a crash case because mPresContext is null.
{{This headspace for lease}}
"Seek wisdom, not knowledge. Knowledge is of the past; wisdom is of the future." -- Native American proverb
"Linux makes everything difficult." -- Lyceus Anubite

User avatar
RealityRipple
Keeps coming back
Keeps coming back
Posts: 763
Joined: 2018-05-17, 02:34
Location: Los Berros Canyon, California
Contact:

Re: CSS Outline and border-radius

Unread post by RealityRipple » 2024-10-09, 16:47

Sounds good. Also, should we stick with the way FF implemented (with a toggle pref, keeping -moz-outline-radius's code in-tact), should we just do the border-radius changes without the pref, or should we pull out all the outline-radius code in favor of border-radius? I kind of don't think the pref is necessary, but we might wanna keep outline-radius, just for any possible backward compatibility problems.


Edit: I'm gonna tinker with this for a few. Looking through the browser's code, there's a few uses of -moz-outline-radius in the default theme and elsewhere, which likely means other themes also use it, and I'd like to include a bit better fallback than the existing code supports, so those don't break.

OK, in fact, there's some uses of -moz-outline-radius that has a different value from the border-radius in some OS-matching theme code... Soooo, -moz-outline-radius as primary, with border-radius as the fallback if none of the outline-radius corners are non-zero?

Also, I think they double-negatived themselves in the IsInvisibleInRect() function... They returned !HasNonZeroCorner in the new HasRadius function, which they then also !'d. I guess with "Non" in "HasNonZeroCorner" it's optionally a triple negative?

User avatar
Moonchild
Pale Moon guru
Pale Moon guru
Posts: 36627
Joined: 2011-08-28, 17:27
Location: Motala, SE
Contact:

Re: CSS Outline and border-radius

Unread post by Moonchild » 2024-10-09, 18:36

RealityRipple wrote:
2024-10-09, 16:47
-moz-outline-radius that has a different value from the border-radius in some OS-matching theme code... Soooo, -moz-outline-radius as primary, with border-radius as the fallback if none of the outline-radius corners are non-zero?
Yes. Let the prefixed outline version override the border-radius prop if both are defined.
RealityRipple wrote:
2024-10-09, 16:47
Also, I think they double-negatived themselves in the IsInvisibleInRect() function... They returned !HasNonZeroCorner in the new HasRadius function, which they then also !'d. I guess with "Non" in "HasNonZeroCorner" it's optionally a triple negative?
Sometimes there's whole stacks of negations in Moz code (drives me nuts...) Carefully check if their logic is correct; maybe double-check with later Firefox code to see if it was changed after the fact in an unreferenced bug?
{{This headspace for lease}}
"Seek wisdom, not knowledge. Knowledge is of the past; wisdom is of the future." -- Native American proverb
"Linux makes everything difficult." -- Lyceus Anubite

User avatar
RealityRipple
Keeps coming back
Keeps coming back
Posts: 763
Joined: 2018-05-17, 02:34
Location: Los Berros Canyon, California
Contact:

Re: CSS Outline and border-radius

Unread post by RealityRipple » 2024-10-09, 19:26

Nope, looks like they haven't caught it. https://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.cpp#4050

FF's original code was:

Code: Select all

  if (borderBox.Contains(aRect) &&
      !nsLayoutUtils::HasNonZeroCorner(outline->mOutlineRadius)) {
    if (outline->mOutlineOffset._0 >= 0.0f) {
The change was to:

Code: Select all

bool nsDisplayOutline::HasRadius() const {
  const auto& radius =
      StaticPrefs::layout_css_outline_follows_border_radius_enabled()
          ? mFrame->StyleBorder()->mBorderRadius
          : mFrame->StyleOutline()->mOutlineRadius;
  return !nsLayoutUtils::HasNonZeroCorner(radius);
}

...
  if (borderBox.Contains(aRect) && !HasRadius() &&
      outline->mOutlineOffset.ToCSSPixels() >= 0.0f) {
Their new code reads:

Code: Select all

bool nsDisplayOutline::HasRadius() const {
  const auto& radius = mFrame->StyleBorder()->mBorderRadius;
  return !nsLayoutUtils::HasNonZeroCorner(radius);
}

...
  if (borderBox.Contains(aRect) && !HasRadius() &&
      outline->mOutlineOffset.ToCSSPixels() >= 0.0f) {
You can kinda see how it happened.

User avatar
Moonchild
Pale Moon guru
Pale Moon guru
Posts: 36627
Joined: 2011-08-28, 17:27
Location: Motala, SE
Contact:

Re: CSS Outline and border-radius

Unread post by Moonchild » 2024-10-09, 19:41

Yup, so Firefox would not draw outlines properly then if I understand correctly, omitting them if there are rounded corners and the rect is clipped...
I guess it is a corner case... only happens if the straight edge outlines are clipped but the corners would still be visible because rounded?
{{This headspace for lease}}
"Seek wisdom, not knowledge. Knowledge is of the past; wisdom is of the future." -- Native American proverb
"Linux makes everything difficult." -- Lyceus Anubite

User avatar
RealityRipple
Keeps coming back
Keeps coming back
Posts: 763
Joined: 2018-05-17, 02:34
Location: Los Berros Canyon, California
Contact:

Re: CSS Outline and border-radius

Unread post by RealityRipple » 2024-10-09, 19:46

Speaking of corner cases, we have one ourselves:

We're treating unset and zero as identical (calling HasNonZeroCorner), but if someone wanted a border-radius of some value (other than zero on at least one corner) but manually includes a -moz-outline-radius that is specifically all zeroes, it will use the border-radius value. I doubt there's any practical implementations of this on any website or theme, but... should we make a "HasNonUnsetCorner" function and use that instead? And if so, what should we count as "fallback to border-radius" besides eStyleUnit_Null? "normal"? "auto"? "none"?
Last edited by RealityRipple on 2024-10-09, 20:10, edited 1 time in total.

User avatar
Moonchild
Pale Moon guru
Pale Moon guru
Posts: 36627
Joined: 2011-08-28, 17:27
Location: Motala, SE
Contact:

Re: CSS Outline and border-radius

Unread post by Moonchild » 2024-10-09, 20:10

I think we can safely ignore that corner case. It makes no logical sense to explicitly set an outline radius to zero (i.e. no radius) while setting a border radius...
But if you want to, we can be explicit for that case and add the extra check method. Just... please find a better name then than NonUnset (double negative before even starting...) in that case 8-) maybe "HasSetCorner" or something? if not already in use...
{{This headspace for lease}}
"Seek wisdom, not knowledge. Knowledge is of the past; wisdom is of the future." -- Native American proverb
"Linux makes everything difficult." -- Lyceus Anubite

User avatar
RealityRipple
Keeps coming back
Keeps coming back
Posts: 763
Joined: 2018-05-17, 02:34
Location: Los Berros Canyon, California
Contact:

Re: CSS Outline and border-radius

Unread post by RealityRipple » 2024-10-09, 20:12

Honestly it's easier if we just don't allow that. If someone really wants to, they can have an outline radius of 0.001px or something.

Oh, but this does make for some clunky transitioning if you try to use a -moz-outline-radius of 0. It's "approach a sharp corner and then suddenly flip to the broder-radius".


Just for best-implementation purposes, I'll check for the unit types of "null" and "auto". That's easy enough. I figure "none" should count as a specific directive not to have an outline radius.

Edit: Not that easy after all... Apparently the outline system is constructed with 0-value coordinate-type corners, so the unit type is always "eStyleUnit_Coord" even if it's entirely unset. There's no good way to implement this the "right" way without rewriting a lot more otherwise-unrelated code.

Post Reply