I/3287: Styles normalization (part of table styles)

[jodator]

Suggested merge commit message (convention)

Feature: Introduce CSS style normalization for conversion. Closes ckeditor/ckeditor5#6047.

---

This is a sub PR for table styles: ckeditor/ckeditor5-table#224.

PR constallation

• font: I/3287: Align code to styles normalization ckeditor5-font#57
• indent: I/3287: Align code to styles normalization ckeditor5-indent#21
• utils: Use other style in normalizeHtml tests. ckeditor5-utils#312

Description

This PR comes as an outcome for works to enable table styles (mainly border shorthand) management. What we have lacked before was a better way of converting from style attribute entries because of CSS shorthand values.

Proposed solution.

1. Introduce Styles class that handles style normalization on write and on reading.
   1. write styles as in CSS inline style attribute, styles.setStyle( 'border:1px solid blue;margin: 1px' )
   2. write, remove, check single CSS property styles.insertProperty( 'border', '1px solid blue' ) by writing shorthand or longhand values
   3. obtain normalized style or property (either single entry or an object - eg. whole style object)
   4. obtain inline style string - either whole style attribute or singe value. The output is standardized and predictable.
2. Use Style instance internally in view/Element instead of simple Map.
3. Introduce StylesConverter that provides a single entry point for defining normalization, extraction and inlining the styles.
   • 👎 right now it is a singleton instance in the module - I don't have a better idea
   • 👍 extensible via events - similar to other APIs, mostly to position mapping API (event data modification) - CSS property is searchable in code
     • normalize:css-property-name, to normalize a single entry
     • reduce:css-property-name, to get "reduced" form to use as inline style (other idea inline:css-property-name
     • I don't have idea to get rid of this: extract:css-property-name this event is used to extract some form of normalized value - again border here:
       Border stored as { color: { top, right, bottom, left }, style: { ... }, width: { ... } } but for inline style is extracted as border-top, border-left, etc - which combines all of them. Another case is extracting borer-top-color longhand property. This is and edge-case (I didn't analyze other CSS properties) as most of the implemented properties are two-level values.
4. Upcast conversion requires a solution for automatic styles consuming (implmenented as consumeAlso mapping (should be moved somewhere from ViewConsumable). It solves the problem of consuming single entry from a shorthand. The first example is indented block converter which consumes only margin-left (in LTR, I'll skip all the RTL variants) so IMO the margin style should be consumed also but margin-right, margin-top and margin-bottom should be available for other converters to process. Similarly, if some converter consumes margin all margin- longhands should be consumed.
5. Next steps, possible enhancements: font shorthand (... )

What this solves?

The main idea is that inline style can be written in many, many ways — especially the border shorthand sets 12 longhand properties (border-top-color, border-left-width, etc). Also, at least for the border, there are another 7 shorthands: border-top, border-right, ... and border-color, border-style, border-width which overlaps each other. Writing converters for each possible combination would result in around 20 converters (12 longhands, 8 shorthands) that could interfere with each other. This isn't a problem for styles coming from CKEditor as we always control this. The problem occurs if the styles come from other editors, pages, etc - there are various ways of writing the same and we can solve most of them by normalizing incoming styles.

Also it should parse shorthand properly as, ie for border shorthand the order of values doesn't matter so: border: 1px solid blue === border:solid blue 1px === border: blue 1px solid.

Anather thing is to simplify converters - especially the partial ones (margin-right).

[jodator]

So after ~58 commits this PR has:

• most of the @Reinmar's comments included
• documentation added in various places but it still lacks some general guide
• I've refactored the code from events to maps because events will be slower then map lookup
• maybe some concepts (reducer, normalizer) requires better names but I'm out of creativity...

[jodator]

~59th commit: year bump :P

[Reinmar]

Welcome back, i/3287! 🤚

[Reinmar]

JSDoc started...
Incorrect link: module:engine/view/styles/background~addMarginRules
	at /Users/p/Workspace/ckeditor5/packages/ckeditor5-engine/src/view/document.js:175

Invalid return type: Array.<Array.<String, String>>.
	at /Users/p/Workspace/ckeditor5/packages/ckeditor5-engine/src/view/stylesmap.js:498

[jodator]

@Reinmar done.

[Reinmar]

      using offset
        ✔ should not convert class to indent attribute
        left–to–right content
          ✔ should convert margin-left to indent attribute (known offset)
          ✔ should convert margin-left to indent attribute (any offset)
          ✖ should convert margin shortcut to indent attribute (one entry)
          ✖ should convert margin shortcut to indent attribute (two entries)
          ✖ should convert margin shortcut to indent attribute (three entries)
          ✖ should convert margin shortcut to indent attribute (four entries)
        right–to–left content
          ✔ should convert margin-right to indent attribute (known offset)
          ✔ should convert margin-right to indent attribute (any offset)
          ✖ should convert margin shortcut to indent attribute (one entry)
          ✖ should convert margin shortcut to indent attribute (two entries)
          ✖ should convert margin shortcut to indent attribute (three entries)
          ✖ should convert margin shortcut to indent attribute (four entries)

I get the above red tests in ckeditor5-indent for this setup:

│ editor-inline            │ master   │ 02763b0 │        │
│ engine                   │ ! i/3287 │ 709bbb2 │ M1     │
│ enter                    │ master   │ 4848bcb │        │
│ essentials               │ master   │ 792e693 │        │
│ font                     │ ! i/3287 │ 4d1b9fb │ M2     │
│ heading                  │ master   │ 94fbca7 │        │
│ highlight                │ master   │ 9589cc3 │        │
│ horizontal-line          │ master   │ 1391e25 │        │
│ image                    │ master   │ 8f06626 │        │
│ indent                   │ ! i/3287 │ 38e2948 │        │
│ link                     │ master   │ 73d5596 │        │
│ list                     │ master   │ cfe7f8e │        │
│ markdown-gfm             │ master   │ fb5cee3 │        │
│ media-embed              │ master   │ c8ba8bf │        │
│ mention                  │ master   │ 3c207d0 │        │
│ page-break               │ master   │ b2b57e1 │        │
│ paragraph                │ master   │ 92b1a8d │        │
│ paste-from-office        │ master   │ 958662e │        │
│ remove-format            │ master   │ 0a448ac │        │
│ restricted-editing       │ master   │ 5e437b7 │        │
│ special-characters       │ master   │ 58f7cac │        │
│ table                    │ ! i/3287 │ a737502 │        │
│ theme-lark               │ master   │ 5252417 │        │
│ typing                   │ master   │ 139989c │        │
│ ui                       │ ! i/3287 │ 1763666 │        │
│ undo                     │ master   │ 6ce1985 │        │
│ upload                   │ master   │ 2657a60 │        │
│ utils                    │ ! i/3287 │ 3fff7ec │        │
│ watchdog                 │ master   │ 72628be │        │
│ widget                   │ master   │ 096a9f9 │        │
│ word-count               │ master   │ 9270325 │        │
└──────────────────────────┴──────────┴─────────┴────────┘

[jodator]

@Reinmar That's right I didn't check the Indent package. After digging up I've fixed their test on i/3287 by: ckeditor/ckeditor5-indent@9f8d9d5 - you see how the code get uglier. The other thing to do is to leave previous two converters for margin and margin-left. That would, however, feel meh as we introduced those fancy normalizations.

Now back to your comment about consumables - when fixing this PR I've forgotten about that case so I've just removed that code which did 2 things:

1. Updated ViewConsumable class logic with mappings for margin => margin-top, `margin-right``, etc.
2. Added logic for consuming longhand value if one of shorthand was consumed (ie when consuming margin-left the margin was automatically consumed.

The point 2. does look like too much but point 1 looks useful for conversion helpers.

How to repair this?

1. Make getStyleNames() -> more clever and return more values (with a switch) (ie all "consumable"/"possible" so margin,margin-top,margin-right,... for margin:1px (all 4 values set).
2. Make ViewConsumable smarter if style is enabled.

Right now the logic for creating view consumable names gets the style names from view element and uses only them. In other words, testing for consuming margin-top will fail if a style is margin: 1px because viewElement.getStyleNames() will return only margin and only this value will be created to create consumable entries.

ps.: this might be a follow-up as indent PR is optional - master works OK. I've used Indent to validate the whole idea of normalizing styles and how we could use that in other places. In this scenario writing a converter for longhand value (margin-top) should be enough (assuming style normalization) for any margin style definition. This also could be used with borders and the shitload of shorthands for writing a bit nicer converters for border-style, border-color and border-width as current converter is not ideal: https://github.com/ckeditor/ckeditor5-table/blob/966bd7d271eba4ff6a0507a9cc5fc075c78b2749/src/converters/tableproperties.js#L40.

[Reinmar]

We've had a F2F discussion with @jodator and @scofalik and decided that:

• All shorthand and longhand property names will become consumable. It makes sense to have Element#getStyleNames( { includeAllCombinations } ) to return those names.
• Consuming a longhand property (e.g. margin-left) will consume margin too.
• The goal is to keep converters as simple as possible (ideally, so they don't need to be aware of anything that we're doing here).

[jodator]

• All shorthand and longhand property names will become consumable. It makes sense to have

@Reinmar I've gone with a previous logic but re-implemented with an ability to "enable" it in view consumable.

The idea of consuming "also" works the same (I've restored previous tests).

The only different thing which I've done in my commit is that I've restored the previous "consumeAlso" logic without element.getStylename( { includeAllCombinations } ). Let me know what you think of it. Eventual change to move that logic to getStyleNames( { inludeAllCombinations: true } ) will be quick as the API for defining this mappings is registered by editing.view.document as other style processing rules.

[Reinmar]

I created a followup for the border processor as I don't like how it works: ckeditor/ckeditor5#6090. Not necessarily it29 material though.

[Reinmar]

I found a quite big problem with our style processor singleton: ckeditor/ckeditor5-utils#312 (comment) and I'm having troubles figuring out what we could do with it.

[Reinmar]

I found a quite big problem with our style processor singleton: ckeditor/ckeditor5-utils#312 (comment) and I'm having troubles figuring out what we could do with it.

Extracted that to ckeditor/ckeditor5#6091.

Other than that, I think we're good to merge these 4 PRs.