Bug 93832

Summary:

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

Product:

WebKit

Reporter:

Koji Ishii <kojii>

Component:

Text

Assignee:

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:

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

Koji Ishii

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

Attachments
Patch (168.18 KB, patch) 2012-08-14 19:45 PDT, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-03 (378.00 KB, application/zip) 2012-08-14 20:23 PDT, WebKit Review Bot	no flags	Details
Patch (230.46 KB, patch) 2012-08-15 02:00 PDT, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-08 (653.37 KB, application/zip) 2012-08-15 03:45 PDT, WebKit Review Bot	no flags	Details
Patch (230.26 KB, patch) 2012-08-16 22:43 PDT, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Patch (231.27 KB, patch) 2012-08-21 18:02 PDT, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Rebase and Update test expectations (123.85 KB, patch) 2012-08-22 23:18 PDT, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-08 (508.40 KB, application/zip) 2012-08-23 00:02 PDT, WebKit Review Bot	no flags	Details
Change 1em calcuration to fit span's background (124.05 KB, patch) 2012-09-02 18:59 PDT, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Reupload to make WebKit Review Bot work (124.05 KB, patch) 2012-09-03 19:58 PDT, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Add failed test to platform/chromium/TestExpectations (125.18 KB, patch) 2012-09-05 22:58 PDT, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Rebase (91.05 KB, patch) 2012-12-10 21:51 PST, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Retry (91.05 KB, patch) 2012-12-10 23:52 PST, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Update TestExpectations (91.24 KB, patch) 2012-12-11 20:24 PST, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Patch (168.27 KB, patch) 2013-01-07 03:06 PST, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Patch (169.59 KB, patch) 2013-01-07 17:12 PST, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2012-08-13 09:49:32 PDT

<rdar://problem/12086597>

Yuki Sekiguchi

Comment 2 2012-08-14 19:45:41 PDT

Created attachment 158484 [details] Patch

Yuki Sekiguchi

Comment 3 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.

WebKit Review Bot

Comment 4 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

WebKit Review Bot

Comment 5 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

Koji Ishii

Comment 6 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.

Yuki Sekiguchi

Comment 7 2012-08-15 02:00:34 PDT

Created attachment 158528 [details] Patch

Yuki Sekiguchi

Comment 8 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.

WebKit Review Bot

Comment 9 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

WebKit Review Bot

Comment 10 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

Yuki Sekiguchi

Comment 11 2012-08-16 22:43:06 PDT

Created attachment 159002 [details] Patch

Yuki Sekiguchi

Comment 12 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.

Koji Ishii

Comment 13 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?

Yuki Sekiguchi

Comment 14 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.

Koji Ishii

Comment 15 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?

Koji Ishii

Comment 16 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.

Yuki Sekiguchi

Comment 17 2012-08-21 18:02:25 PDT

Created attachment 159828 [details] Patch

Yuki Sekiguchi

Comment 18 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.

Yuki Sekiguchi

Comment 19 2012-08-22 23:18:57 PDT

Created attachment 160092 [details] Rebase and Update test expectations

WebKit Review Bot

Comment 20 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

WebKit Review Bot

Comment 21 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

mitz

Comment 22 2012-08-30 13:07:07 PDT

<rdar://problem/11997940>

Yuki Sekiguchi

Comment 23 2012-09-02 18:59:47 PDT

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

Yuki Sekiguchi

Comment 24 2012-09-03 19:58:42 PDT

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

WebKit Review Bot

Comment 25 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

Yuki Sekiguchi

Comment 26 2012-09-05 22:58:23 PDT

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

Yuki Sekiguchi

Comment 27 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.

Yuki Sekiguchi

Comment 28 2012-12-10 21:51:41 PST

Created attachment 178711 [details] Rebase

WebKit Review Bot

Comment 29 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.

Yuki Sekiguchi

Comment 30 2012-12-10 23:52:06 PST

Created attachment 178732 [details] Retry

WebKit Review Bot

Comment 31 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

Yuki Sekiguchi

Comment 32 2012-12-11 20:24:11 PST

Created attachment 178953 [details] Update TestExpectations

Koji Ishii

Comment 33 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.

Yuki Sekiguchi

Comment 34 2013-01-07 03:06:12 PST

Created attachment 181488 [details] Patch

Build Bot

Comment 35 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

Yuki Sekiguchi

Comment 36 2013-01-07 17:12:09 PST

Created attachment 181608 [details] Patch

Yuki Sekiguchi

Comment 37 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.

Dean Jackson

Comment 38 2014-02-11 10:07:35 PST

Yuki, does this patch still apply or need review?

Yuki Sekiguchi

Comment 39 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

Koji Ishii

Comment 40 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.

Yuki Sekiguchi

Comment 41 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.

Yuki Sekiguchi

Comment 42 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.

Dean Jackson

Comment 43 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!

Yuki Sekiguchi

Comment 44 2014-02-13 17:21:40 PST

(In reply to comment #43) > Done! Thank you for quick review!

Jon Lee

Comment 45 2015-11-02 17:26:43 PST

rdar://problem/11997940

Koji Ishii

Comment 46 2015-11-20 00:00:35 PST

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

Note You need to log in before you can comment on or make changes to this bug.