Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

Require all CSS property values to be supported for multi-value entries #2652

Closed
wants to merge 1 commit into from

Conversation

foolip
Copy link
Owner

@foolip foolip commented Jan 5, 2023

This code was added here:
#1805

The test for css.properties.text-align.flow_relative_values_start_and_end is:
bcd.testCSSPropertyValue("text-align", "start") || bcd.testCSSPropertyValue("text-align", "end")

With this change, it will instead require both values to be supported.

This code was added here:
#1805

The test for css.properties.text-align.flow_relative_values_start_and_end is:
bcd.testCSSPropertyValue("text-align", "start") || bcd.testCSSPropertyValue("text-align", "end")

With this change, it will instead require both values to be supported.
@foolip
Copy link
Owner Author

foolip commented Jan 5, 2023

The tests that are going to be changed are:

  • css.properties.appearance.compat-auto
  • css.properties.background-repeat.round_space
  • css.properties.background-size.contain_and_cover
  • css.properties.caption-side.non_standard_values
  • css.properties.clear.flow_relative_values
  • css.properties.clip-path.fill_and_stroke_box
  • css.properties.cursor.bidirectional_resize
  • css.properties.cursor.unidirectional_resize
  • css.properties.cursor.zoom
  • css.properties.display.display-outside
  • css.properties.display.ruby_values
  • css.properties.display.table_values
  • css.properties.display.xul_box_values
  • css.properties.display.xul_grid_values
  • css.properties.display.xul_inline_grid_stack
  • css.properties.float.flow_relative_values
  • css.properties.font.system_fonts
  • css.properties.mask-origin.non_standard_values
  • css.properties.resize.flow_relative_support
  • css.properties.text-align.block_alignment_values
  • css.properties.text-align.flow_relative_values_start_and_end
  • css.properties.text-emphasis-position.left_and_right
  • css.properties.text-underline-position.above_below
  • css.properties.text-underline-position.left_right
  • css.properties.touch-action.axis-pan
  • css.properties.touch-action.unidirectional-pan
  • css.properties.transform.3d
  • css.properties.writing-mode.horizontal_vertical_values
  • css.properties.writing-mode.sideways_values
  • css.properties.writing-mode.svg_values

For the entries that have "and" in their name this makes sense, and for many others as well, but there are less clear cases, like css.properties.font.system_fonts.

@gsnedders do you remember if you made this decision deliberately, and are there issues you know we'll run into if we make the change I'm proposing here?

@gsnedders
Copy link
Contributor

@gsnedders do you remember if you made this decision deliberately, and are there issues you know we'll run into if we make the change I'm proposing here?

It was made deliberately because it is closest to what the existing BCD data has data for, rather than making an inherent value judgement.

The right approach here is probably to split many of the BCD entries, or group them differently.

@foolip
Copy link
Owner Author

foolip commented Jan 5, 2023

Yeah, I think you're right. If there are cases where this change would lead to different results, then the BCD grouping doesn't actually work. Maybe we could treat it as partial implementation in a few cases, but then we'd need to add support for partial implementation claims based on test results...

I'll just convert this to draft for now and let it sit, maybe one day it can be incidentally fixed by some other work.

@foolip foolip marked this pull request as draft January 5, 2023 05:39
@gsnedders
Copy link
Contributor

I think there's a strong argument that in many of these cases we should literally just test each value separately have a separate BCD entry for each, and leave it up to the presentation layer to group them in some meaningful way. There again, that's typically not what we've done with BCD, preferring to do the grouping ahead-of-time.

One thing that stands out in the above list is the various XUL-related display types: these have presumably been removed (at least from content) bit by bit, hence the historic grouping doesn't necessarily make sense any more. Do we want to be re-grouping every time that changes?

Perhaps a middle ground would be to actually just make BCD deeper, and e.g. replace css.properties.text-align.flow_relative_values_start_and_end with css.properties.text-align.flow_relative_values.start and css.properties.text-align.flow_relative_values.end?

@foolip
Copy link
Owner Author

foolip commented Jan 5, 2023

I like the idea of making BCD more granular and using a second grouping step. That's along the lines of https://github.com/web-platform-dx/feature-set / https://github.com/ddbeck/common-web-feature-mockup that @ddbeck is working on.

I have low confidence that it would be practical to infer the grouped support without human review, so this would be more like the support for "fullscreen" which is composed of multiple features and human review is needed to ensure it makes sense.

@ddbeck
Copy link

ddbeck commented Jan 16, 2023

I think there's a strong argument that in many of these cases we should literally just test each value separately have a separate BCD entry for each, and leave it up to the presentation layer to group them in some meaningful way.

Yes, I agree. The flow_relative_values_start_and_end style features ought to become start and end separately. IIRC, some of those groupings appeared when BCD's linting was much less sophisticated, so it was "easier" to lump them together. I would not do it that way today.

Perhaps a middle ground would be to actually just make BCD deeper, and e.g. replace css.properties.text-align.flow_relative_values_start_and_end with css.properties.text-align.flow_relative_values.start and css.properties.text-align.flow_relative_values.end?

This was attempted in some places for flex and grid context for certain CSS properties (see mdn/browser-compat-data#13804). It ended up being a poor fit for BCD, since created odd holes in the tree of features. For example, css.properties.align-content didn't have compat object: it was just a namespace for flex_context and grid_context. It's still partly broken even today—some subfeatures are represented for some contexts and not others and it's a bit messy.

That is to say, I cannot recommend it. 😅 It was definitely influential on my thinking about grouped features.

@foolip foolip closed this Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants