Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(212)

Issue 766053004: text-justify fix for Chinese and Japanese (Closed)

Created:
6 years ago by kojii
Modified:
6 years ago
CC:
blink-reviews, Rik, danakj, krit, dw.im1, f(malita), jbroman, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

text-justify has been broken for Chinese and Japanese. This patch fixes this to the same behavior of WebKit. There are 3 code path to justify; LChar+SimpleShaper, UChar+SimplerShaper, and UChar+HarfbuzzShaper. This patch fixes UChar and SimpleShaper code path. Chinese and Japanese justification always occur at UChar (not LChar), and SimpleShaper is used most of the time for these scripts. I'll work on complex path later in another patch. Tests imported from WebKit were rebaselined to incorrect results at some point. I rebaselined all such tests by looking at them and by comparing the results with WebKit. Also one ref test was added. Note that Chinese/Japanese justification had been disabled under the switch |canExpandAroundIdeographsInComplexText| for Blink except on Mac, and then it was removed because it's off in https://codereview.chromium.org/618383003. What this patch means in that context is to fix SimpleShaper to support the switch and turn it back always, as long as text-justify: auto, which is the same as WebKit behavior. BUG=248894, 368108 TEST=fast/text/justify-ideograph.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187112

Patch Set 1 : WIP #

Patch Set 2 : TestExpectations updated #

Patch Set 3 : More test cases added, no changes in code #

Patch Set 4 : One more NeedsRebaseline for Linux and Win #

Unified diffs Side-by-side diffs Delta from patch set Stats (+82 lines, -2 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 1 chunk +18 lines, -0 lines 0 comments Download
A LayoutTests/fast/text/justify-ideograph.html View 1 2 1 chunk +24 lines, -0 lines 0 comments Download
A LayoutTests/fast/text/justify-ideograph-expected.html View 1 2 1 chunk +24 lines, -0 lines 0 comments Download
M LayoutTests/fast/text/justify-ideograph-leading-expansion.html View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/platform/fonts/Character.cpp View 2 chunks +14 lines, -0 lines 0 comments Download
M Source/platform/fonts/shaping/SimpleShaper.cpp View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 7 (3 generated)
kojii
leviw@, PTAL! The code removed in https://codereview.chromium.org/618383003 were disabled for a long time because it ...
6 years ago (2014-12-14 04:52:11 UTC) #3
leviw_travelin_and_unemployed
Thanks for the detailed explanation. LGTM.
6 years ago (2014-12-15 02:53:12 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/766053004/80001
6 years ago (2014-12-15 03:57:49 UTC) #6
commit-bot: I haz the power
6 years ago (2014-12-15 04:00:05 UTC) #7
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187112

Powered by Google App Engine
This is Rietveld 408576698