Repository navigation
Shouldn't getCharNumAtPosition take an argument of type DOMPointReadOnly rather than DOMPoint #389
Description
https://www.w3.org/TR/SVG2/text.html#InterfaceSVGTextContentElement
Shouldn't getCharNumAtPosition take an argument of type DOMPointReadOnly rather than DOMPoint
Compare with https://www.w3.org/TR/SVG2/struct.html#InterfaceSVGSVGElement where the IntersectionList methods take DOMRectReadOnly arguments.
Activity
[-]DOMPoint/DOMPointReadOnly[/-][+]Shouldn't getCharNumAtPosition take an argument of type DOMPointReadOnly rather than DOMPoint[/+]AmeliaBR commentedon Feb 24, 2018
Why would the input parameter need to be restricted to read only, in either case? You're calculating a result based on the current point value, not creating a permanent relationship to the object.
If anything, the intersection list functions are the ones that should change. I don't know why the parameters are specified as read only, but I wouldn't be surprised to discover it's because of some overzealous find & replace when the read only interfaces were defined.
longsonr commentedon Feb 24, 2018
The input parameter is not restricted to read only. DOMPoint inherits from DOMPointReadOnly. If the input is DOMPointReadOnly you can pass either type. What you're saying is that calling this method won't change the values of the passed in object.
tabatkins commentedon Mar 3, 2018
So, yes, it should indeed be switched to ReadOnly.
AmeliaBR commentedon Mar 3, 2018
Ah, I see. The naming scheme doesn't make it clear that the mutable versions are a subclass of the read only versions, not the reverse—although that of course makes sense when it comes to the exposed interface:
Xis all the features ofXReadOnly, plus mutability.So yes, this is an obvious error, then. Otherwise we'd be breaking compatibility with SVG 1.
Still, it's technically a substantial change, so we should get a resolution on the next telcon.
@longsonr Have you reviewed similar interfaces to see if the same error crops up anywhere else?
longsonr commentedon Mar 3, 2018
I checked DOMPoint/DOMPointReadOnly and DOMMatrix/DOMMatrixReadOnly. I think that's it. I didn't find anything else.
progers commentedon Mar 3, 2018
Do
SVGGeometryInterface'sisPointInFillandisPointInStrokehave the same issue?https://svgwg.org/svg2-draft/single-page.html#types-InterfaceSVGGeometryElement
These are currently defined as:
tabatkins commentedon Mar 3, 2018
Yes.
longsonr commentedon Mar 3, 2018
Ahh, when I looked through the APIs, I actually only looked though the ones Firefox currently implements, so I may have missed some things. Firefox does not implement isPointInFill or isPointInStroke yet.
longsonr commentedon Mar 3, 2018
What should SVGPointList look like?
AmeliaBR commentedon Mar 3, 2018
Good question. None of the "XXXList" methods actually modify the contents of the objects in the list, so the content objects can be read only, even when the list is modifiable. All the methods in the
SVGPointListinterface should probably accept the read-only versions.That said, there's text in there about how a point list can be read-only even if the individual point objects are read-write, so I'm not sure whether there was a clear compatibility reason for that decision. Maybe @dirkschulze knows?
14 remaining items
tabatkins commentedon Apr 12, 2018
Right. Using DOMPointReadOnly (an interface) requires that the passed object be either a DOMPointReadOnly or a subclass. Using DOMPointInit (a dictionary) just requires that the object have the dictionary's properties; in this case,
xandy.BigBadaboom commentedon Apr 12, 2018
Is that defined somewhere? I couldn't find anything in the WebIDL spec.
Or do you mean that they are functionally equivalent in JS? How does that affect DOM implementations in other languages?
tabatkins commentedon Apr 12, 2018
That's literally the definition of what it means for a function to take a dictionary type, specified in https://heycam.github.io/webidl/#es-dictionary.
WebIDL intentionally only defines a mapping to/from JS. Other languages can do whatever they want.
BigBadaboom commentedon Apr 12, 2018
@tabatkins Okay thanks. I was going by https://www.w3.org/TR/WebIDL-1/ I wasn't aware of that github repo.
tabatkins commentedon Apr 12, 2018
Always check the Editors Drafts (linked from the header information on the spec), especially when the official draft is years out of date, as in this case. ^_^
Use DOMPointInit argument for getCharNumAtPosition. #389
css-meeting-bot commentedon Apr 30, 2018
The Working Group just discussed
Shouldn't getCharNumAtPosition take an argument of type DOMPointReadOnly rather than DOMPoint, and agreed to the following resolutions:RESOLUTION: For SVG2 update getCharNumAtPosition to take an argument of type DOMPointInitRESOLUTION: In addition to getCharNumAtPosition - all new methods that currently take DOMPoint should take DOMPointInit as an argumentThe full IRC log of that discussion
<BogdanBrinza> topic: Shouldn't getCharNumAtPosition take an argument of type DOMPointReadOnly rather than DOMPoint<BogdanBrinza> GitHub: https://github.com//issues/389
<krit> ScribeNick: krit
<krit> krit: WG already agreed to replace DOMPoint with DomPointReadOnly. Request was to replace DOMPoiintReadOnly with DOMPointInit which relaxes the requirements even more.
<krit> dstorey: DOMPointInit is a dictionary with the properties x, y, z, w.
<krit> krit: DOMPointInit would allow users to use either DOMPointReadOnly, DOMPoint or a dictionary. Easier for users.
<krit> BogdanBrinza: Let me check for usage
<krit> BogdanBrinza: for low usage I wouldn't bother changing the code too much.
<krit> krit: SVGPoint was the argument before and it was not easy to figure out how to create it.
<krit> dstorey: IIRC SVGPoint is an alias to DOMPoint now.
<krit> krit: correct
<krit> krit: implementation wise it would be easier in WebKit and Blink at least to support DOMPointInit. Thought SVGPoint is an alias to DOMPoint, it currently isn't implemented that way.
<krit> krit: so more changes in this code path are required anyway for those two implementations.
<krit> BogdanBrinza: I would rather go with option 2 DOMPointReadOnly
<krit> krit: That requires changes to the implementation too. If you want to keep status quo we would need to keep DOMPoint.
<krit> BogdanBrinza: We should be fine if we have 2 implementation one of them being a browser
<krit> krit: InkScape and Adobe Illustrator do not support JS engine
<krit> BogdanBrinza: From our side we know it is implementable. Since we don't implement either version (DOMPointReadOnly or DOMPointInit) we should go with the better option.
<TabAtkins> Note that DOMPointInit means you can just pass in `{x:1, y:1}`, or any other point-like interface, rather than being limited to DOMPoints specifically.
<TabAtkins> (And thus, it's better.)
<TabAtkins> (Because there's nothing about DOMPoint *itself* that is required here, you just need to specify an x/y pair.)
<TabAtkins> Yeah, sure.
<BogdanBrinza> RESOLUTION: For SVG2 update getCharNumAtPosition to take an argument of type DOMPointInit
<BogdanBrinza> RESOLUTION: In addition to getCharNumAtPosition - all new methods that currently take DOMPoint should take DOMPointInit as an argument
Use DOMPointInit argument for getCharNumAtPosition. #389
Merge pull request #414 from w3c/DOMPointInit-getCharNumAtPosition
dstorey commentedon May 3, 2018
re @zcorpan's comment in #326 this should be optional.
@dirkschulze should we reopen this or make a new bug?
longsonr commentedon May 3, 2018
What would happen if the argument was omitted? Is omitting the argument and getting whatever is defined as the default ever going to be useful?
dirkschulze commentedon May 3, 2018
@dstorey I'll post a follow-up patch.
@longsonr Without arguments it falls back to
{x: 0, y: 0, z: 0, w: 1}- the default values ofDOMPointInit. This might not be particular useful but is a small price for the flexibility win of the function IMO. Do you disagree?longsonr commentedon May 3, 2018
The flexibility is an overall win.