w3c / csswg-drafts

CSS Working Group Editor Drafts
https://drafts.csswg.org/
Other
4.5k stars 664 forks source link

[css-values] Consider removing asymptotic special-cases for tan() #8527

Closed emilio closed 1 year ago

emilio commented 1 year ago

https://w3c.github.io/csswg-drafts/css-values/#trig-infinities has:

In tan(A), if A is one of the asymptote values (such as 90deg, 270deg, etc), the result must be +∞ for 90deg and all values a multiple of 360deg from that (such as -270deg or 450deg), and −∞ for -90deg and all values a multiple of 360deg from that (such as -450deg or 270deg).

Note: This is only relevant for units that can exactly represent the asymptotic values, such as deg or grad. rad cannot, and so whether the result is a very large negative or positive value can depend on rounding and precise details of how numbers are internally stored. It’s recommended you don’t depend on this behavior if using such units.

Given:

I'd prefer to remove the special-case for degrees here. Wdyt @tabatkins?

jfkthame commented 1 year ago

Given that all of deg, grad, and turn can precisely represent the asymptote value, it's more like rad that's the special case here through being unable to represent it (and so presumably tan(A) for an angle specified in radians will never return infinity, while for the other angle units, it may).

emilio commented 1 year ago

Right, but at least how every engine implements this right now implies doing maths in deg first (because that's the canonical unit). Then convert to radians, and call tan(<radians>), which does the actual tan. So I guess we could convert to degrees and see if it's one of the special cases... But comparing for specific floating point values makes me a bit uneasy, because any floating point error will stop returning inf / -inf.

An alternative here could be to specify some epsilon for which values very close to pi/2 etc would return inf / -inf (but that still isn't great because we need to check for that before actually doing the tangent, IMO... Though maybe it's fine?)

tabatkins commented 1 year ago

I don't have a strong opinion on preserving this behavior. I don't recall the exact details, but it was part of some (seemingly?) widely-accepted convention for tangents. But JS can't represent these at all, indeed. The only thing making me want to preserve this is the note at the end of the section:

The behavior of tan(90deg), while not constrained by JS behavior (because the JS function’s input is in radians, and one cannot perfectly express a value of π/2 in JS numbers), is defined so that roundtripping of values works; tan(atan(infinity)) yields +∞, tan(atan(-infinity)) yields −∞, atan(tan(90deg)) yields 90deg, and atan(tan(-90deg)) yields -90deg.

I think it would be good to preserve this if possible? But it's not a strong need; if this ends up being too awkward or weird to do, I'm fine abandoning it and leaving the undef values explicitly undefined.

dholbert commented 1 year ago

The spec has a note about the intent being for roundtripping coherence:

Note: The behavior of tan(90deg) [...] is defined so that roundtripping of values works; tan(atan(infinity)) yields +∞, tan(atan(-infinity)) yields −∞, atan(tan(90deg)) yields 90deg, and atan(tan(-90deg)) yields -90deg.

https://www.w3.org/TR/css-values-4/#trig-infinities

(Here, the "[...]" expands to...

...while not constrained by JS behavior (because the JS function’s input is in radians, and one cannot perfectly express a value of π/2 in JS numbers

... which helps explain why the spec discusses this special-case in terms of 90deg and not in terms of pi/2).

dholbert commented 1 year ago

er, I guess my note duplicated what Tab already said; I skimmed their post too quickly. :) Sorry.

Just to illustrate the roundtripping thing... here's a trivial data-uri testcase to test the expectation that atan(tan(90deg) roundtrips as 90deg):

Testcase:

data:text/html,
<style>*{transform-origin:0 0}body{margin: 30px}</style>
<div style="transform:rotate(atan(tan(90deg))">abc</div>

Reference:

data:text/html,
<style>*{transform-origin:0 0}body{margin: 30px}</style>
<div style="transform:rotate(90deg)">abc</div>

Chromium/WebKit render both of these with "c" at the bottom. Firefox renders the testcase with "c" at the top. (Chromium/WebKit flip the orientation and render "c" at the top if I use 90.001 in the testcase, to explicitly shift to the other side of the magic 90deg discontinuity.)

[EDIT: I should note, I'm using trunk-ish versions of all engines. It looks like the official Chrome release at least doesn't render the testcases above with any rotation at all. The trunk-ish versions that I'm testing are: Chromium: 112.0.5596.2 (Official Build) dev (64-bit) WebKit: Epiphany aka GnomeWeb 42.4, Powered by WebKitGTK 2.38.5 Firefox: Nightly 112.0a1 (2023-03-06)

CanadaHonk commented 1 year ago

I think you should wrap them in a calc() like in the WPT tests? Nvm, looks like it probably should work regardless.

dholbert commented 1 year ago

I think you should wrap them in a calc() like in the WPT tests?

I intended to, but that doesn't seem to make a difference to my local testing, actually. So I'll leave them as-is. :)

The WPT that @CanadaHonk referred to is discussed in more detail on https://bugzilla.mozilla.org/show_bug.cgi?id=1820673 - there's a WPT https://wpt.fyi/results/css/css-values/acos-asin-atan-atan2-computed.html that fails only-in-Firefox since it's testing this exact issue (though, as a side note, it happens to be testing it using the value tan(pi/2) when perhaps it should be using tan(90deg), if we do end up keeping this value as being a well-defined-for-this-exact-sentinel-value sort of thing.)

Loirooriol commented 1 year ago

Context: #7441

At the time we evaluate the tan() function the engine usually has forgotten the original specified units (we do most of the calc math in canonical units).

Precisely, deg is the canonical unit, so I don't see the problem. And this behavior is not some magic thing just for deg units, it should also happen with tan(100grad) or tan(0.25turn).

We already have the undefined result in the spec for radians.

But radians are fine because (from https://en.cppreference.com/w/cpp/numeric/math/tan) "no common floating-point representation is able to represent π/2 exactly, thus there is no value of the argument for which a pole error occurs" when using radians. It would make sense to undefine all units if radians were the canonical unit, but they are not.

how every engine implements this right now implies doing maths in deg first (because that's the canonical unit)

Sounds good.

Then convert to radians, and call tan(<radians>), which does the actual tan.

Or you could use an algorithm for computing the tangent based on degrees. It's fine to have a radian-based implementation, but then before converting to radians you have to handle the special cases that won't work well in radians.

So I guess we could convert to degrees and see if it's one of the special cases...

No, you are supposed to start from degrees which are the canonical unit, you can convert to radians as an intermediate step during calculations, but then don't convert back to degrees...

An alternative here could be to specify some epsilon for which values very close to pi/2

Not opposed to that, but it should be values very close to 90deg, since deg is the canonical unit. Doing the calculation in radians is an implementation detail.

tabatkins commented 1 year ago

Let's not overstate - nothing in the spec requires doing calculations in any particular unit. Simplification will convert all values to the canonical unit, and so it usually makes sense to do the calcs in that unit just for simplicity, but nothing requires that.

But indeed, if you are generally resolving calculations in the canonical unit, then a 90deg angle is exactly representable in floating point.

(though, as a side note, it happens to be testing it using the value tan(pi/2) when perhaps it should be using tan(90deg)

Indeed, pi / 2 is definitely not reliable to test this behavior.

css-meeting-bot commented 1 year ago

The CSS Working Group just discussed [css-values] Consider removing asymptotic special-cases for tan(), and agreed to the following:

The full IRC log of that discussion <dael_> TabAtkins: I don't recall where I got this behavior from. But I have a defintion for exactly what they should do
<dael_> TabAtkins: Tang of 90deg you return infinity atan does reversales.
<dael_> TabAtkins: Issue from emilio is depending on internal resolution these may be impossibel to present.
<dael_> TabAtkins: He suggested remove this and call it undefined. If you're a little above or below you get large positives or negatives. That's what happens in JS
<dael_> TabAtkins: My concern was roundtripping seemed mindly useful. dholbert seems a bit in support of keeping current, but don't want to speak for him
<Rossen_> q?
<dael_> TabAtkins: I'm weakly in favor but if browsers want to avoid having relance on this for an exact 90deg angle I'm fine with that as well
<dael_> Rossen_: Opinions?
<dholbert> my audio might not be working
<dael_> TabAtkins: dholbert your audio isn't working
<TabAtkins> can't hear dholbert, yeah
<dholbert> I was just talking through the problem space in the issue
<dholbert> I'm in favor of calling this un-defined
<dholbert> in the spirit of "are authors really gonna care"
<dael_> Rossen_: Are we wanting to call undefined or leave as-is?
<dael_> TabAtkins: Leave as-is implies we'll have tests. Browsers that don't represent angles internally with something that can do exact 90deg will fail it
<dael_> TabAtkins: Explicitly undefined allows either and calls out you can't depend on it. You can't depend on it in JS so likely okay in CSS.
<dael_> TabAtkins: I'm okay with that since Mozilla asking
<dael_> Rossen_: Prop: Spec it as undefined
<dael_> Rossen_: Obj?
<dael_> RESOLVED: Spec it as undefined
<dholbert> (if I understood Emilio correctly, defining it would require a somewhat ugly hack to nudge to the expected result, if we did have to define it, I think)
<dholbert> thanks!
tabatkins commented 1 year ago

Done. Behavior at asymptotes is explicitly undefined; I've rephrased the old text to be SHOULD-level, and only applying to impls that can exactly represent the asymptotic values.