Bug 245055

Created attachment 462271 [details]
WIP patch

This patch doesn't make the tests pass. I don't know why. They are almost same color for my eyes.

RenderTheme::transformSelectionBackgroundColor is using blendWithWhite to convert the value of platformAnnotationHighlightColor.
blendWithWhite tries to find the corresponding color if the givin color is blended with white.
I think it's OK to add fuzzy meta tag in these test cases.

Created attachment 462274 [details]
Patch

Created attachment 462277 [details]
Patch

Comment on attachment 462277 [details]
Patch

gtk-wk2 EWS reported test failures:

  http/tests/scroll-to-text-fragment/start-text-emoji.html [ ImageOnlyFailure ]
  http/tests/scroll-to-text-fragment/start-text-sentence.html [ ImageOnlyFailure ]
  http/tests/scroll-to-text-fragment/start-text-word.html [ ImageOnlyFailure ]

I didn't know that GTK has yellow -expected.html files. I'm going to remove them.

Created attachment 462296 [details]
Patch

Comment on attachment 462296 [details]
Patch

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

> Source/WebCore/rendering/RenderTheme.cpp:1430
>  Color RenderTheme::platformAnnotationHighlightColor(OptionSet<StyleColorOptions>) const
>  {
> -    return Color::yellow;
> +    return { { 255, 238, 190 } };
>  }

I'll leave it to Aditya, but I wonder if it's really useful at this point to keep the RenderThemeIOS/Mac methods given they also just return this hardcoded color.

Comment on attachment 462296 [details]
Patch

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

>> Source/WebCore/rendering/RenderTheme.cpp:1430
>>  }
> 
> I'll leave it to Aditya, but I wonder if it's really useful at this point to keep the RenderThemeIOS/Mac methods given they also just return this hardcoded color.

I agree. Will fix.

Created attachment 462304 [details]
Patch

I had a discussion in https://webkit.slack.com/archives/CU81QR94P/p1663044973078139

Darin Adler
> Here’s why I didn’t review yet:
> I am puzzled that we are changing the actual color displayed on the screen to match Apple’s colors. The reason this is in RenderTheme is the concept that this is not necessarily the same on all platforms. That‘s what "theme" is about. Is this really the best behavior for all ports? If so, then why do we indirect this through the theme?
> Obviously not theming a color is the simplest way to use a single set of graphical results in our regression tests, but outside of that, maybe there are reasons to match platform standards rather than just matching Apple’s style. If we don’t happen to want the same color everywhere, for some of our testing we could even set a mode where all platforms use the same color and ignore the theme

fujihiro
> all browsers use blue color for hyperlinks by default by using UA stylesheet. It increases the web compatibility and reduces the user confusing. I think it isn't a bad idea to use the same annotation color for all WebKit engines. But, your opinion makes sense. How about an idea changing the ref test to a non-match ref test?

Created attachment 462380 [details]
Patch

Comment on attachment 462380 [details]
Patch

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

> COMMIT_MESSAGE:9
> +The annotation highlight color is platform-dependent that is customize
> +by RenderTheme::platformAnnotationHighlightColor virtual function. The
> +expectations of the ref-tests hard-coded the annotation highlight
> +color of Cocoa ports. Changed the ref-tests to mismatch ref-tests.

I don’t like this strategy for making the tests work well cross-platform. Mismatch tests are fragile. Any mismatch is a "pass".

Instead I suggest we expose the correct color somehow to the testing machinery so the expected files can use the correct color. Maybe just make an internals function that will return the color. Or if we want to be super-fancy we could find a way add a CSS named color that is there while testing.

We should use very few mismatch tests. They aren’t very good.

<rdar://problem/100096699>

It sounds like a bit complicated.
I prefer an idea to use platform-specific expectations because it's much simpler.

I found out another problem.
GTK port has yellow test expectations for some tests, but they are failing.
https://build.webkit.org/results/GTK-Linux-64-bit-Release-Tests/254602@main%20(9198)/results.html
maxDifference is 51.
Does blendWithWhite have a problem?

Because yellow has 0 blue, no semi-transparent color can't result in yellow by blending with white.
So, blendWithWhite falls back to return a color with 80% alpha.
https://github.com/WebKit/WebKit/blob/cfb088fd56631a371a89d06697cfba363583d33d/Source/WebCore/platform/graphics/ColorBlending.cpp#L57

Created attachment 462452 [details]
Patch

Created attachment 462463 [details]
[fast-cq] Patch for landing

Committed 254664@main (c2c84973bcac): <https://commits.webkit.org/254664@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 462463 [details].