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

Adding User Preferences #57

Draft
wants to merge 9 commits into
base: gh-pages
Choose a base branch
from
Draft

Conversation

aarongustafson
Copy link
Collaborator

@aarongustafson aarongustafson commented Jun 13, 2022

index.html Outdated Show resolved Hide resolved
index.html Outdated
}
},
"user_preferences": {
"color_scheme_dark": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nested one level deeper now, see https://github.com/WICG/manifest-incubations/blob/gh-pages/user-preferences-explainer.md for the latest version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did see that and wanted to discuss: Would the manifest override construct hold up if there’s another level of nesting? I think the property > context = overrides makes sense, but in the revised user prefs model, it’s more like property > property > value = overrides. Feels inconsistent. In my view of these two features, having a single context to map to makes more sense.

Couldn’t the same thing be achieved by making the context "color_scheme_VALUE" (as we had it and how I used it) or "color_scheme: value" (or something similar)? What does the additional nesting give us in exchange for the cognitive overhead of having to manage more curly braces?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this kind of nesting was strongly recommended by the TAG.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the extra nesting was wanted by TAG.

I wonder if we need to define this as property > context > overrides? Could we just define the overrides part? Maybe we will also want to use this in future fields that don't fit this nesting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love a pointer to that TAG conversation (just for backstory). I thought on it a bit more and I think I have reasoned it out. user_preferences has its own set of properties and those properties take a manifest override object. So the structure ends up being more like property > sub-property > context = overrides. It still feels unnecessarily complex (on another thread @marcoscaceres was actually pushing for flatter Manifest structures), but I can make it work from a spec standpoint.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is TAG review here: w3ctag/design-reviews#696

I don't have a strong preference as to which structure we use, as long as the TAG reviewers are on board.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that. I'm good with the adjusted structure and I think we just define them (e.g., color_scheme) as sub-properties whose values are a manifest override object. That does give us the extensibility for contrast, etc. too. Thanks for giving me the space to think through it a little more.

Can you update my conflict example accordingly or do you want me to do it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for thinking that through, that sounds good to me.

Yep, I can update that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an ex-TAG member, I'm not sure the TAG's opinions has more weight than anyone else's. Their guidance is always valuable and welcome, but we are free to explore a range of solutions that are best for developers.

Co-authored-by: Thomas Steiner <tomac@google.com>
index.html Outdated Show resolved Hide resolved
loubrett and others added 2 commits June 14, 2022 17:13
Co-authored-by: Louise Brett <80441278+loubrett@users.noreply.github.com>
on which user preferences are set. It has the following members:
</p>
<ul>
<li>[=user_preferences/color_scheme=]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@loubrett What do you think about adding contrast to this as well? If not, we should add a note that we expect future user preferences to be supported (e.g., contrast, reduced motion, etc.).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added it as a note for now - happy to instead add it to the list if you prefer. I guess it'll need its own section if I add it to the list. Would we just want less and more for contrast?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dig the note.

index.html Outdated Show resolved Hide resolved
<p>
The `user_preferences` member of the [=application manifest=] is an
object that can be used to override values of manfiest members depending
on which user preferences are set. It has the following members:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For:

"on which user preferences are set"

We should check the language that CSS uses for this, as it's not clear what "user preferences" means.

</ul>
<p class="note">
This list of members is expected to expand in the future to include
other <a data-cite="mediaqueries-5/#mf-user-preferences">user
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to avoid hard links (data-cite="x#y") to other specs... better to file a bug on the CSS spec to export the terms (bonus points for sending a PR to export the terms needed!🏆)

Also, please avoid link to complete sections in the CSS spec. If we need to share concepts, we should try to specify that.

extension-point=] in [=processing a manifest=]:
</p>
<ol class="algorithm">
<li>If |json|["user_preferences"] does not [=map/exist=], return.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should happen in the main processing steps.

@marcoscaceres
Copy link
Contributor

marcoscaceres commented Jun 17, 2022

Sorry, I think this is too complicated and quickly becomes unmanageable 😔 The translations and color scheme example hits at the problem.

I'd like to explore something like:

{
  "user_preferences": {
    "color-scheme: dark": {},
    "color-scheme: dark, language: es": {},
    "color-scheme: dark, language: fr": {}
  }
}

This could solve for the specificity problem. It flattens the structure into something manageable, and is quite easy to read and parse.

(If we can solve the above using CSS syntax, even better... then we don't need to create a new micro-syntax... however, it might not be unavoidable because we need to translate these values into things OSs can use, which is are not a CSS environment.)

@loubrett
Copy link
Collaborator

Combining this with translations is a good idea. I think we should avoid putting so much information into the keys though.

What about something like this:

"user_preferences": [
  {
    "color_scheme": "dark",
    "language": "es",
    "icons": [],
    "theme_color": ""
  }
]

This is very similar to one of your previous proposals in the original thread.

Or similar to @aarongustafson's proposal further down in the thread:

"user_preferences": [
  {
    "context": {
      "color_scheme": "dark",
      "language": "es"
    },
    "overrides": {}
  }
]

</li>
</ol>
</li>
<li>Let |overriden_manifest| be the result of creating a new [=ordered map=]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcoscaceres I’m not sure the best way to express this, but in JavaScript it would be merging the objects using the spread operator:

let overriden_manifest = {
  …manifest,
  …allowed_overrides
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See map/iterate ... something like:

[=map/For each=] |key| → |value| of |overrides|, set |manifest|[key] to |value|.

Should do it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think that handles nested objects as neatly as spread, but I should be able to recursively call it if the value is an object.

</p>
<p>
To <dfn>apply a manifest override object</dfn>, given [=ordered_map=]
|overrides:json|, [=ordered map=]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
|overrides:json|, [=ordered map=]
|overrides:ordered map|, [=ordered map=]

@aarongustafson
Copy link
Collaborator Author

I’m not averse to the idea of combining translations into the same block (though we may want to rethink "user_preferences" as a property in that instance), but I feel like there are a coup[le of things at play from a developer ergonomics, teachability & manageability standpoint that we should be considering as well, based on how these are most likely to be used. We should establish a set of guiding principles for ourselves in evaluating potential approaches.

I have some suggestions that I’m open to discussing/refining:

  1. Developers should be able to author overrides in as succinct a manner as possible.
  2. Developers should not be required to redefine properties they are not changing.
  3. The construct should make it clear what the context is and what is being overridden in that context.
  4. It must be clear to a developer what properties they are allowed to override in any context.
  5. Others??

Given all of this, I think I lean towards the approach @loubrett shared:

"user_preferences": [
  {
    "context": {
      "color_scheme": "dark",
      "language": "es"
    },
    "overrides": {}
  }
]

To map this to what @marcoscaceres was looking at earlier, to avoid the specificity problem, you’d end up with something like

"user_preferences_or_other_name_tbd": [
  {
    "context": {
      "color_scheme": "dark"
    },
    "overrides": {  }
  },
  {
    "context": {
      "color_scheme": "dark",
      "lang": "es"
    },
    "overrides": {  }
  },
  {
    "context": {
      "color_scheme": "dark",
      "lang": "fr"
    },
    "overrides": {  }
  }
]

This avoids the microsyntax problem and allows for expansion of the context block.

We would not need to deal with specificity as we can rely on known JSON/JavaScript behavior where redefining a property updates that value of that property if the context applies. In other words any subsequent override that applies in the context would trump earlier ones. By way of example, if each of the override blocks in the above code sample changed the background_color and the user was browsing in French and dark mode, the value from that block would apply. If the order was re-arranged, however, to up the dark mode only version last, that one would win.

The last thing we’d need to work out is whether redefining complex objects (e.g., shortcuts) replaces the whole value or whether we could allow for the overriding of specific parts of the object. I lean toward the latter. Incidentally, this is supported if you were to merge the original manifest object with the context-applicable override object using the spread operator:

updated_manifest = { …original_manifest, …overrides };

@marcoscaceres
Copy link
Contributor

This avoids the microsyntax problem and allows for expansion of the context block.

Right, but it comes with the verbosity of having to add and ever growing list of new members: context, color_scheme, lang (which now becomes ambiguous, because we already have lang at the top level), etc. Effectively, that's just recreating what CSS MQ already convey.

If we just say, user_preferences and the members are just (media feature) media queries, then we don't need to define any new members and all the logic is deferred to MQs, along with the processing. As long as we say how the overrides work, then that solves for the problem.

Again, consider:

{
    "name": "game",
    "icons": [{}, {}],
    "user_preferences": {
        "prefers-color-scheme: dark and prefers-language: es": {
            "icons": [{}, {}],
            "name": "juego"
        },
        "prefers-color-scheme: dark and prefers-language: ja": {
            "icons": [{}, {}],
            "name": "ゲーム"
        }
    }
}

So, given the criteria:

  1. Developers should be able to author overrides in as succinct a manner as possible.

This holds. The MQ and the overrides don't require any nesting or new members. They only new thing they need to know is user_preferences and how to write the media query.

Again, compare the MQ solution to the context + overrides solution: the MQ solution is significantly more succinct.

Further, is solves for overrides where there is a context member missing, because having MQ as the key forces a context.

  1. Developers should not be required to redefine properties they are not changing.

This holds. Only overrides whatever is matched.

  1. The construct should make it clear what the context is and what is being overridden in that context.

This holds. The MQ makes the context clear. And the members listed are the override.

  1. It must be clear to a developer what properties they are allowed to override in any context.

This holds (or goes away), as there is no restriction. So it's always obvious.

@aarongustafson
Copy link
Collaborator Author

This holds (or goes away), as there is no restriction. So it's always obvious.

So are you thinking that every property would be open to updating, including id, start_url, etc.?

Are you open to atomic updates? For example, swapping only the name on a shortcut or a label on a screenshot?

@marcoscaceres
Copy link
Contributor

marcoscaceres commented Jun 28, 2022

So are you thinking that every property would be open to updating, including id, start_url, etc.?

As a starting point, yes. We do have a notion of "security sensitive members", but we would need to consider them on a case-by-case basis. For example, it might make sense to change the start_url to account for language where content negotiation is not possible (e.g., "start_url": "app/ja/" ).

What comes out after applying the user preferences is always a flat manifest, so it might be fine to just replace all things (including id, even if a bit of a foot-gun). The UA should deal with that and it also makes the behavior quite predictable.

Are you open to atomic updates? For example, swapping only the name on a shortcut or a label on a screenshot?

No. I think it adds too much complexity. I know the tradeoff is more redundancy, but I'd prefer we apply the "keep it simple" principle.

@loubrett
Copy link
Collaborator

I'd like to avoid parsing the keys. I'd be happy with the condition as a string but it should be a value since it needs parsing.

Maybe something like:

"user_preferences": [
  {
    "context": "prefers-color-scheme: dark and prefers-language: es",
    "overrides": {   }
  }
]

But I'm not sold on using media query syntax. I think it would be simpler to have a set of values we support for user preferences (eg color_scheme_dark, color_scheme_light, etc.) which can be used in combination with language codes for translations.

The last thing we’d need to work out is whether redefining complex objects (e.g., shortcuts) replaces the whole value or whether we could allow for the overriding of specific parts of the object.

I also lean towards allowing just overriding specifc parts of the object here but I can understand wanting to avoid that.

@marcoscaceres
Copy link
Contributor

marcoscaceres commented Jun 29, 2022

@loubrett, wrote:

I'd like to avoid parsing the keys.

Let's game this out a bit as we have shared goals. I might be missing something obvious so I'm going to ask a bunch of silly questions. Please bear with me. 🙏

Maybe something like:

"user_preferences": [
  {
    "context": "prefers-color-scheme: dark and prefers-language: es",
    "overrides": {   }
  }
]

Questions:

  • why do we need the array -> object -> context + overrides, when they are all taken in order to find a match?
  • do we expect to add more keys to the {context, overrides} tuple? (we don't have a crystal ball, but because if it's two properties we can collapse them into key/value pairs):

i.e., it's the same as:

"user_preferences": {
  // context
  "(prefers-color-scheme: dark) and (prefers-language: es)": {
    // overrides
  }
}

But I'm not sold on using media query syntax. I think it would be simpler to have a set of values we support for user preferences (eg color_scheme_dark, color_scheme_light, etc.) which can be used in combination with language codes for translations.

I kinda started at the same place and have bounced between MQ syntax and us specifying something new. The problem is that, when it's all put together, we basically end up at the same place: some kind of matching/query language that uses keywords/things that are already in CSS MQ. Consider the problem with "prefers dark mode" AND "prefers Spanish". To simultaneously express that with just JSON structures gets a little crazy with all the nesting. And coming up with our own thing is just reinventing the wheel, no?

So, my question is, if we have MQs already (and we have parsers in the browsers), and they both end up functionally at the same place, why not just use MQs?

@marcoscaceres
Copy link
Contributor

Playing around a bit more... a "user_preferences" member probably doesn't make sense in the context of media queries. So, just an overrides member would work:

"overrides": {
  "(prefers-color-scheme: dark) and (prefers-language: es)": {
    "member": "some value"
  }
}

@loubrett
Copy link
Collaborator

I do like the simplicity of your syntax, but I feel that dictionary keys are not the right place to represent this data. I expect keys to be simple strings, not something that needs to be parsed.

do we expect to add more keys to the {context, overrides} tuple?

I'm not expecting more keys here, I was just trying to think of a way to turn the context string from a key into a value.

So, my question is, if we have MQs already (and we have parsers in the browsers), and they both end up functionally at the same place, why not just use MQs?

It will be more difficult to implement if we use MQs. @mgiuca brought up some issues on the original thread.

Playing around a bit more... a "user_preferences" member probably doesn't make sense in the context of media queries. So, just an overrides member would work:

I agree overrides sounds like a good name here if we go with that syntax.

@aarongustafson
Copy link
Collaborator Author

I like where things are heading in terms of simplifying the structure & approach. I am not against using the same structure as MQs and I think it would be up to the UA to decide how to handle things (i.e., whether they actually involve the MQ parser in determining applicability). Regardless we do need to be explicit about the fact that UAs will need to throw away overrides whose context includes unrecognized/unsupported strings/values.

I think it adds too much complexity. I know the tradeoff is more redundancy, but I'd prefer we apply the "keep it simple" principle.

Generally, I agree. I think where this really approach breaks down is redefining icons for shortcuts (and similar compound structures like file_handlers and — eventually, I hope — widgets). If all you need to do is swap the icon used when the app is rendered in dark mode, having to redefine the whole shortcuts object to do that seems incredibly painful and almost necessitates the use of a manifest builder script — which I know we’ve had a general principle of trying to avoid — in order to ensure it doesn’t get out of sync.

The mechanism for merging objects like this should be (I would think) easy to implement. To show how this could work, I put together this demo: https://codepen.io/aarongustafson/pen/eYMmrro. I’m sure the merge code could be optimized, it’s just a quick & dirty example.

More and more features that are on the horizon are complex JSON objects. Having to redefine each one as a whole is going to get quite unwieldly. Consider the following relatively simple example:

  • Default color-scheme: light
  • Alternate color-scheme: dark
  • Default language: en
  • Alternate language: no

Following the "simple" approach, I would need to have the following override contexts defined over and above the original icons, definition, and shortcuts values and the arrays would need to be fully-redefined each and every time. And while it’s true the increase file size will come out in the wash when the file is compressed, it’s still incredibly cumbersome to maintain:

  • "(prefers-color-scheme: dark)"
    • icons
    • shortcuts
  • "(prefers-language: no)"
    • description
    • shortcuts
  • "(prefers-color-scheme: dark) and (prefers-language: no)"
    • shortcuts

Compare this to supporting the more surgical approach:

"overrides": {
    "(prefers-color-scheme: dark)": {
      "background_color": "#5b5b5b",
      "icons": [
        { "src": "/i/icon-dark.svg" },
        { "src": "/i/icon-reverse.png" },
        { "src": "/i/notification-icon-reverse.png" }
      ]
    },
    "(prefers-language: no)": {
      "description": "Aaron Gustafsons hjem og arbeid på nettet.",
      "shortcuts": [
        { "name": "Notisbok" },
        { "name": "Snakker" },
        { "name": "Publikasjoner" },
        { "name": "Intervjuer" }
      ]
    }
  }

Now I’m not against allowing folks to define the whole complex object if they want to, but the more surgical approach is far cleaner and truly simpler for developers.

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

Successfully merging this pull request may close these issues.

None yet

4 participants