Bug 93832

Summary: text-combine falls back to none rather than scale when wider than 1.1em
Product: WebKit Reporter: Koji Ishii <kojiishi>
Component: TextAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: dglazkov, dino, eric, jonlee, mmaxfield, ojan.autocc, webkit-bug-importer, webkit.review.bot, yuki.sekiguchi
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from gce-cr-linux-03
none
Patch
none
Archive of layout-test-results from gce-cr-linux-08
none
Patch
none
Patch
none
Rebase and Update test expectations
none
Archive of layout-test-results from gce-cr-linux-08
none
Change 1em calcuration to fit span's background
none
Reupload to make WebKit Review Bot work
none
Add failed test to platform/chromium/TestExpectations
none
Rebase
none
Retry
none
Update TestExpectations
none
Patch
none
Patch none

Description Koji Ishii 2012-08-13 05:47:46 PDT
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.
Comment 1 Radar WebKit Bug Importer 2012-08-13 09:49:32 PDT
<rdar://problem/12086597>
Comment 2 Yuki Sekiguchi 2012-08-14 19:45:41 PDT
Created attachment 158484 [details]
Patch
Comment 3 Yuki Sekiguchi 2012-08-14 19:53:09 PDT
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.
Comment 4 WebKit Review Bot 2012-08-14 20:23:53 PDT
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
Comment 5 WebKit Review Bot 2012-08-14 20:23:56 PDT
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
Comment 6 Koji Ishii 2012-08-14 22:49:37 PDT
* 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.
Comment 7 Yuki Sekiguchi 2012-08-15 02:00:34 PDT
Created attachment 158528 [details]
Patch
Comment 8 Yuki Sekiguchi 2012-08-15 02:03:35 PDT
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.
Comment 9 WebKit Review Bot 2012-08-15 03:45:27 PDT
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
Comment 10 WebKit Review Bot 2012-08-15 03:45:31 PDT
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
Comment 11 Yuki Sekiguchi 2012-08-16 22:43:06 PDT
Created attachment 159002 [details]
Patch
Comment 12 Yuki Sekiguchi 2012-08-17 00:39:23 PDT
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.
Comment 13 Koji Ishii 2012-08-17 03:55:33 PDT
It looks to me that combine-compress-expected.png has wrong expectations for emphasis, overline, and link are wrong. Can you check?
Comment 14 Yuki Sekiguchi 2012-08-19 19:49:23 PDT
>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.
Comment 15 Koji Ishii 2012-08-20 12:07:17 PDT
(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?
Comment 16 Koji Ishii 2012-08-20 12:13:12 PDT
> combine-compress-expected.html

combine-compress-expected.png is hard to see, especially "compress" line is hardly read. Better to make the font larger.
Comment 17 Yuki Sekiguchi 2012-08-21 18:02:25 PDT
Created attachment 159828 [details]
Patch
Comment 18 Yuki Sekiguchi 2012-08-21 18:14:29 PDT
> 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.
Comment 19 Yuki Sekiguchi 2012-08-22 23:18:57 PDT
Created attachment 160092 [details]
Rebase and Update test expectations
Comment 20 WebKit Review Bot 2012-08-23 00:02:21 PDT
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
Comment 21 WebKit Review Bot 2012-08-23 00:02:24 PDT
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
Comment 22 mitz 2012-08-30 13:07:07 PDT
<rdar://problem/11997940>
Comment 23 Yuki Sekiguchi 2012-09-02 18:59:47 PDT
Created attachment 161853 [details]
Change 1em calcuration to fit span's background
Comment 24 Yuki Sekiguchi 2012-09-03 19:58:42 PDT
Created attachment 161960 [details]
Reupload to make WebKit Review Bot work
Comment 25 WebKit Review Bot 2012-09-03 22:49:18 PDT
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
Comment 26 Yuki Sekiguchi 2012-09-05 22:58:23 PDT
Created attachment 162424 [details]
Add failed test to platform/chromium/TestExpectations
Comment 27 Yuki Sekiguchi 2012-09-05 23:02:00 PDT
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.
Comment 28 Yuki Sekiguchi 2012-12-10 21:51:41 PST
Created attachment 178711 [details]
Rebase
Comment 29 WebKit Review Bot 2012-12-10 21:55:18 PST
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.
Comment 30 Yuki Sekiguchi 2012-12-10 23:52:06 PST
Created attachment 178732 [details]
Retry
Comment 31 WebKit Review Bot 2012-12-11 01:59:47 PST
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
Comment 32 Yuki Sekiguchi 2012-12-11 20:24:11 PST
Created attachment 178953 [details]
Update TestExpectations
Comment 33 Koji Ishii 2012-12-27 06:09:03 PST
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.
Comment 34 Yuki Sekiguchi 2013-01-07 03:06:12 PST
Created attachment 181488 [details]
Patch
Comment 35 Build Bot 2013-01-07 03:40:38 PST
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
Comment 36 Yuki Sekiguchi 2013-01-07 17:12:09 PST
Created attachment 181608 [details]
Patch
Comment 37 Yuki Sekiguchi 2013-01-07 21:01:26 PST
(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.
Comment 38 Dean Jackson 2014-02-11 10:07:35 PST
Yuki, does this patch still apply or need review?
Comment 39 Yuki Sekiguchi 2014-02-12 22:35:02 PST
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
Comment 40 Koji Ishii 2014-02-12 23:30:14 PST
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.
Comment 41 Yuki Sekiguchi 2014-02-13 17:02:18 PST
(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.
Comment 42 Yuki Sekiguchi 2014-02-13 17:08:00 PST
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.
Comment 43 Dean Jackson 2014-02-13 17:20:13 PST
(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!
Comment 44 Yuki Sekiguchi 2014-02-13 17:21:40 PST
(In reply to comment #43)
> Done!

Thank you for quick review!
Comment 45 Jon Lee 2015-11-02 17:26:43 PST
rdar://problem/11997940
Comment 46 Koji Ishii 2015-11-20 00:00:35 PST

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