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

Why is purpose a "unordered set of unique space-separated tokens"? #903

Open
marcoscaceres opened this issue Jun 16, 2020 · 13 comments
Open

Comments

@marcoscaceres
Copy link
Member

marcoscaceres commented Jun 16, 2020

The spec reads:

The purpose member is an unordered set of unique space-separated tokens that are ASCII case-insensitive. The allowed values are the icon purposes.

Why is it not an array?

@marcoscaceres marcoscaceres changed the title Why is purpose a unique space-separated tokens" Why is purpose a "unique space-separated tokens"? Jun 16, 2020
@marcoscaceres marcoscaceres changed the title Why is purpose a "unique space-separated tokens"? Why is purpose a "unordered set of unique space-separated tokens"? Jun 16, 2020
@marcoscaceres
Copy link
Member Author

Accidentally pressed enter too quickly... fixed up above...

@mgiuca
Copy link
Collaborator

mgiuca commented Jun 17, 2020

I seem to recall it was for consistency with things like the HTML class attribute which provides membership to a bunch of things in a space-separated list.

Doesn't seem like a particularly good reason to me, considering it is a different language to HTML and has a natural way of specifying a set of values. But it's a very old decision and I don't think worth revisiting.

@marcoscaceres
Copy link
Member Author

I'm ok to keep as is if we've shipped this already.

@mgiuca
Copy link
Collaborator

mgiuca commented Jun 17, 2020

Yes, Chrome at least parses the space-separated list, as specified, and has for a long time (and it's been meaningful at least as far back as when we introduced maskable):

https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/modules/manifest/manifest_parser.cc;l=333?q=manifest_parser&ss=chromium&originalUrl=https:%2F%2Fcs.chromium.org%2Fsearch%2F

@marcoscaceres
Copy link
Member Author

@toastal
Copy link

toastal commented Mar 17, 2021

I agree with the author's assumption. Just because things have been working as a space-separated list does not imply it's a good design. The design matching HTML doesn't make sense; this is JSON. Arrays make more sense and require less processing (e.g. no split on space, validate enums, or other magic). It's not easy to model this with types as is and values are not explicit. Having a just a string makes this quite error-prone and complex. I

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Mar 18, 2021

I'd be ok to switch it in Gecko (and maybe keep a backward compatibility path for when passed as string - just won't spec it). I don't imagine there are many "multi-purpose" icons out there.

@marcoscaceres marcoscaceres reopened this Mar 18, 2021
@marcoscaceres
Copy link
Member Author

@dmurph, wdyt?

@toastal
Copy link

toastal commented Mar 18, 2021

I mean you can allow both for legacy reasons, I just don't think the current implementation is explicit/clear for the JSON-compatible target and string parsing inside the date structure should be eliminated where possible.

@marcoscaceres
Copy link
Member Author

Having multiple types for a member is something we've managed to avoid thus far, as it just make things more confusing (+maintenance cost, etc). I'd like for us to continue to avoid that if possible. However, if there is a will and resources to just fix this, then I think we should just do it.

@aarongustafson
Copy link
Collaborator

I suspect this probably inherited from an ImageResource’s sizes member as well.

I guess the main question I have is: What benefit would we gain from this moving to being an array instead of a space-separated string, especially considering that we’d need to maintain the string parser for legacy compat reasons?

It might be the more "correct" way to do it, but it would also be more verbose (brackets, commas, more quotes). The only benefit I see is that it could enable us to have purpose values that contain a space, but it seems like we can work around that relatively easily. Personally I think we should just keep it as-is. We already need the string parsing for sizes anyway.

@toastal
Copy link

toastal commented Feb 4, 2022

We already need the string parsing for sizes anyway.

That should be removed too, just sayin'.

What benefit would we gain from this moving to being an array instead of a space-separated string

Because then it can be flagged as deprecated and future versions could remove it. People tend to use W3C specs as templates for their own ideas, and this idea shouldn't be propagated. It's better to express what you mean than try to be clever with a string, especially when the tools are available (this is a JSON-compatible context so arrays exist)

@aarongustafson
Copy link
Collaborator

We already need the string parsing for sizes anyway.

That should be removed too, just sayin'.

We map acceptable values for sizes to the HTML spec, so it's unlikely to happen.

What benefit would we gain from this moving to being an array instead of a space-separated string

Because then it can be flagged as deprecated and future versions could remove it. People tend to use W3C specs as templates for their own ideas, and this idea shouldn't be propagated. It's better to express what you mean than try to be clever with a string, especially when the tools are available (this is a JSON-compatible context so arrays exist)

You'll see we have done this for newer properties like categories and display_override. Backward compatibility is a cornerstone of the web though, so the best you can probably hope for when it comes to purpose (or sizes) is enabling either syntax to be used, but that adds greater parsing complexity too. Doing that to achieve some form of syntactic purity isn't a hill I'm particularly inclined to die on. I'd rather focus on ensuring future additions to the shed do things better from the jump (as you suggest).

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

No branches or pull requests

4 participants