Last Comment Bug 774560 - Implement text-transform: full-width
: Implement text-transform: full-width
Status: RESOLVED FIXED
: css3, dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-16 20:11 PDT by Makoto Kato [:m_kato]
Modified: 2012-11-11 04:41 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (9.70 KB, patch)
2012-08-16 04:19 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
pt 1 - add Unicode full-width character mapping data. (24.05 KB, patch)
2012-11-04 08:53 PST, Jonathan Kew (:jfkthame)
smontagu: review+
Details | Diff | Splinter Review
pt 2 - implement text-transform:full-width. (5.94 KB, patch)
2012-11-04 08:54 PST, Jonathan Kew (:jfkthame)
smontagu: review+
Details | Diff | Splinter Review
pt 3 - testcase for all characters with a full-width mapping. (5.88 KB, patch)
2012-11-04 08:55 PST, Jonathan Kew (:jfkthame)
smontagu: review+
Details | Diff | Splinter Review

Description Makoto Kato [:m_kato] 2012-07-16 20:11:17 PDT
Since full-size-kana is dropped from CSS Text Level 3 spec, full-width value is last piece of text-transform support.
Comment 1 Makoto Kato [:m_kato] 2012-08-16 04:19:03 PDT
Created attachment 652389 [details] [diff] [review]
v1
Comment 2 Simon Montagu :smontagu 2012-08-19 21:08:12 PDT
Comment on attachment 652389 [details] [diff] [review]
v1

Review of attachment 652389 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like a test that covers all the affected characters. It should be no problem to generate such a test by script from http://unicode.org/Public/UNIDATA/UnicodeData.txt

I'd like to get Jonathan's opinion about this too: does this seem like the right approach or would you prefer to do it like the transforms in nsUnicodeProperties.cpp?

::: intl/unicharutil/util/nsUnicharUtils.cpp
@@ +460,5 @@
> +};
> +
> +PRUint32
> +ConvertToFullWidth(const PRUint32 aChar)
> +{

This seems to be missing U+FFE8 - U+FFEE.

Also please add a comment that we don't need to consider surrogate characters
Comment 3 Jonathan Kew (:jfkthame) 2012-09-02 04:29:17 PDT
(In reply to Simon Montagu from comment #2)
> I'd like to get Jonathan's opinion about this too: does this seem like the
> right approach or would you prefer to do it like the transforms in
> nsUnicodeProperties.cpp?

ISTM that we should be able to extend the genUnicodePropertyData.pl tool to extract the fullwidth mappings from UnicodeData.txt, and use a two-stage lookup table like those we're using for other character properties. That ought to provide more efficient lookup, particularly for the (many) CJK characters that are _not_ affected by the transform.

As currently written, if the user applies text-transform:full-width to a block of text that may contain some ASCII or kana, but is mostly Kanji, it looks like the ConvertToFullWidth() function will end up doing about a dozen "if"-tests for almost every character. The alternative approach would reduce that to a single "if" and a couple of array-indexing calculations. Less branching is probably better, in general.
Comment 4 Jonathan Kew (:jfkthame) 2012-11-04 08:53:01 PST
Created attachment 678129 [details] [diff] [review]
pt 1 - add Unicode full-width character mapping data.

This extends our Unicode property data to support fullwidth mapping info, based on the <wide> and <narrow> compatibility decompositions in the Unicode database.
Comment 5 Jonathan Kew (:jfkthame) 2012-11-04 08:54:23 PST
Created attachment 678130 [details] [diff] [review]
pt 2 - implement text-transform:full-width.

This is basically m_kato's patch, just rebased to current trunk and modified to use the fullwidth mapping from the preceding patch instead of a custom function.
Comment 6 Jonathan Kew (:jfkthame) 2012-11-04 08:55:13 PST
Created attachment 678131 [details] [diff] [review]
pt 3 - testcase for all characters with a full-width mapping.

Testcase covering all the characters for which text-transform:full-width has a mapping.
Comment 7 Jonathan Kew (:jfkthame) 2012-11-04 09:03:47 PST
(In reply to Jonathan Kew (:jfkthame) from comment #5)
> Created attachment 678130 [details] [diff] [review]
> pt 2 - implement text-transform:full-width.
> 
> This is basically m_kato's patch, just rebased to current trunk and modified
> to use the fullwidth mapping from the preceding patch instead of a custom
> function.

Oh, one more thing we should do in this patch is to add "full-width" to the other_values for text-transform in layout/style/test/property_database.js.
Comment 8 Jonathan Kew (:jfkthame) 2012-11-04 09:57:41 PST
Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=2c246ba76534
Comment 9 Simon Montagu :smontagu 2012-11-05 01:31:01 PST
Comment on attachment 678130 [details] [diff] [review]
pt 2 - implement text-transform:full-width.

Review of attachment 678130 [details] [diff] [review]:
-----------------------------------------------------------------

I take it we're OK with implementing this unprefixed?
Comment 10 Jonathan Kew (:jfkthame) 2012-11-05 01:43:59 PST
Hmm... I assumed so, but let's check with dbaron.
Comment 11 Jonathan Kew (:jfkthame) 2012-11-07 12:20:13 PST
Comment on attachment 678130 [details] [diff] [review]
pt 2 - implement text-transform:full-width.

fantasai, what's the current status of text-transform:full-width in CSS3 Text - in particular, should we be implementing it unprefixed at this point?
Comment 12 Jonathan Kew (:jfkthame) 2012-11-08 13:46:21 PST
from #layout, 2012-11-08:
[8:25pm] jfkthame: dbaron_: so did fantasai's reply re css3-text sound stable enough for us to land full-width without prefixing it?
[9:32pm] dbaron_ is now known as dbaron.
[9:32pm] dbaron: jfkthame, yes, I think so, assuming that we track the spec closely until it gets to CR
[9:33pm] jfkthame: dbaron: ok, sounds good to me
[9:34pm] jfkthame: dbaron: it'll take awhile to work its way up to the release channel anyway
[9:35pm] dbaron: jfkthame, yeah, that's basically what I was thinking
[9:35pm] jfkthame: right - so if something comes up in the next couple months, we could still hold back

After checking with fantasai & dbaron (email and irc), we'll go ahead with this. We'll keep an eye on the spec as it heads towards CR (editors are aiming for Last Call by the end of the year), just in case anything comes up that would cause us to re-evaluate before this hits the release channel.
Comment 15 Jean-Yves Perrier [:teoli] 2012-11-11 03:29:27 PST
I've documented it in:
https://developer.mozilla.org/en-US/docs/CSS/text-transform
and
https://developer.mozilla.org/en-US/docs/Firefox_19_for_developers

Two points I note:
1) we can't mixed full-width and uppercase in CSS Text L3 (and in the implementation) but we would be able to do in CSS Text L4.
2) Only latin characters without diacritics are affected (é à ö and other å stay narrow). (This is a Unicode constraint if I have read correctly)

Am I correct?
Comment 16 Jonathan Kew (:jfkthame) 2012-11-11 04:41:31 PST
(In reply to Jean-Yves Perrier [:teoli] from comment #15)
> Two points I note:
> 1) we can't mixed full-width and uppercase in CSS Text L3 (and in the
> implementation)

That's correct...

> but we would be able to do in CSS Text L4.

...and so is that, according to the current draft, though obviously that's subject to possible change.

> 2) Only latin characters without diacritics are affected (é à ö and other å
> stay narrow). (This is a Unicode constraint if I have read correctly)

Yes, only a very limited set of letters (and a few symbols, etc) have fullwidth variants. Note that it doesn't _only_ apply to Latin characters, however; there are also a bunch of "halfwidth" Japanese kana and Korean hangul letters in Unicode that map to fullwidth versions.

(You can see the full collection of affected characters in the test file layout/reftests/text-transform/fullwidth-all.html.)

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