Delan Azabani

Chromium spelling and grammar, part 2

 4978 words 27 min  home igalia

Modern web browsers can help users with their word processing needs by drawing squiggly lines under possible spelling or grammar errors in their input. CSS will give authors more control over when and how they appear, with the new ::spelling- and ::grammar-error pseudo-elements, and spelling- and grammar-error text decorations. Since part 1 in May, we’ve done a fair bit of work in both Chromium and the CSSWG towards making this possible.

The client funding this work had an internal patch that allowed you to change the colors of those squiggly lines, and our job was to upstream it. The patch itself was pretty simple, but turning that into an upstream feature is a much bigger can of worms. So far, we’ve landed over 30 patches, including dozens of new web platform tests, opened 8 spec issues, and run into some gnarly bugs going back to at least 2009.

Check out our project index for a complete list of demos, tests, patches, and issues. For more details about the CSS highlight pseudos in particular, check out my BlinkOn 15 talk, including the highlight painting visualiser.

(slides)

Contents

Implementation status

Chromium 96 includes a rudimentary version of highlight inheritance, with support for ::highlight in Chromium 98 (Fernando Fiori). This is currently behind a Blink feature:

--enable-blink-features=HighlightInheritance

Adding to our initial support for ::{spelling,grammar}-error, we’ve since made progress on the new {spelling,grammar}-error decorations. While they are accepted but ignored in Chromium 96, you’ll be able to see them in Chromium 98, with our early paint support.

Chromium 96 also makes it possible to change the color of native squiggly lines by setting ‘text-decoration-color’ on either of the new pseudo-elements. This feature, and the features above, are behind another flag:

--enable-blink-features=CSSSpellingGrammarErrors

Charlie’s bird spec lawyerings

I’ve learned a lot of things while working on this project. One interesting lesson was that no matter how clearly a feature is specified, and how much discussion goes into spec details, half the questions won’t become apparent until someone starts building it.

Squiggly lines

Since landing ‘text-decoration-color’ support for the new pseudos, my colleague Rego has taken the lead on the rest of the core spelling and grammar features, starting with the new ‘text-decoration-line’ values.

Currently, when setting ‘text-decoration-color’ on the pseudos, we change the color, but ‘text-decoration-line’ is still ‘none’, which doesn’t really make sense. This might sound like it required gross hacks, but the style system just gives us a blob of properties, where ‘color’ and ‘line’ are independent. All of the logic that uses them is in paint and layout.

CL:3162169

We started by adding the new values to the stylesheet parser. While highlight painting still needs a lot more work before we can do so, the idea is that eventually the pseudos and decorations will meet in the default stylesheet.

::spelling-error { text-decoration-line: spelling-error; }
::grammar-error { text-decoration-line: grammar-error; }
CL:3194336

Something that’s often neglected in CSS tests is dynamic testing, which checks that the rendering updates correctly when styles are changed by JavaScript, since the easiest and most common way to write a rendering test involves no scripting at all.

In this case, only ::selection had dynamic tests, and only ::selection actually worked correctly, so we then fixed the other pseudos.

CL:3177663

Platform “conventions”

Blink’s squiggly lines look quite different to anything CSS can achieve with wavy or dotted decorations, and they’re painted on unrelated codepaths (more details). We want to unify these codepaths, to make them easier to maintain and help us integrate them with CSS, but this creates a few complications.

The CSS codepath naïvely paints as many bézier curves as needed to span the necessary width, but the squiggly codepath has always painted a single rectangle with a cached texture, which is probably more efficient. This texture used to be a hardcoded bitmap, but even when we made the decorations scale with the user’s dpi, we still kept the same technique, so the approach we use for CSS decorations might be too slow.

Another question is the actual appearance of spelling and grammar decorations. We don’t necessarily want to make them identical to the default wavy or dotted decorations, because it might be nice to tell when, say, a wavy-decorated word is misspelled.

We also want to conform to platform conventions where possible, and you would think there’s at least a consistent convention for macOS… but not exactly. One thing that’s clear is that gradients are no longer conventional.

macOS (compare demo0)

SafariNotesTextEditKeynote
CL:3139819

But anyway, if we’re adding new decoration values that mimic the native ones, which codepath do we paint them with? We decided to go down the CSS route — leaving native squiggly lines untouched for now — and take this time to refactor and extend those decoration painters for the needs of spelling and grammar errors.

CL:3275457
CL:3284869
CL:3290417
CL:3291658
CL:3297885

Precise wavy decorations

To that end, one of the biggest improvements we’ve landed is making wavy decorations start and stop exactly where needed, rather than falling short. This includes the new spelling and grammar decoration values, other than on macOS.

Wavy decorations under ‘letter-spacing’, top version 96, bottom version 97 (demo0).
CL:3237072
CL:3264203

You may have noticed that the decorations in that last example sometimes extend to the right of “h”. This is working as expected: ‘letter-spacing’ adds a space after letters, not between them, even though it Really Should Not. I tried wrapping the last letter of each word in a span, but then the letter appears to have its own decoration, out of phase with the rest of the word. This is because Blink lacks phase-locked decorations.

Phase-locked decorations

Blink uses an inheritance hack to propagate decorations from parents to children, rather than properly implementing the concept of decorating box. In other words, we paint two independent decorations, whereas we should paint one decoration that spans the entire word. This has been the cause of a lot of bugs, and is widely regarded as a bad move.

Note that we don’t actually have to paint the decoration in a single pass, we only have to render as if that was the case. For example, when testing the same change in Firefox, the decoration appears to jitter near the last letter, which suggests that the decoration is probably being painted separately for that element.

Gecko goes above and beyond with this, even synchronising separate decorations introduced under the same block, which allows authors to make it look like their decorations change color partway through.

A related problem in the highlight painting space is that the spec calls for “recoloring” originating decorations to the highlight foreground color. By making these decorations “lose their color”, we avoid situations where a decoration becomes illegible when highlighted, despite being legible in its original context.

I’ve partially implemented this for ::selection in Chromium 95, by adding a special case that splits originating decorations into two clipped paints with different colors — though not yet the correct colors — while carefully keeping them in phase.

highlight-painting-004 and -ref3, version 97. In this test, the originating element has a red underline, while ::selection introduces a purple line-through. The underline needs to become blue in the highlighted part, to match the ::selection ‘color’, but for now, we match its ‘text-decoration-color’.

To paint the highlighted part of the decoration, we clip the canvas to a rectangle as wide as the background, and paint the decoration in full. To paint the rest, we clip “out” the canvas to the same rectangle, which means we don’t touch anything inside the rectangle.

But how tall should that rectangle be? Short answer: infinity.

Bézier bounding box

Long answer: Skia doesn’t let us clip to an infinitely tall rectangle, so it depends on several things, including ‘text-decoration-thickness’, ‘text-underline-offset’, and in the case of wavy decorations, the amplitude of the bézier curves.

In the code, there was a pretty diagram that illustrated the four relevant points to each “wave” repeated in the decoration. Clearly, it suggested that the pattern in that example was bounded by the control points, but I had no idea whether this was true for all cubic béziers, my terrible search engine skills failed me again, and I don’t like assuming.

/*                   controlPoint1
 *                         +
 *
 *
 *                  . .
 *                .     .
 *              .         .
 * (x1, y1) p1 +           .            + p2 (x2, y2)
 *                          .         .
 *                            .     .
 *                              . .
 *
 *
 *                         +
 *                   controlPoint2
 */

To avoid getting stuck on those questions for too long, and because I genuinely didn’t know how to determine the amplitude of a bézier curve, I went with three times the background height. This should be Good Enough™ for most content, but you can easily break it with, say, a very large ‘text-underline-offset’.

Weeks later, I stumbled upon a video by Freya Holmér answering that very question.

So, how do we get [the bounding] box?

The naïve solution is to simply use the control points of the bézier curve. This can be good enough, but what we really want is the “tight bounding box”; in some cases, the difference between the two is huge.

For now, the code still clips to a fixed three times the background height, but at least we now have some ideas for how to properly measure these decorations:

Cover me!

Writing the reference pages for that test was also a fun challenge. When written the obvious way, Blink would actually fail, because in general we make no attempt to keep any decoration paints in phase.

highlight-painting-004-ref1 and -ref3, version 96.

The ref that Blink ended up matching has five layers. Each layer contains the word “quick” in full with any decorations spanning the whole word, but only part of the layer is shown. This is achieved by an elaborate system of positioned “covers” and “hiders”: the former clips a layer from the right with a white rectangle, while the latter clips a layer from the left by way of right:0 wrapped in overflow:hidden.

Wanna know the best part though? All three refs are identical in Firefox. Someday, hopefully, this will also be true for Blink.

Highlight inheritance

Presto (Opera), uniquely, supported inheritance for ::selection before it was cool, by mapping those styles to synthesised (internal) ‘selection-color’ and ‘selection-background’ properties that were marked as inherited.

Blink also has internal properties for things like :visited links and forced colors, where we need to keep track of both “original” and “new” colors. This works well enough, but internal properties add a great deal of complexity to the code that applies and consumes styles. Now that there are multiple highlight pseudos, supporting a lot more than just ‘color’ and ‘background-color’, this complexity is hard to justify.

To understand the approach we went with, let’s look at how CSS works in Chromium.

CSS is managed by Blink’s style system, which at its highest level consists of the engine, the resolver, and the ComputedStyle data structure. The engine maintains all of the style-related state for a document, including all of its stylesheet rules and the information needed to recalculate styles efficiently when the document changes. The resolver’s job is to calculate styles for some element, writing the results to a new ComputedStyle object.

ComputedStyle itself is also interesting. Blink recognises over 600 properties, including internal properties, shorthands (like ‘margin’), and aliases (like ‘-webkit-transform’), so most of the fields and methods are actually generated (ComputedStyleBase) with the help of some Python scripts.

These fields are “sharded” into field groups, so we can efficiently reuse style data from ancestors and previous resolver outputs. Some of these field groups are human-defined, like “surround” for all of the margin/border/padding properties, but there are also several raredata groups generated from property popularity stats.

When resolving styles, we usually clone an “empty” ComputedStyle, then we copy over the inherited properties from the parent to this fresh new object. Many of these live in the “inherited” field group, so all we need to do for them is copy a single pointer. At this point, we have the parent’s inherited properties, and everything else as initial values, so if the element doesn’t have any rules of its own, we’re more or less done.

Otherwise, we search for matching rules, sort all of their declarations by things like specificity, then apply the winning declarations by overwriting various ComputedStyle fields. If the field we’re overwriting is in a field group, we need to clone the field group too, to avoid clobbering someone else’s styles.

For ordinary elements, as well as pseudo-elements with a clear place in the DOM tree (e.g. ::before, ::marker), we resolve styles as part of style’s regular tree traversal. We start by updating :root’s styles, then any children affected by the update, and so on. But for other pseudos we usually use a “lazy” approach, where we don’t bother resolving styles unless they are needed by a later phase of the rendering process, like layout or paint.

Let’s say we’re resolving styles for some ordinary element. When we’re searching for matching rules, if we find one that actually matches our ::selection, we make a note in our pseudo bits saying we’ve seen rules for that pseudo, but otherwise ignore the rule.

Once we’re in paint, if the user has selected some text, then we need to know our ::selection styles, so we check our pseudo bits. If the ::selection bit was set, we call our resolver with a special request for pseudo styles, then cache the result into a vector inside the originating element’s ComputedStyle.

This is how ::selection used to work, and at first I tried to keep it that way.

Status quo

My initial solution was to make paint pass in a custom inheritance parent with its style request. Normally pseudo styles inherit from the originating element, but here they would inherit from the parent’s highlight styles, which we would obtain recursively. Then in the resolver, if we’re dealing with a highlight, we copy non-inherited properties too.

On the surface, this worked, but to make it correct, we had to work around an optimisation where the resolver would bail out early if there were no matching rules. Worse still, we had to bypass the pseudo cache entirely. While we already had to do so under :window-inactive, the performance penalty there was at least pretty contained.

PS7

If we copy over the parent’s inherited properties as usual, and for highlights, copy the non-inherited properties too, that more or less means we’re copying all the fields, so why not do away with that and just clone the parent’s ComputedStyle?

PS10

The pseudo cache is only designed for pseudos whose styles won’t need to change between the originating element’s style updates. For most pseudos, this is true anyway, as long as we bypass the cache under pseudo-classes like :window-inactive.

These caches are rarely actually cleared, but when the next update happens, the whole ComputedStyle — including the cache — gets discarded. Caching results with custom inheritance parents is usually frowned upon, because changing the parent you inherit your styles from can yield different styles. But for highlights, we will always have the same parent throughout an update cycle, so surely we can use the cache here?

PS13

…well, yes and no.

Given an element that inherits a bunch of highlight styles, the initial styles are correct. But when those inherited values change in some ancestor, our highlight styles fail to update! This is a classic cache invalidation bug. Our invalidation system wasn’t even the problem — it’s just unaware of lazily resolved styles in pseudo caches. This is usually fine, because most pseudos inherit from the originating element, but not here.

Storing highlight styles

With the pseudo cache being unsuitable for highlight styles, we needed some other way of storing them. Only a handful of properties are allowed in highlight styles, so why not make a dedicated type with only those fields?

The declarations and basic methods for CSS properties are entirely generated, so let’s write some new templates…

{% macro declare_highlight_class(name, fields, field_templates): -%}
class {{name}} : public RefCounted<{{name}}> {
 public:
  static scoped_refptr<{{name}}> Create() { /* ... */ }
  scoped_refptr<{{name}}> Copy() const { /* ... */ }
  bool operator==(const {{name}}& other) const { /* ... */ }
  bool operator!=(const {{name}}& other) const { /* ... */ }
  {% for field in fields %}
  {{declare_storage(field)}}
  {% endfor %}
  {% for field in fields %}
  {{field_templates[field.field_template]
      .decl_public_methods(field.without_group())
    |indent(2)}}
  {% endfor %}
 private:
  {{name}}();
  CORE_EXPORT {{name}}(const {{name}}&);
};
{%- endmacro %}

…then use them in the ComputedStyleBase template.

{{declare_highlight_class(
    'StyleHighlightData',
    computed_style.all_fields
        |sort(attribute='name')
        |selectattr('valid_for_highlight')
        |list,
    field_templates)
  |indent(2)}}
PS16

Trouble is, all of the methods that apply and serialise property values — and there are hundreds of them — take a ComputedStyle, not some other type.

const blink::Color Color::ColorIncludingFallback(
    bool visited_link,
    const ComputedStyle& style) const { /* ... */ }

const CSSValue* Color::CSSValueFromComputedStyleInternal(
    const ComputedStyle& style,
    const LayoutObject*,
    bool allow_visited_style) const { /* ... */ }

Combined with the fact that our copy-on-write field groups mitigate a lot of the wasted memory (well hopefully anyway), we quickly abandoned this dedicated type.

PS25

We then optimised the top-level struct a bit, saving a few pointer widths by moving the four highlight style pointers into a separate type, but this was still less than ideal. We were widening ComputedStyle by one pointer, but the vast majority of web content doesn’t use highlight pseudos at all, and ComputedStyle and ComputedStyleBase are very sensitive to size changes. To give you an idea of how much it matters, Blink even throws a compile-time error if the size inadvertently changes!

struct SameSizeAsComputedStyleBase {
  SameSizeAsComputedStyleBase() { Alias(&pointers); Alias(&bitfields); }
 private:
  void* pointers[9];
  unsigned bitfields[5];
};

struct SameSizeAsComputedStyle : public SameSizeAsComputedStyleBase,
                                 public RefCounted<SameSizeAsComputedStyle> {
  SameSizeAsComputedStyle() { Alias(&own_pointers); }
 private:
  void* own_pointers[1];
};

ASSERT_SIZE(ComputedStyle, SameSizeAsComputedStyle);

To move highlights out of the top-level and into a raredata group, we had to get rid of all the fancy generated code and Just write a plain struct, which has the added benefit of making the code easier to read. Luckily, we were only using that code to loop through the four highlight pseudos at this point, not dozens or hundreds of properties.

Then all we needed was a bit of JSON to tell the code generator to add an “extra” field, and find an appropriate field group for us ("*"). Because this field is not for a popular CSS property, or a property at all really, it automatically goes in a raredata group.

[{
  name: "HighlightData",
  inherited: true,
  field_template: "external",
  type_name: "StyleHighlightData",
  include_paths: ["third_party/blink/renderer/core/style/style_highlight_data.h"],
  default_value: "",
  wrapper_pointer_name: "DataRef",
  field_group: "*",
  computed_style_custom_functions: ["initial", "getter", "setter", "resetter"],
}]
PS35

Single-pass resolution

With our new storage ready, we now needed to actually write to it. We want to resolve highlight styles as part of the regular style update cycle, so that they can eventually benefit from style invalidation.

Looking at the resolver, I thought wow, there does seem to be a lot of redundant work being done when resolving highlight styles in a separate request, so why not weave highlight resolution into the resolver while we’re at it?

PS36
@@ third_party/blink/renderer/core/css/css_selector.h @@
   enum RelationType {
+    kHighlights,
@@ third_party/blink/renderer/core/css/css_selector.cc @@
       case kShadowSlot:
+      case kHighlights:
@@ third_party/blink/renderer/core/css/element_rule_collector.h @@
   MatchedRule(const RuleData* rule_data,
               unsigned style_sheet_index,
               const CSSStyleSheet* parent_style_sheet,
+              absl::optional<PseudoId> highlight)
@@ third_party/blink/renderer/core/css/resolver/match_result.h @@
   void AddMatchedProperties(
       const CSSPropertyValueSet* properties,
       unsigned link_match_type = CSSSelector::kMatchAll,
       ValidPropertyFilter = ValidPropertyFilter::kNoFilter,
+      absl::optional<PseudoId> highlight = absl::nullopt);
@@ ... @@
   const MatchedPropertiesVector& GetMatchedProperties(
+      absl::optional<PseudoId> highlight) const {
+    DCHECK(!highlight || highlight_matched_properties_.Contains(*highlight));
+    return highlight ? *highlight_matched_properties_.at(*highlight)
                      : matched_properties_;
@@ ... @@
   MatchedPropertiesVector matched_properties_;
+  HeapHashMap<PseudoId, Member<MatchedPropertiesVector>>
+      highlight_matched_properties_;
@@ third_party/blink/renderer/core/css/resolver/style_cascade.h @@
   void Apply(CascadeFilter = CascadeFilter());
+  void ApplyHighlight(PseudoId);
@@ third_party/blink/renderer/core/css/resolver/style_cascade.cc @@
 const CSSValue* ValueAt(const MatchResult& result,
+                        absl::optional<PseudoId> highlight,
@@ ... @@
 const TreeScope& TreeScopeAt(const MatchResult& result,
+                             absl::optional<PseudoId> highlight,
                              uint32_t position) {

In general we must find a less intrusive way to implement this. We can not have |highlight| params on everything.

— andruud, Blink style owner

You know what? Fair enough.

⭯ PS35

Multi-pass resolution

Element::Recalc{,Own}Style are pretty big friends of the style system. They drive the style update cycle by determining how the tree has changed, making a resolver request for the element, and determining which descendants also need to be updated.

This makes them the perfect place to update highlight styles. All we need to do is make an additional resolver request for each highlight pseudo, store it in the highlight data, and bob’s your uncle.

StyleRecalcChange Element::RecalcOwnStyle(
    const StyleRecalcChange change,
    const StyleRecalcContext& style_recalc_context) {
  // ...
  if (new_style) {
    StyleHighlightData* highlights = new_style->MutableHighlightData();
    if (new_style->HasPseudoElementStyle(kPseudoIdSelection)) {
      ComputedStyle* parent = ParentComputedStyle()->HighlightData()->Selection();
      StyleRequest request{kPseudoIdSelection, parent};
      highlights->SetSelection(StyleForPseudoElement(style_recalc_context, parent));
    }
    // kPseudoIdTargetText
    // kPseudoIdSpellingError
    // kPseudoIdGrammarError
    // ...
  }
  // SetComputedStyle(new_style);
  // ...
}
PS43

Pathology in legacy

So far, I had been writing this patch as a replacement for the old inheritance logic, but since we decided to defer highlight inheritance for ::highlight to a later patch, we had to undelete the old behaviour and switch between them with a Blink feature.

Another reason for the feature gate was performance. Of the pages in the wild already using highlight pseudos, most of them probably use universal ::selection rules, if only because of how useless the old model was for more complex use cases.

::selection { color: lime; background: green; }

But ::selection isn’t magic — it literally means *::selection, which makes the rule match everywhere in the ::selection tree. When highlight inheritance is enabled, that means we end up cloning highlight styles for each descendant, only to apply the same property values, which wastes time and memory.

The reality is a bit more complicated than this, because ‘color’ and ‘background-color’ are actually in field groups that would also need to be cloned.

Under the old model, where lack of inheritance made this necessary, *::selection rules suffered from roughly the same problem, but the lazy style resolution meant that time and memory was only wasted on the elements directly containing selected content.

As a result, this will need to be fixed before we can enable the feature for everyone.

PS51

Paired cascade

Next we tried to reimplement paired cascade. For compatibility reasons, ::selection has special logic for the browser’s default ‘color’ and ‘background-color’ (e.g. white on blue), where we only use those colors if neither of them were set by the author. Otherwise, they default to initial values, usually black on transparent.

default on default
+ 
::selection { background: yellow; }
=initial on yellow

The spec says so in a mere 22 words:

The UA must use its own highlight colors for ::selection only when neither color nor background-color has been specified by the author.

Brevity is a good thing, and this seemed clear enough to me in the past. But once I actually had to implement it, I had questions about almost every word (#6386). While they aren’t entirely resolved, we’ve been getting pretty close over the last few weeks.

PS52

Who’s got green?

Much of the remaining work was to fix test failures and other bugs. These included crashes under legacy layout, since we only implemented this for LayoutNG, and functional changes leaking out of the feature gate. One of the reftest failures was also interesting to deal with. Let’s minimise it and take a look.

<!doctype html><meta charset="utf-8">
<title>active selection and background-color (basic)</title>
<style>
    main { color: fuchsia; background: red; }
    main::selection { background: green; }
</style>
<p>Pass if text is fuchsia on green, not fuchsia on red.
<main>Selected Text</main>
<script>/* selectNodeContents(main); */</script>

In the past, the “Selected Text” would render as fuchsia on green, and the test passes. But under highlight inheritance it fails, rendering as initial (black) on green, because we now inherit styles in a tree for each pseudo, not from the originating element.

Selected Text → Selected Text

So if the test is wrong, then how do we fix it? Well… it depends on the intent of the test, at least if we want to Do The Right Thing and preserve that. Clearly the primary intent of the test is ‘background-color’, given the <title>, but tests can also have secondary, less explicit intents. In this case, the flavour text1 even mentions fuchsia!

It might have helped if the test had a <meta name=assert>, an optional field dedicated to conveying intent, but probably not. Most of the assert tags I’ve seen are poorly written anyway, being a more or less verbose adaptation of the title or flavour text, and there’s a good chance that the intent for fuchsia (if any) was simply to inherit it from the originating element, so we would still need to invent a new intent.

We could change the reference to initial (black) on green, which would serve as a secondary test that we don’t inherit from the originating element, or remove the existing ‘color’, which would serve as a secondary test for paired cascade. But I didn’t think it through that far at the time, so I gave ::selection a new ‘color’, achieving neither.

 main::selection {
+ color: aqua;
  background: green; }
 </style>
 <p>Pass if text is
- fuchsia
+ aqua
 on green, not fuchsia on red.

Because the selected and unselected text colors were now different, I created another test failure, though only under legacy layout. The reference for this test was straightforward: aqua on green, no mention of fuchsia. This makes sense on the surface, given that all of the text under test was selected.

In this case, the tip of the “t” was crossing the right edge of the selection as ink overflow, and were carefully painting the overflow in the unselected color. The test would have failed under LayoutNG too, if not for an optimisation that skips this technique when everything is selected. Let me illustrate with an exaggerated example:

This behaviour is generally considered desirable, at least when there are unselected characters, so Blink isn’t exactly wrong here. It’s definitely possible to make the active-selection tests account for this — and the tools to do so already exist in the Web Platform Tests — but I don’t have the time to pursue this right now.

PS76

What now?

After the holidays, we plan to:

Other work needed before we can ship the spelling and grammar features:

Special thanks to Rego, Frédéric (Igalia), Rune, andruud (Google), Florian, fantasai (CSSWG), Emilio (Mozilla), and Fernando (Microsoft). We would also like to thank Bloomberg for sponsoring this work.

  1. This is an automated reftest, so the instructions in <p> have no effect on the outcome. We require them anyway, because they add a bit of redundancy that helps humans understand and verify the test’s assertions. ↩