Bug 93832 – text-combine falls back to none rather than scale when wider than 1.1em

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Text

[Koji Ishii]

CSS Writing Modes Level 3 has the text-combine-mode property, and its initial value defines:
  If the contents are wider than 1em, the UA must attempt to fit
  the contents within 1em, but may use any method to do so.
http://dev.w3.org/csswg/css3-writing-modes/#text-combine-mode

Current behavior of WebKit is from old WD:
  The UA may scale the contents to fit instead, or in addition to the method above.
http://www.w3.org/TR/2011/WD-css3-writing-modes-20110531/#text-combine

Note "may scale" was changed to "must attempt to fit" in the current WD. WebKit's current implementation chose to fallback to "none" if wider than 1.1em, but this behavior is no longer conformant.

This issue has been controversial, but after a long discussion, Adobe, W3C I18N WG JLTF, and EPUB Japan ML all agree with the current WD behavior, and it was resolved at CSS WG F2F, so the editors of the spec consider that this is stable now.

Authors and publishers are making feedback that falling back to none is hard to accept, some are using span + writing-mode: horizontal-tb to avoid this. Changing the behavior to follow the new WD is important for authors and publishers.

[Radar WebKit Bug Importer]

<rdar://problem/12086597>

[Yuki Sekiguchi]

Created attachment 158484 [details]
Patch

[Yuki Sekiguchi]

text-combine fallback code remains.
I delete the fallback behavior at Readium project.
https://github.com/readium/Readium-WebKit/issues/2

I will contribute that code from Readium project.

If you want to delete text-combine fallback code soon, please let me know.

[WebKit Review Bot]

Comment on attachment 158484 [details]
Patch

Attachment 158484 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13491854

New failing tests:
fast/writing-mode/combine-compless.html
fast/dynamic/text-combine.html

[WebKit Review Bot]

Created attachment 158489 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Koji Ishii]

* ChangeLog: I think you should mention it's not conformant with the current WD, along with the spec URL, rather than just saying "annoying for many Japanese users".
* *-expected.png: Are they correct? I guess expectations are to text-combine with scale, no?
* You're restoring CTM by multiplying (1 / combinedComplessScale), but I don't think it's reliable nor fast. I think it's better to save/restore.
* Is "m_emWidth = description.computedSize() * textCombineMargin" correct? We should scale to 1em, not to 1.1em, correct?
* I think you should leave a comment in source file too why you always set m_isCombined to true, and fallback code should be removed. I actually think it might be better to remove them, because it reduces footprint, but I can't be sure how reviewers would say, so you may wait for the real reviewer, up to you.

[Yuki Sekiguchi]

Created attachment 158528 [details]
Patch

[Yuki Sekiguchi]

Thank you, Ishii-san for good advice.

Fixed typos..


> * ChangeLog: I think you should mention it's not conformant with the current WD, along with the spec URL, rather than just saying "annoying for many Japanese users".
Fixed.

> * *-expected.png: Are they correct? I guess expectations are to text-combine with scale, no?
*-expected.png is old one.
I just delete the png.

"run-webkit-test" don't execute pixel test.
"run-webkit-test -p" execute pixel test.
But the expectation file may not be for Lion because their scrollbar is Aqua style.
I understand that pixel test is not maintained or cannot generate expectation on Lion.
So I just delete the png.

> * You're restoring CTM by multiplying (1 / combinedComplessScale), but I don't think it's reliable nor fast. I think it's better to save/restore.
I think so.
Fixed.

> * Is "m_emWidth = description.computedSize() * textCombineMargin" correct? We should scale to 1em, not to 1.1em, correct?
Thank you!
It is not correct.
I fixed it.
I don't understand why original author added 10% margin from the original ticket, but it may be to avoid fall back to none.
https://bugs.webkit.org/show_bug.cgi?id=50621
I remove the margin.

> * I think you should leave a comment in source file too why you always set m_isCombined to true, and fallback code should be removed. I actually think it might be better to remove them, because it reduces footprint, but I can't be sure how reviewers would say, so you may wait for the real reviewer, up to you.
Add comment.

[WebKit Review Bot]

Comment on attachment 158528 [details]
Patch

Attachment 158528 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13503586

New failing tests:
fast/dynamic/text-combine.html
fast/writing-mode/combine-compress.html

[WebKit Review Bot]

Created attachment 158539 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Yuki Sekiguchi]

Created attachment 159002 [details]
Patch

[Yuki Sekiguchi]

Update test expectations.

I try to use reftest by the following way, but I cannot get good result.

* Using Ahem and text-combine single character with background black.
  e.g. test: "<span style='background-color: black; -webkit-text-combine: horizontal'>12345</span>", ref: "<span style='background-color: black; -webkit-text-combine: horizontal'>a</span>"
** Result: Some line have 1em x 1px difference.
* Above test + letter-spacing: 0px
** Result: Pixels are same. But png hashes are not same.
* Using Ahem and CSS transform
** Result: I cannot write test because I cannot get description.computedSize() and compute scale.

[Koji Ishii]

It looks to me that combine-compress-expected.png has wrong expectations for emphasis, overline, and link are wrong. Can you check?

[Yuki Sekiguchi]

>emphasis

I checked text-combine with text-emphasis-style behavior.
Emphasis don't work well in Chromium Linux Nightly 20.0.1240.0(152297) on my Ubuntu 12.04.

> overline, and link

Please read my ChangeLog.
text-decoration is known issue.

[Koji Ishii]

(In reply to comment #14)
> >emphasis
> 
> I checked text-combine with text-emphasis-style behavior.
> Emphasis don't work well in Chromium Linux Nightly 20.0.1240.0(152297) on my Ubuntu 12.04.
> 
> > overline, and link
> 
> Please read my ChangeLog.
> text-decoration is known issue.

Ah, I see, thank you. It's probably better to enter a bug, and write the bug #.

> * platform/mac/fast/dynamic/text-combine-expected.png: Removed.
> * platform/mac/fast/text/international/text-combine-image-test-expected.png: Removed.

Why are these two files removed, not rebaselined?

> * platform/chromium-linux/fast/writing-mode/combine-compress-expected.png: Added.
> * platform/chromium-linux/fast/writing-mode/combine-compress-expected.txt: Added.
> * platform/mac/fast/writing-mode/combine-compress-expected.txt: Added.

Why no mac/fast/writing-mode/combine-compress-expected.png?

> RenderCombineText::combineText

It looks to me that, in the original code, when m_isCombined is set, font is set to setOrientation(Horizontal) and RenderText::setTextInternal is called, but the patch changes the rule. Setting m_isCombined = true at line 124 looks better to me. Is this intentional and I'm missing something?

[Koji Ishii]

> combine-compress-expected.html

combine-compress-expected.png is hard to see, especially "compress" line is hardly read. Better to make the font larger.

[Yuki Sekiguchi]

Created attachment 159828 [details]
Patch

[Yuki Sekiguchi]

> Ah, I see, thank you. It's probably better to enter a bug, and write the bug #.

I reported bugs and add the IDs to LayoutTest/ChangeLog.
https://bugs.webkit.org/show_bug.cgi?id=94577
https://bugs.webkit.org/show_bug.cgi?id=94654


> Why are these two files removed, not rebaselined?
> Why no mac/fast/writing-mode/combine-compress-expected.png?

I wrote about it at Comment #8.
https://bugs.webkit.org/show_bug.cgi?id=93832#c8

Summary: I think pixel test of Mac is unmaintained or cannot be generated on Mac OS X Lion.


> It looks to me that, in the original code, when m_isCombined is set, font is set to setOrientation(Horizontal) and RenderText::setTextInternal is called, but the patch changes the rule. Setting m_isCombined = true at line 124 looks better to me. Is this intentional and I'm missing something?

You are right.
If the string overflow line and the glyph is not substituted, font description is vertical and the combined text is not replaced by object replacement character.
Your suggestion may be wrong because font description problem is not fixed.

> combine-compress-expected.png is hard to see, especially "compress" line is hardly read. Better to make the font larger.

I enlarge font of test case.
I replaced combine-compress-expected.png by next patch.

[Yuki Sekiguchi]

Created attachment 160092 [details]
Rebase and Update test expectations

[WebKit Review Bot]

Comment on attachment 160092 [details]
Rebase and Update test expectations

Attachment 160092 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13562579

New failing tests:
fast/dynamic/text-combine.html
fast/writing-mode/combine-compress.html

[WebKit Review Bot]

Created attachment 160097 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

<rdar://problem/11997940>

[Yuki Sekiguchi]

Created attachment 161853 [details]
Change 1em calcuration to fit span's background

[Yuki Sekiguchi]

Created attachment 161960 [details]
Reupload to make WebKit Review Bot work

[WebKit Review Bot]

Comment on attachment 161960 [details]
Reupload to make WebKit Review Bot work

Attachment 161960 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13742346

New failing tests:
fast/dynamic/text-combine.html
fast/writing-mode/combine-compress.html

[Yuki Sekiguchi]

Created attachment 162424 [details]
Add failed test to platform/chromium/TestExpectations

[Yuki Sekiguchi]

Cr-linux is failed because there is no disc space...
That make build bot not to attach result.

I watch failed tests and it works well on my Ubuntu.

But cr-linux environment may be different from my Ubuntu.
So I just add TestExpectation these tests and wait for rebaseline.

[Yuki Sekiguchi]

Created attachment 178711 [details]
Rebase

[WebKit Review Bot]

Attachment 178711 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:143:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yuki Sekiguchi]

Created attachment 178732 [details]
Retry

[WebKit Review Bot]

Comment on attachment 178732 [details]
Retry

Attachment 178732 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15257568

New failing tests:
fast/text/emphasis-combined-text.html
fast/text/international/text-combine-image-test.html

[Yuki Sekiguchi]

Created attachment 178953 [details]
Update TestExpectations

[Koji Ishii]

After discussing with a few experts, I think this patch needs to revise:
1. to remove unused code. Since it always combines, it'd be the right thing to remove m_isCombined = true and also remove where it's false.
2. When only some glyphs do support hwid/twid/qwid but some don't, the current patch combines hwid/twid/qwid versions of glyphs and normal glyphs and then scale. This looks poor. The right fix should be to find the thinnest feature where all glyphs support, and then scale if it's still needed.

Test case should cover #2 (for example, put "#123" where "#" usually doesn't have hwid/twid/qwid).

Another suggestion is that combine-compress test case is too small to see the result, it'd be better to use larger font size.

I'm afraid to say that this means the patch needs to be re-done, sorry about that, but appreciate your understanding.

[Yuki Sekiguchi]

Created attachment 181488 [details]
Patch

[Build Bot]

Comment on attachment 181488 [details]
Patch

Attachment 181488 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15744561

New failing tests:
fast/text/decorations-with-text-combine.html
fast/writing-mode/combine-compress.html
fast/text/international/text-combine-image-test.html

[Yuki Sekiguchi]

Created attachment 181608 [details]
Patch

[Yuki Sekiguchi]

(In reply to comment #33)
> After discussing with a few experts, I think this patch needs to revise:
> 1. to remove unused code. Since it always combines, it'd be the right thing to remove m_isCombined = true and also remove where it's false.
> 2. When only some glyphs do support hwid/twid/qwid but some don't, the current patch combines hwid/twid/qwid versions of glyphs and normal glyphs and then scale. This looks poor. The right fix should be to find the thinnest feature where all glyphs support, and then scale if it's still needed.

Fixed.

> Test case should cover #2 (for example, put "#123" where "#" usually doesn't have hwid/twid/qwid).

Hiragino font have width variant glyph of "#".
It also have "\" and "@".
Therefore, I use japanese character.

> Another suggestion is that combine-compress test case is too small to see the result, it'd be better to use larger font size.

I enlarge font and move expectations to comment.
 
> I'm afraid to say that this means the patch needs to be re-done, sorry about that, but appreciate your understanding.

No problem!
Any suggestions are welcome.

[Dean Jackson]

Yuki, does this patch still apply or need review?

[Yuki Sekiguchi]

Hi  Dean,

I want to ask reviewers to review this patch. However, since text-combine-mode is removed from the current spec[1], I have no basis to change the default behavior.

[1]: http://www.w3.org/TR/css3-writing-modes/#changes-201205

[Koji Ishii]

Yuki, does the patch still apply?

I do not understand what you mean by "text-combine-mode was removed...", the property was removed as you said, but the default behavior is to compress within (almost) 1em.

http://dev.w3.org/csswg/css-writing-modes/#text-combine-compression

and in this point, the current WebKit behavior is not compliant to the spec.

If you could double-check if the patch still applies, re-base if necessary, and ask for review, that'd be great.

[Yuki Sekiguchi]

(In reply to comment #40)
> Yuki, does the patch still apply?
> 
> I do not understand what you mean by "text-combine-mode was removed...", the property was removed as you said, but the default behavior is to compress within (almost) 1em.
> 
> http://dev.w3.org/csswg/css-writing-modes/#text-combine-compression
> 
> and in this point, the current WebKit behavior is not compliant to the spec.

Thank you for good information. I didn't read the new spec. I'll change ChangeLog description.

> If you could double-check if the patch still applies, re-base if necessary, and ask for review, that'd be great.

Yes. I should do this. However, current WebKit doesn't show text combine, so I'll rebase this and request for review if the bug is fixed.

[Yuki Sekiguchi]

Hi Dean,

(In reply to comment #38)
> Yuki, does this patch still apply or need review?

Bug 127324 blocks this patch.
If you or your coworker are interested in this or that bug, could you review the patch in Bug 127324?
After the bug is fixed, I'll rebase this patch and request review again.

[Dean Jackson]

(In reply to comment #42)
> Hi Dean,
> 
> (In reply to comment #38)
> > Yuki, does this patch still apply or need review?
> 
> Bug 127324 blocks this patch.
> If you or your coworker are interested in this or that bug, could you review the patch in Bug 127324?
> After the bug is fixed, I'll rebase this patch and request review again.

Done!

[Yuki Sekiguchi]

(In reply to comment #43)
> Done!

Thank you for quick review!

[Jon Lee]

rdar://problem/11997940

[Koji Ishii]

*** This bug has been marked as a duplicate of bug 151051 ***