Bug 187632 – Dark Mode: document markers are difficult to see

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Antoine Quint]

The contrast of the document markers, especially for auto-corrected words (blue), is quite bad. We should update these colours in this mode.

[Antoine Quint]

<rdar://problem/41099719>

[Antoine Quint]

Created attachment 344922 [details]
Patch

[Tim Horton]

Comment on attachment 344922 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344922&action=review

> Source/WebCore/platform/graphics/GraphicsContext.h:112
> +enum class DocumentMarkerLineStyle {

: uint8_t, didn’t you see Simon’s slides :)

These aren’t stored much, but they are recorded in display lists!!

> Source/WebCore/rendering/RenderThemeMac.mm:2784
> +static RetainPtr<CGColorRef> colorWithComponents(int r, int g, int b, int a)
> +{
> +    CGFloat components[4] = { CGFloat(r / 255.0), CGFloat(g / 255.0), CGFloat(b / 255.0), CGFloat(a / 100.0) };
> +    return adoptCF(CGColorCreate(sRGBColorSpaceRef(), components));
> +}

This is pretty goofy because A) it doesn’t use our platform color cache, and B) you’ve invented your own color format where alpha is 0-100.

> Source/WebCore/rendering/RenderThemeMac.mm:2810
> +    auto dotSize = CGSizeMake(patternHeight, patternHeight);

What’s the point of dotSize? This is a weirdly roundabout way to propagate patternHeight to the two places it goes below :)

> Source/WebCore/rendering/RenderThemeMac.mm:2828
> +    for (int numberOfDots = 0; numberOfDots * patternWidth < width; ++numberOfDots)
> +        CGContextAddEllipseInRect(ctx, CGRectMake(offsetPoint.x() + numberOfDots * patternWidth, offsetPoint.y(), dotSize.width, dotSize.height));

I think I would make the iterator be the x position instead of numberOfDots and increment by the patternWidth and then this would be less repeaty, but it doesn’t really matter.

[Antoine Quint]

(In reply to Tim Horton from comment #3)
> Comment on attachment 344922 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344922&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext.h:112
> > +enum class DocumentMarkerLineStyle {
> 
> : uint8_t, didn’t you see Simon’s slides :)

I don't know, did I?

> These aren’t stored much, but they are recorded in display lists!!

Changing the enum to uint8_t.
 
> > Source/WebCore/rendering/RenderThemeMac.mm:2784
> > +static RetainPtr<CGColorRef> colorWithComponents(int r, int g, int b, int a)
> > +{
> > +    CGFloat components[4] = { CGFloat(r / 255.0), CGFloat(g / 255.0), CGFloat(b / 255.0), CGFloat(a / 100.0) };
> > +    return adoptCF(CGColorCreate(sRGBColorSpaceRef(), components));
> > +}
> 
> This is pretty goofy because A) it doesn’t use our platform color cache, and
> B) you’ve invented your own color format where alpha is 0-100.

What I think is goofy is to use a 0-255 range for alpha, but it's OK, I'll use makeRBA() and Color().

> > Source/WebCore/rendering/RenderThemeMac.mm:2810
> > +    auto dotSize = CGSizeMake(patternHeight, patternHeight);
> 
> What’s the point of dotSize? This is a weirdly roundabout way to propagate
> patternHeight to the two places it goes below :)

I'll remove it.

> > Source/WebCore/rendering/RenderThemeMac.mm:2828
> > +    for (int numberOfDots = 0; numberOfDots * patternWidth < width; ++numberOfDots)
> > +        CGContextAddEllipseInRect(ctx, CGRectMake(offsetPoint.x() + numberOfDots * patternWidth, offsetPoint.y(), dotSize.width, dotSize.height));
> 
> I think I would make the iterator be the x position instead of numberOfDots
> and increment by the patternWidth and then this would be less repeaty, but
> it doesn’t really matter.

I will take this suggestion!

[Antoine Quint]

Created attachment 344968 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 344968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344968&action=review

> Source/WebCore/rendering/RenderTheme.h:256
> +    virtual void drawLineForDocumentMarker(const RenderText&, GraphicsContext&, const FloatPoint&, float, DocumentMarkerLineStyle);

Please give names to the parameters whose roles are not obvious.

> Source/WebCore/rendering/RenderThemeMac.h:171
> +    void drawLineForDocumentMarker(const RenderText&, GraphicsContext&, const FloatPoint&, float, DocumentMarkerLineStyle) final;

Likewise.

> Source/WebCore/rendering/RenderThemeMac.mm:2803
> +    auto patternWidth = cMisspellingLinePatternWidth;

Why not just use cMisspellingLinePatternWidth below?

> Source/WebCore/rendering/RenderThemeMac.mm:2804
> +    auto patternHeight = cMisspellingLineThickness;

Ditto.

> Source/WebCore/rendering/RenderThemeMac.mm:2807
> +    // Make sure to draw only complete dots allowing slightly more considering that the pattern ends with a transparent pixel.

Not sure this comment still makes sense.

[Antoine Quint]

Committed r233814: <https://trac.webkit.org/changeset/233814>

[Daniel Bates]

Comment on attachment 344968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344968&action=review

> Source/WebCore/ChangeLog:16
> +        No new test due to webkit.org/b/105616, webkit.org/b/187655 was raised to track the creation of new tests

webkit.org/b/105616 is about spell checking not working in WebKit2. Spell checkers work in WebKitLegacy. We have existing tests for spelling dots that run without issue in DumpRenderTree. One such example is <https://trac.webkit.org/browser/trunk/LayoutTests/editing/spelling/spelling-marker-includes-hyphen.html?rev=223013>. Clearly with such support we can write a pixel test though it would be better to write tests using some other strategy as we do not run pixel tests on the bots. It may be non-trivial to write a ref test for this :/

[Simon Fraser (smfr)]

A ref test should be easy. Just scale up 20x with a transform, and clip to show the interior of a couple of dots.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #9)
> A ref test should be easy. Just scale up 20x with a transform, and clip to
> show the interior of a couple of dots.

The problem has been getting the dots to draw _at all_ in DRT/WKTR.

[Simon Fraser (smfr)]

Just add testing SPI to explicitly mark a range as having spelling dots.