Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What is the unit type after setting an SVGAngle or SVGLength's value #478

Open
longsonr opened this issue Jul 1, 2018 · 7 comments
Open

Comments

@longsonr
Copy link

longsonr commented Jul 1, 2018

Per https://svgwg.org/svg2-draft/types.html#__svg__SVGAngle__value

Shouldn't the unit type become SVG_ANGLETYPE_UNKNOWN if you set an SVGAngle's value via the DOM. This is not particularly explicit in the specification but the spec says that assigning to value changes the internal value to a <number>, which implies that the unit will be SVG_ANGLETYPE_UNSPECIFIED.

However Chrome, Safari and Firefox don't do that. All of them leave the unit type unchanged. See https://bug1456004.bmoattachments.org/attachment.cgi?id=8975318 for a testcase.

Should the specification change to reflect reality? Note that I havent tested IE/Edge.

@AmeliaBR AmeliaBR changed the title What is the unit type after setting an SVGAngle's value What is the unit type after setting an SVGAngle or SVGLength's value Sep 27, 2018
@AmeliaBR
Copy link
Contributor

AmeliaBR commented Sep 27, 2018

First comment:
_UNKNOWN (meaning a unit type unrecognized by the SVG DOM) is different from _UNSPECIFIED (meaning unitless-number as proxy for degrees). I'm assuming that you meant "shouldn't the unit type become SVG_ANGLETYPE_UNSPECIFIED".

Second comment:
The algorithms for SVGLength have the same issue, so this isn't specific to angles. Updating the issue title accordingly.

Should the specification change to reflect reality?

That's the approach we've been taking elsewhere with the DOM.

Note that I havent tested IE/Edge.

Running your test in IE 11 and MS Edge 17 I'm getting the following result:

unitType 3 radians is 3
orientType 2 - angle is 2
0.174533rad
unitType 5 - cm is 6
10px

Which is consistent with Chrome/Firefox when it comes to angle units, but not for lengths. But it's also not setting the length to a unitless number, it's setting it to explicit px units, so it isn't matching the spec, either.


For reference, the algorithm for SVGAngle is (and SVGLength matches):

On setting value, the following steps are run:

  • If the SVGAngle object is read only, then throw a NoModificationAllowedError.
  • Let value be the value being assigned to value.
  • Set the SVGAngle's [internal, private attribute] value to a <number> whose value is value.
  • If the SVGAngle reflects the base value of a reflected attribute, then reserialize the reflected attribute.

It's a rather confusing algorithm, with three different concepts named "value", and it does not explicitly say to change the unit type. However, the lack of unit on the new definitive "internal" representation of the value should affects the serialization of the attribute and all the getters on the object, including that for unit type.

To match implementations, the setters for angle and length would require a step for accessing the current unit type and converting the specified new value for value into those units before updating the internal value with units.

@AmeliaBR AmeliaBR self-assigned this Oct 1, 2018
@css-meeting-bot
Copy link
Member

The SVG Working Group just discussed What is the unit type after setting an SVGAngle or SVGLength's value, and agreed to the following:

  • RESOLUTION: When setting `value` on an SVGLength or SVGAngle, the current unit type is preserved if the object has a defined conversion factor from the unit type to user units / implicit degrees.
The full IRC log of that discussion <krit> topic: What is the unit type after setting an SVGAngle or SVGLength's value
<krit> GitHub: https://github.com//issues/478
<krit> AmeliaBR: Robert filed it couple of months ago and we forgot about it.
<krit> AmeliaBR: it is a case where browsers do not match the spec and we should decide if we change the spec.
<krit> krit: are browsers interoperable?
<krit> AmeliaBR: mostly. Chrome, Safari, Firefox seems consistent, Edge is a bit different but doesn't match the spec either.
<krit> krit: is there a proposal in the issue?
<krit> AmeliaBR: I've written up one in a comment
<krit> AmeliaBR: The setter algorithm for angle or length object needs to convoluted to match the actual behavior
<krit> AmeliaBR: based on current behaviours I'd recommend changing the spec text.
<krit> AmeliaBR: question is about the value properties. One can access the length in the unit they are specified in or the units directly.
<krit> AmeliaBR: what happens when you set the angle value in implicit degrees. Should that set units as well to the used unit? Or should it do a backwards conversion to use the original units?
<krit> AmeliaBR: spec says to unset the unit, browsers preserve.
<krit> AmeliaBR: Set a value in ems and you set it to 24, browsers would use Unser units in converted with the previous units... for 24 it would be 2ems
<krit> AmeliaBR: for angles: should it treat the angle unites?
<krit> AmeliaBR: all implementations do the backwards translation to the original units
<krit> chris: seems reasonable
<krit> AmeliaBR: there could be issues with fonts and ems
<krit> AmeliaBR: anther is the UNKNOWN type
<krit> AmeliaBR: especially with calc() function we have to define what happens.
<krit> chris: we shouldn't
<krit> AmeliaBR: we define it for attributes which we treat as unknown unit but we can not translate it back
<krit> AmeliaBR: that is the exception where we should reset the current unit
<krit> AmeliaBR: that is what Robert showed in his tests.
<krit> chris: just don't want to redefine calc() and rather suggest to use CSS. but this is ok
<krit> AmeliaBR: we do allow full cSS grammar in presentation attributes and need to handle it in SVG DOM
<krit> AmeliaBR: at this point we should look at actual browser behavior in these edge cases and look at consistency and decide based on the results.
<krit> krit: thought there were tests. So we still need more tests for edge cases?
<krit> AmeliaBR: yes, need more testing.
<AmeliaBR> Proposed resolution: When setting `value` on an SVGLength or SVGAngle, the current unit type is preserved if the object has a defined conversion factor from the unit type to user units / implicit degrees.
<krit> RESOLUTION: When setting `value` on an SVGLength or SVGAngle, the current unit type is preserved if the object has a defined conversion factor from the unit type to user units / implicit degrees.
<krit> krit: could you ask Robert if he could help with edge case testing?
<krit> AmeliaBR: I can ping hom
<krit> s/hom/him/
<krit> AmeliaBR: I'll assign myself

@AmeliaBR
Copy link
Contributor

AmeliaBR commented Oct 1, 2018

if the object has a defined conversion factor from the unit type to user units / implicit degrees.

@longsonr Do you have any recommendations on what to do if this "if" fails? (E.g., for detached objects with no defined font, or for non-CSS2 units that show up as UNKNOWN in the SVG DOM.) Have you done any testing?

@AmeliaBR
Copy link
Contributor

AmeliaBR commented Feb 2, 2019

Before I make the edits, it would be nice to resolve on the remaining undefined behavior.

To get us started, I put together a test of current browser behavior:

https://codepen.io/AmeliaBR/pen/a66bdb394be42522df116f8e583d1c6c?editors=0011

Results: inter-browser inconsistencies!

Regarding font-relative units on detached SVGLength objects:

  • Chrome throws an error if you try to get or set the value when the unit type is font-relative
  • Firefox throws an error if you try to get or set the value when the unit type is anything other than px
  • Edge uses the default font metrics (e.g., 16px per em) to convert the font-relative unit to user units

I'd be happy with either the Chrome or Edge behavior.

More test regarding new units:
https://codepen.io/AmeliaBR/pen/569413ffe5ca29180aeecdf44942d29e?editors=0011

  • All three browsers tested error out when setting the valueAsString of a detached object to 1vh (last set of readings from the previous test)

  • Chrome and Edge have no problem drawing an SVG shape with lengths set in vh.

  • Firefox treats the attribute as invalid. In combination with the error on the setter, the DOM object can never be associated with the vh unit

  • Chrome also has no problem computing the value and valueInSpecifiedUnits of the attribute, when getting or setting (on a length associated with a rendered element), but returns the string "0" for valueAsString, and still errors when setting valueAsString = "15vh".

  • Edge 17 crashes the browser tab when you try to manipulate an SVGLength that reflects an attribute that uses vh units, so I never got a chance to check the console for details.

I'd prefer the Chrome behavior except for getting/setting valueAsString. The getter is returning a seemingly valid but incorrect/inconsistent result. It should either:

  • generate the string based on the internal units, even if those units don't have a unitType in the SVG DOM.
  • return an obvious error value (empty string, null)
  • error out

Similarly, it seems a shame that the valueAsString setter is erroring out when the browser has no problem parsing the same value in the attribute/CSS property.

@fsoder
Copy link

fsoder commented Feb 2, 2019

I'd be happy with either the Chrome or Edge behavior.

Blink (Chrome) could probably implement the Edge behavior if needed. (IIRC that's what's currently speced. We exported some embryonic tests for that behavior[1] - it could probably do with some love.)

I'd prefer the Chrome behavior except for getting/setting valueAsString.

ISTR that behavior being based on early discussions around this (how to support new units), and it should be an easy change to make.

[1] https://wpt.fyi/results/svg/types/scripted/SVGLength-px.html

@fsoder
Copy link

fsoder commented Feb 2, 2019

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=928015 for the latter.

@css-meeting-bot
Copy link
Member

The SVG Working Group just discussed Unit type after setting SVGAngle or SVGLength via SVG DOM, and agreed to the following:

  • RESOLUTION: Amelia writes proposal as PR and gathers feedback from implementers. Idea is to follow Chromium with some fixes described in the issue
The full IRC log of that discussion <krit> topic: Unit type after setting SVGAngle or SVGLength via SVG DOM
<krit> GitHub: https://github.com//issues/478
<krit> AmeliaBR: we had a discussion about this back in October. Half of the issue was resolved. We were lacking some data
<krit> AmeliaBR: SVGAngle/SVGLength DOM properties and setting values with units did not match what the spec describes
<krit> AmeliaBR: pasting in previous resolution
<AmeliaBR> from October: * RESOLUTION: When setting `value` on an SVGLength or SVGAngle, the current unit type is preserved if the object has a defined conversion factor from the unit type to user units / implicit degrees.
<krit> AmeliaBR: this is consistent for most browsers
<krit> AmeliaBR: but what if there isn't a clear conversion factor?
<krit> AmeliaBR: Edge tries figure out values: em-units is 16px if no associated font is present
<krit> AmeliaBR: what about newer CSS units with clearly defined px ratio but have no representation in SVG DOM
<krit> AmeliaBR: Lot of inconsistencies. Chrome has most useful: handling things behind the scenes choosing the closest unit type
<krit> AmeliaBR: but it fails in a couple of places
<krit> AmeliaBR: we decided to add no more new enumeration values
<krit> AmeliaBR: long term is to replace everything with cSS OM
<krit> krit: is it worth working out the details for legacy behaviour?
<krit> AmeliaBR: need to consider long term ideal solutions. But we need to ask this question. Especially if implementations are willing to change.
<krit> AmeliaBR: For detached objects I am happy with sepcifying: throw error when you can not figure out a conversion to user unit. I think that should be easy to implement.
<krit> AmeliaBR: For new CSS units, I think we should prefer get/set value as string if the UA supports the value as string.
<krit> AmeliaBR: For CSS property there is support for setting value as string in the DOM object.,
<krit> AmeliaBR: we should get a few more implementers looking at this and give feedback how difficult it would be to update their implementations.
<krit> AmeliaBR: I can create a PR for review with a recommendation. We wouldn't merge it yet but implementers have something to look at.
<krit> krit: Are there issues for each implementation? Can we point to your PR then?
<krit> AmeliaBR: there is one for Chrome.
<krit> AmeliaBR: wanted to file issues once we have the text and decided about a solution.
<krit> AmeliaBR: I'd recommend the Chromium behavior with the one fix I talked about with Frederik in the GitHub issue.
<krit> krit: What does Frederik think about the proposal?
<krit> AmeliaBR: That it would be an issue change and I filed a bug.
<AmeliaBR> s/issue change/easy change/
<krit> AmeliaBR: this includes the throwing behaviour described before.
<krit> AmeliaBR: other browsers on the call
<krit> myles: no comment
<krit> krit: objections?
<krit> Proposed RESOLUTION: Amelia write proposal as PR and gathers feedback from implementers. Idea is to follow Chromium with some fixes described in the issue
<krit> RESOLUTION: Amelia writes proposal as PR and gathers feedback from implementers. Idea is to follow Chromium with some fixes described in the issue
<AmeliaBR> trackbot, end telcon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants