Bug 234707 – REGRESSION(r286955): Fix painting text-decorations with combined text

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Tim Nguyen (:ntim)]

I screwed this up in bug 227445

WPT: imported/w3c/web-platform-tests/css/css-writing-modes/text-combine-upright-decorations-001.html

[Radar WebKit Bug Importer]

<rdar://problem/86944442>

[Tim Nguyen (:ntim)]

Created attachment 448007 [details]
Patch

[Tim Nguyen (:ntim)]

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/32204

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Tim Nguyen (:ntim)]

Created attachment 448011 [details]
Screenshot of bug

[Tim Nguyen (:ntim)]

Created attachment 448012 [details]
Expected result

[Darin Adler]

Comment on attachment 448007 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Test: imported/w3c/web-platform-tests/css/css-writing-modes/text-combine-upright-decorations-001.html

Has this test been failing? If not, then how is this test covering the bug?

We need a test that can detect the bug, right?

> LayoutTests/imported/w3c/web-platform-tests/css/css-writing-modes/text-combine-upright-decorations-001.html:28
> +    -webkit-text-combine: horizontal;

Why are we making this change to a WPT test? Is this being upstreamed? Do the other browser vendors want this test to have a "-webkit-" prefixed property in it?

Change log doesn’t explain any of this.

[Tim Nguyen (:ntim)]

(In reply to Darin Adler from comment #7)
> Comment on attachment 448007 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448007&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Test: imported/w3c/web-platform-tests/css/css-writing-modes/text-combine-upright-decorations-001.html
> 
> Has this test been failing? If not, then how is this test covering the bug?
> 
> We need a test that can detect the bug, right?

It hasn't been failing because it uses text-combine-upright (and we support -webkit-text-combine instead). If you add `-webkit-text-combine`, the test starts failing. 

Bug 150821 aliases `-webkit-text-combine: horizontal` to `text-combine-upright: all` (and the test does fail there). 

> > LayoutTests/imported/w3c/web-platform-tests/css/css-writing-modes/text-combine-upright-decorations-001.html:28
> > +    -webkit-text-combine: horizontal;
> 
> Why are we making this change to a WPT test? Is this being upstreamed? Do
> the other browser vendors want this test to have a "-webkit-" prefixed
> property in it?

I'm planning to upstream it: https://github.com/web-platform-tests/wpt/pull/32204 (this PR should get approved automatically when this patch gets r+). It's not uncommon practice to add vendor prefixes if the main thing being tested isn't the syntax itself, see current usages: https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/web-platform-tests/wpt+-webkit-text-combine&patternType=literal

Once bug 150821 lands, I'll remove usages of -webkit-text-combine in the WPT repo.

---

Alternatively, we can land bug 150821 first, and then remove the ImageOnlyFailure test expectation in this bug.


> Change log doesn’t explain any of this.

I'll add explanations as appropriate before landing.

[Dean Jackson]

Comment on attachment 448007 [details]
Patch

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

>>> Source/WebCore/ChangeLog:8
>>> +        Test: imported/w3c/web-platform-tests/css/css-writing-modes/text-combine-upright-decorations-001.html
>> 
>> Has this test been failing? If not, then how is this test covering the bug?
>> 
>> We need a test that can detect the bug, right?
> 
> It hasn't been failing because it uses text-combine-upright (and we support -webkit-text-combine instead). If you add `-webkit-text-combine`, the test starts failing. 
> 
> Bug 150821 aliases `-webkit-text-combine: horizontal` to `text-combine-upright: all` (and the test does fail there).

OK, so 150821 marks this test as ImageOnlyFailure. But this patch doesn't remove that expectation?

> Source/WebCore/rendering/TextBoxPainter.cpp:397
> +        GraphicsContext& context = m_paintInfo.context();

Is there value in having the local variable here? (and below)

>>> LayoutTests/imported/w3c/web-platform-tests/css/css-writing-modes/text-combine-upright-decorations-001.html:28
>>> +    -webkit-text-combine: horizontal;
>> 
>> Why are we making this change to a WPT test? Is this being upstreamed? Do the other browser vendors want this test to have a "-webkit-" prefixed property in it?
>> 
>> Change log doesn’t explain any of this.
> 
> I'm planning to upstream it: https://github.com/web-platform-tests/wpt/pull/32204 (this PR should get approved automatically when this patch gets r+). It's not uncommon practice to add vendor prefixes if the main thing being tested isn't the syntax itself, see current usages: https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/web-platform-tests/wpt+-webkit-text-combine&patternType=literal
> 
> Once bug 150821 lands, I'll remove usages of -webkit-text-combine in the WPT repo.
> 
> ---
> 
> Alternatively, we can land bug 150821 first, and then remove the ImageOnlyFailure test expectation in this bug.

Now I get it. Yeah, I think this patch should land after 150821 and change the expectation to pass.

[Tim Nguyen (:ntim)]

Created attachment 448112 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 448113 [details]
Patch

[EWS]

Committed r287488 (245623@main): <https://commits.webkit.org/245623@main>

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