Bug 245055 – Add platform-specific expectations of http/tests/scroll-to-text-fragment ref-tests for WinCairo, GTK and WPE port

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Fujii Hironori]

RenderTheme::platformAnnotationHighlightColor is yellow which isn't matching to RenderThemeMac and RenderThemeIOS

Non-Cocoa ports are failing the following tests because the annotation highlight color is yellow.

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

See also: Bug 236693 – Draw highlights for Scroll To Text Fragment

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 462274 [details]
Patch

[Fujii Hironori]

Created attachment 462277 [details]
Patch

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 462296 [details]
Patch

[Tim Nguyen (:ntim)]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 462304 [details]
Patch

[Fujii Hironori]

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?

[Fujii Hironori]

Created attachment 462380 [details]
Patch

[Darin Adler]

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.

[Radar WebKit Bug Importer]

<rdar://problem/100096699>

[Fujii Hironori]

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

[Fujii Hironori]

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?

[Fujii Hironori]

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

[Fujii Hironori]

Created attachment 462452 [details]
Patch

[Fujii Hironori]

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

[EWS]

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].