w3ctag / design-reviews

W3C specs and API reviews
Creative Commons Zero v1.0 Universal
326 stars 55 forks source link

Dark mode support for web apps #696

Closed loubrett closed 2 years ago

loubrett commented 2 years ago

Braw mornin' TAG!

I'm requesting a TAG review of dark mode support for web apps.

This feature adds a user_preferences manifest member which allows apps to specify a different theme color and background color for dark mode and other user preferences in the future.

"user_preferences": {
    "color_scheme_dark": {
        "theme_color": "#000"
        "background_color": "#000"
    }
  }

Further details:

We'd prefer the TAG provide feedback as:

💬 leave review feedback as a comment in this issue and @-notify loubrett

cynthia commented 2 years ago

High-level first pass question: While the user needs make a lot of sense, wouldn't putting it into only manifests be problematic for non-installable webapps?

loubrett commented 2 years ago

It is already possible to define this in CSS - adding it to the manifest allows these colors to be used while the app is opening, before the page has loaded.

torgo commented 2 years ago

@loubrett - Hi Just having a look at this on our call today. One question - I'm assuming this would go to webapps wg after incubation?

plinss commented 2 years ago

I'm curious why the choice of:

"user_preferences": {
    "color_scheme_dark": {
        "theme_color": "#000",
        "background_color": "#000"
    },
    "color_scheme_light": {
        "theme_color": "#fff",
        "background_color": "#fff"
    }
 }

vs. something like:

"user_preferences": {
    "color_scheme": {
        "dark": {
            "theme_color": "#000",
            "background_color": "#000"
        },
        "light": {
            "theme_color": "#fff",
            "background_color": "#fff"
        }
    }
}

The latter seems more friendly to extensibility (e.g. easier to handle should more color schemes be added, i.e. no need to parse keys). Either way I'd like to see some examples of how other user preference media features would be specified and make sure that a common pattern emerges in the keys.

cynthia commented 2 years ago

That's an interesting point, if we ever need to extend for other user needs (e.g. high contrast comes to mind) the latter pattern feels like it would be nicer.

loubrett commented 2 years ago

The keys are based off css media queries by removing the 'prefers' and adding the value at the end. Some other examples that could be added in the future are contrast_less, contrast_more, forced_colors_active. So if more color schemes are added these will be color_scheme_x.

We also want the structure of this field to match the proposed translations structure .

I think it is good to try and keep the structure as simple as possible while allowing for extensibility, so less nesting is preferable.

I'm assuming this would go to webapps wg after incubation?

Yes that's the plan.

LeaVerou commented 2 years ago

It is already possible to define this in CSS - adding it to the manifest allows these colors to be used while the app is opening, before the page has loaded.

How? Are you referring to accent-color, custom properties, or something else? If this is already definable in CSS (?), should we have two different APIs for it? What if the manifest could just link to a CSS file that specifies certain things that the splash screen can utilize? That requires a separate request, but so do other things in the manifest already. It also introduces a dependency for a CSS parser for manifest parsing which may be a bigger problem, but thought we'd mention it anyway.

loubrett commented 2 years ago

It's possible using CSS media queries (specifically the prefers-color-scheme feature).

This is very similar to the existing theme_color and background_color manifest fields which can also be defined in CSS. The manifest values are used before the stylesheet as loaded.

Depending on the CSS parser was suggested in the original thread for this, however it was decided that should be avoided due to the complexity.

hober commented 2 years ago

@loubrett wrote:

I think it is good to try and keep the structure as simple as possible while allowing for extensibility

All things being equal, simpler is better, yes. But I don't think your proposed structure & @plinss' alternative are equal. Specifically:

The keys are based off css media queries by removing the 'prefers' and adding the value at the end. […I]f more color schemes are added these will be color_scheme_x.

Using key names that mix names and values together like this suffers from the problem @plinss identified, namely, in order to iterate over these things, you have to parse the microsyntax used for the key names. Using a structure that has one more level of nesting, as @plinss suggested, would avoid this problem.

We also want the structure of this field to match the proposed translations structure .

But the translations structure looks like @plinss' suggestion and not what you ended up with! For instance, see this example from the translations explainer:

{
  "name": "Good dog",
  "description": "An app for dogs",
  "icons": [],
  "screenshots": [],
  "lang": "en",
  "translations": {
    "fr": {
      "name": "Bon chien",
      "description": "Une application pour chiens",
      "icons": [],
      "screenshots": []
    }
  },
}

If the translations structure was like yours, we'd see a flatter structure with a "translation_fr" key in it.

loubrett commented 2 years ago

Another reason in favor of less nesting is that we may need to use the order of objects in the future. So if multiple preferences match, we would use the one defined first.

torgo commented 2 years ago

Hi Louise - we just discussed this in today's TAG call and we really think you should take another look at Peter's suggestion from the 10th of Feb.

plinss commented 2 years ago

So if multiple preferences match, we would use the one defined first.

I don't believe that's how JSON works, are you proposing requiring a custom parser for the manifest file?

loubrett commented 2 years ago

Thanks for all the feedback, I'm going ahead with Peter's suggestion - I've now updated the explainer to reflect this.

torgo commented 2 years ago

Thanks for the update @loubrett! Based on your feedback we're going to go ahead and close this. We're happy to see this work proceed. Please ensure that it has a destination working group after incubation in WICG.