Closed Bug 1498148 Opened 6 years ago Closed 6 years ago

Implement text-transform: full-size-kana

Categories

(Core :: Layout: Text and Fonts, enhancement, P4)

Product:
Core
Component:
Layout: Text and Fonts
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla64
Tracking Flags:
Tracking Status
firefox64 --- fixed

People

(Reporter: xidorn, Assigned: dpino)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, good-first-bug)

Attachments

(1 file, 7 obsolete files)

Bug-1498148-Implement-text-transform-full-size-kana..patch
37.23 KB, patch
xidorn
: review+
Details | Diff | Splinter Review 
The working group resolved on adding this back in w3c/csswg-drafts#3143.

Spec: https://drafts.csswg.org/css-text-3/#valdef-text-transform-full-size-kana

This should be something fairly easy to implement. The layout side involves adding an extra branch in nsCaseTransformTextRunFactory::TransformString after NS_STYLE_TEXT_TRANSFORM_FULL_WIDTH (the class should probably have a different name, but that doesn't matter in this bug), and the style system side needs to add a new value to text-transform in https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/servo/components/style/properties/longhands/inherited_text.mako.rs#26

This can probably be a good first bug.
Blocks: css-text-3
Here's a first patch.

I still need to implement a few more kanas conversions. So far, I implemented conversion of small hiraganas to big hiraganas. I used a `std:map`. I'm not sure if that's the right approach though.

Next I plan to support the rest of the kanas specified at https://specs.rivoal.net/css-custom-tt/#full-size-kana
Attachment #9016606 - Flags: feedback?(xidorn+moz)
Comment on attachment 9016606 [details] [diff] [review]
Bug-1498148-Implement-text-transform-full-size-kana.patch

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

Looks pretty good.

I wouldn't recommend you to use std::map, though. It's probably better to just use a static const array and search in it (either linear or binary). There are very few items, so a map may not be very performant, while it is more complicated and can take more memory.

Also you should probably look at https://drafts.csswg.org/css-text-3/#small-kana rather than Florian's informal spec, although they seem to be identical :)
Attachment #9016606 - Flags: feedback?(xidorn+moz) → feedback+
Assignee: nobody → dpino
I added support for the remaining kana. I also replaced the `std:map` for constant static arrays as suggested and used a `BinarySearch`.

In the end, I implemented the list at https://specs.rivoal.net/css-custom-tt/#full-size-kana, since it's more complete than https://drafts.csswg.org/css-text-3/#small-kana. It seems to me that the mapping in the draft's Appendix G (Small Kana Mappings) is stated as an example, but it doesn't cover all the mappings. For instance, that list is missing the 'ゃ' (HIRAGANA SMALL YA) used as an example at https://github.com/w3c/csswg-drafts/issues/3143.

OTOH, the list at https://specs.rivoal.net/css-custom-tt/#full-size-kana was missing two mappings: 'ㇺ' => 'ム' and 'ｯ' => 'ﾂ'. I added them to the arrays.
Attachment #9016606 - Attachment is obsolete: true
Attachment #9016710 - Flags: review?(xidorn+moz)
If you're doing *anything* that disagrees with https://drafts.csswg.org/css-text-3/#small-kana or isn't clearly specified in it, you should raise issues at https://github.com/w3c/csswg-drafts/issues so that the spec can describe that behavior, so that other implementations will do the same.
Thanks for the pointer David. I'll post my questions at https://github.com/w3c/csswg-drafts/issues
Comment on attachment 9016710 [details] [diff] [review]
Bug-1498148-Implement-text-transform-full-size-kana.patch

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

Thanks for working on this! Just a couple of drive-by comments:

::: layout/generic/nsTextRunTransformations.cpp
@@ +607,5 @@
>        ch = mozilla::unicode::GetFullWidth(ch);
>        break;
>  
> +    case NS_STYLE_TEXT_TRANSFORM_FULL_SIZE_KANA: {
> +      static const uint32_t sortedSmallKanas[] = {

You should be able to use uint16_t for these arrays to make them more compact, as only plane-0 codepoints are involved.

@@ +640,5 @@
> +        // ｱ    ｲ       ｳ       ｴ       ｵ       ﾔ       ﾕ       ﾖ        ﾂ
> +        0xFF71, 0xFF72, 0xFF73, 0xFF74, 0xFF75, 0xFF94, 0xFF95, 0xFF96, 0xFF82
> +      };
> +
> +      unsigned long index;

This should be type 'size_t', to match the declaration of BinarySearch.
For reference, I posted the questions raised in this ticket regarding the spec at https://github.com/w3c/csswg-drafts/issues/3143#issuecomment-429452645
It would be better to open a new issue in csswg-drafts; comments in a closed issue are likely to get lost.
Agree, I moved the comment to a new issue https://github.com/w3c/csswg-drafts/issues/3209
Updated patch. Addressed comments by :jfkthame.
Attachment #9016710 - Attachment is obsolete: true
Attachment #9016710 - Flags: review?(xidorn+moz)
Attachment #9016813 - Flags: review?(dbaron)
There was one thing I forgot to mention: test.

Please add a web-platform reftest, probably in testing/web-platform/tests/css/css-text/text-transform, covering all characters being mapped. Probably another test which checks kanas which are not touched.

Also, please add the new value to property_database.js: https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/layout/style/test/property_database.js#4588
Comment on attachment 9016813 [details] [diff] [review]
Bug-1498148-Implement-text-transform-full-size-kana.patch

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

::: layout/generic/nsTextRunTransformations.cpp
@@ +607,5 @@
>        ch = mozilla::unicode::GetFullWidth(ch);
>        break;
>  
> +    case NS_STYLE_TEXT_TRANSFORM_FULL_SIZE_KANA: {
> +      static const uint16_t sortedSmallKanas[] = {

Please prefix const with k, like kSortedSmallKanas.

Probably just kSmallKanas, and rename the variable below as kFullSizeKanas, with a comment above it saying this array is sorted.
Comment on attachment 9016813 [details] [diff] [review]
Bug-1498148-Implement-text-transform-full-size-kana.patch

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

::: layout/generic/nsTextRunTransformations.cpp
@@ +646,5 @@
> +      if (mozilla::BinarySearch(sortedSmallKanas, 0, len, ch, &index)) {
> +        ch = kanas[index];
> +      }
> +      }
> +      break;

The '}' corresponding to '{' starts with case tag should generally go to the same indent level, so it should be:
> case XXX: {
>   // do something
>   break;
> }
rather than leaving the break outside and have two '}' at the same level above it.
Attachment #9016813 - Flags: review?(dbaron)
Other than the two issues mentioned above and the test, it looks good to me.
I addressed the comments in the latest review. I also added 2 WPT tests. The point is that they don't pass and I don't understand why. Visually the tests are Ok, however the tests report as FAIL. This is the output I got when running `text-transform-full-size-kana-001.html`:

$ ./mach wpt testing/web-platform/tests/css/css-text/text-transform/text-transform-full-size-kana-001.html
 0:14.16 INFO Application command: /home/dpino/workspace/gecko/obj-x86_64-pc-linux-gnu/dist/bin/firefox --marionette about:blank -profile /tmp/tmpKKLhox.mozrunner
 0:14.17 INFO Starting runner
 0:14.80 pid:11006 Full command: /home/dpino/workspace/gecko/obj-x86_64-pc-linux-gnu/dist/bin/firefox --marionette about:blank -profile /tmp/tmpKKLhox.mozrunner
pid:11006 (firefox:11006): Gtk-WARNING **: 08:21:44.294: Theme parsing error: <data>:1:34: Expected ')' in color definition
 0:14.80 pid:11006 (firefox:11006): Gtk-WARNING **: 08:21:44.294: Theme parsing error: <data>:1:77: Expected ')' in color definition
 0:16.26 pid:11006 1539411705750        Marionette      INFO    Listening on port 2828
 0:16.53 TEST_START: /css/css-text/text-transform/text-transform-full-size-kana-001.html
 0:16.89 pid:11006 1539411706375        Marionette      INFO    Testing http://web-platform.test:8000/css/css-text/text-transform/text-transform-full-size-kana-001.html == http://web-platform.test:8000/css/css-text/text-transform/reference/text-transform-full-size-kana-001.html
 0:17.00 pid:11006 JavaScript error: resource://devtools-client-jsonview/lib/require.js, line 166: Error: Script error for: viewer-config
 0:17.00 pid:11006 http://requirejs.org/docs/errors.html#scripterror
 0:17.05 pid:11006 1539411706534        Marionette      INFO    Found 30298 pixels different, maximum difference per channel 255
 0:17.07 TEST_END: FAIL, expected PASS - Testing http://web-platform.test:8000/css/css-text/text-transform/text-transform-full-size-kana-001.html == http://web-platform.test:8000/css/css-text/text-transform/reference/text-transform-full-size-kana-001.html

It seems the test fails because:

0:17.05 pid:11006 1539411706534        Marionette      INFO    Found 30298 pixels different, maximum difference per channel 255

Any hint or advice?
Attachment #9016813 - Attachment is obsolete: true
Attachment #9016917 - Flags: review?(xidorn+moz)
Comment on attachment 9016917 [details] [diff] [review]
Bug-1498148-Implement-text-transform-full-size-kana.patch

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

It seems you don't have your reference files included in your patch.

::: testing/web-platform/tests/css/css-text/text-transform/text-transform-full-size-kana-001.html
@@ +7,5 @@
> +<link rel='author' title='Diego Pino Garcia' href='mailto:dpino@igalia.com'>
> +<link rel='help' href='https://drafts.csswg.org/css-text-3/#text-transform'>
> +<link rel="match" href="reference/text-transform-full-size-kana-001.html">
> +<style type='text/css'>
> +@font-face {

Please remove this and the `font-family` declaration below. This test doesn't seem to need any special font to support. If we have a test font for kana, that would be great, but otherwise let's just don't bother it.

@@ +20,5 @@
> +</style>
> +</head>
> +<body>
> +<p class="instructions">Test passes if both characters in each pair match. If you are missing a font glyph for a character, ignore that pair, but report which characters were ignored.</p>
> +<div class="test"><span title="U+3041">&#x3041; &#x3042;</span> <span title="U+3043">&#x3043; &#x3044;</span> <span title="U+3045">&#x3045; &#x3046;</span> <span title="U+3047">&#x3047; &#x3048;</span> <span title="U+3049">&#x3049; &#x304A;</span> <span title="U+3095">&#x3095; &#x304B;</span> <span title="U+3096">&#x3096; &#x3051;</span> <span title="U+3063">&#x3063; &#x3064;</span> <span title="U+3083">&#x3083; &#x3084;</span> <span title="U+3085">&#x3085; &#x3086;</span> <span title="U+3087">&#x3087; &#x3088;</span> <span title="U+308E">&#x308E; &#x308F;</span> <span title="U+30A1">&#x30A1; &#x30A2;</span> <span title="U+30A3">&#x30A3; &#x30A4;</span> <span title="U+30A5">&#x30A5; &#x30A6;</span> <span title="U+30A7">&#x30A7; &#x30A8;</span> <span title="U+30A9">&#x30A9; &#x30AA;</span> <span title="U+30F5">&#x30F5; &#x30AB;</span> <span title="U+31F0">&#x31F0; &#x30AF;</span> <span title="U+30F6">&#x30F6; &#x30B1;</span> <span title="U+31F1">&#x31F1; &#x30B7;</span> <span title="U+31F2">&#x31F2; &#x30B9;</span> <span title="U+30C3">&#x30C3; &#x30C4;</span> <span title="U+31F3">&#x31F3; &#x30C8;</span> <span title="U+31F4">&#x31F4; &#x30CC;</span> <span title="U+31F5">&#x31F5; &#x30CF;</span> <span title="U+31F6">&#x31F6; &#x30D2;</span> <span title="U+31F7">&#x31F7; &#x30D5;</span> <span title="U+31F8">&#x31F8; &#x30D8;</span> <span title="U+31F9">&#x31F9; &#x30DB;</span> <span title="U+31FA">&#x31FA; &#x30E0;</span> <span title="U+30E3">&#x30E3; &#x30E4;</span> <span title="U+30E5">&#x30E5; &#x30E6;</span> <span title="U+30E7">&#x30E7; &#x30E8;</span> <span title="U+31FB">&#x31FB; &#x30E9;</span> <span title="U+31FC">&#x31FC; &#x30EA;</span> <span title="U+31FD">&#x31FD; &#x30EB;</span> <span title="U+31FE">&#x31FE; &#x30EC;</span> <span title="U+31FF">&#x31FF; &#x30ED;</span> <span title="U+30EE">&#x30EE; &#x30EF;</span> <span title="U+FF67">&#xFF67; &#xFF71;</span> <span title="U+FF68">&#xFF68; &#xFF72;</span> <span title="U+FF69">&#xFF69; &#xFF73;</span> <span title="U+FF6A">&#xFF6A; &#xFF74;</span> <span title="U+FF6B">&#xFF6B; &#xFF75;</span> <span title="U+FF6F">&#xFF6F; &#xFF82;</span> <span title="U+FF6C">&#xFF6C; &#xFF94;</span> <span title="U+FF6D">&#xFF6D; &#xFF95;</span> <span title="U+FF6E">&#xFF6E; &#xFF96;</span></div>

Consider having each pair in its own line in the source code, that would make it much easier to edit.

::: testing/web-platform/tests/css/css-text/text-transform/text-transform-full-size-kana-002.html
@@ +7,5 @@
> +<link rel='author' title='Diego Pino Garcia' href='mailto:dpino@igalia.com'>
> +<link rel='help' href='https://drafts.csswg.org/css-text-3/#text-transform'>
> +<link rel="match" href="reference/text-transform-full-size-kana-002.html">
> +<style type='text/css'>
> +@font-face {

Same here.
Attachment #9016917 - Flags: review?(xidorn+moz)
Right, I didn't copy the same test files to the reference directory. Thanks for pointing that out (I didn't fully understand how the test worked, now it makes sense).

I've updated the patch addressing the comments in the last review. I edited the commit message and added "r=xidorn". I think the patch is ready to go now, although perhaps there's some last bit to fix.

If you're ok with the final patch I will push it to the treeherder. Please let me know.
Attachment #9016917 - Attachment is obsolete: true
Attachment #9016926 - Flags: review?(xidorn+moz)
Comment on attachment 9016926 [details] [diff] [review]
Bug-1498148-Implement-text-transform-full-size-kana..patch

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

Please fix the issues below. Otherwise it looks good now.

::: testing/web-platform/tests/css/css-text/text-transform/reference/text-transform-full-size-kana-001.html
@@ +1,2 @@
> +<!DOCTYPE html>
> +<html  lang="en" >

Maybe
> <html lang="en">
(remove redundant whitespaces)

@@ +6,5 @@
> +<meta name="assert" content="For small kanas, text-transform: full-size-kana puts all kanas in full-size kanas.">
> +<link rel='author' title='Diego Pino Garcia' href='mailto:dpino@igalia.com'>
> +<link rel='help' href='https://drafts.csswg.org/css-text-3/#text-transform'>
> +<style type='text/css'>
> +.test, .ref { font-size: 200%; line-height: 2.5em; font-family: webfont, sans-serif; }

Please remove "font-family: webfont, sans-serif;" in this line, since it doesn't look useful.

@@ +14,5 @@
> +</style>
> +</head>
> +<body>
> +<p class="instructions">Test passes if both characters in each pair match. If you are missing a font glyph for a character, ignore that pair, but report which characters were ignored.</p>
> +<div class="test">

The reference file is exactly identical to the test file (except the <link rel="match"> tag), which doesn't sound right.

I suppose you want to use "ref" here instead.

::: testing/web-platform/tests/css/css-text/text-transform/reference/text-transform-full-size-kana-002.html
@@ +14,5 @@
> +</style>
> +</head>
> +<body>
> +<p class="instructions">Test passes if both characters in each pair match. If you are missing a font glyph for a character, ignore that pair, but report which characters were ignored.</p>
> +<div class="test">

All stuff above applies to this file as well.

::: testing/web-platform/tests/css/css-text/text-transform/text-transform-full-size-kana-001.html
@@ +7,5 @@
> +<link rel='author' title='Diego Pino Garcia' href='mailto:dpino@igalia.com'>
> +<link rel='help' href='https://drafts.csswg.org/css-text-3/#text-transform'>
> +<link rel="match" href="reference/text-transform-full-size-kana-001.html">
> +<style type='text/css'>
> +.test, .ref { font-size: 200%; line-height: 2.5em; font-family: webfont, sans-serif; }

And some here as well.

::: testing/web-platform/tests/css/css-text/text-transform/text-transform-full-size-kana-002.html
@@ +7,5 @@
> +<link rel='author' title='Diego Pino Garcia' href='mailto:dpino@igalia.com'>
> +<link rel='help' href='https://drafts.csswg.org/css-text-3/#text-transform'>
> +<link rel="match" href="reference/text-transform-full-size-kana-002.html">
> +<style type='text/css'>
> +.test, .ref { font-size: 200%; line-height: 2.5em; font-family: webfont, sans-serif; }

And here.
Attachment #9016926 - Flags: review?(xidorn+moz)
Comment on attachment 9016926 [details] [diff] [review]
Bug-1498148-Implement-text-transform-full-size-kana..patch

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

::: testing/web-platform/tests/css/css-text/text-transform/reference/text-transform-full-size-kana-001.html
@@ +15,5 @@
> +</head>
> +<body>
> +<p class="instructions">Test passes if both characters in each pair match. If you are missing a font glyph for a character, ignore that pair, but report which characters were ignored.</p>
> +<div class="test">
> +  <span title="U+3041">&#x3041; &#x3042;</span>

Actually, this reference file is completely identical to the test file, which means it isn't testing anything at all. You should make the reference file using the same characters for each pair, so that it ensures that we do apply the transformation.

::: testing/web-platform/tests/css/css-text/text-transform/text-transform-full-size-kana-001.html
@@ +5,5 @@
> +<title>CSS3 Text, text transform: full-size-kana, small kanas</title>
> +<meta name="assert" content="For small kanas, text-transform: full-size-kana puts all kanas in full-size kanas.">
> +<link rel='author' title='Diego Pino Garcia' href='mailto:dpino@igalia.com'>
> +<link rel='help' href='https://drafts.csswg.org/css-text-3/#text-transform'>
> +<link rel="match" href="reference/text-transform-full-size-kana-001.html">

Also I think in general we suffix the reference file with "-ref" in file name. It's not clear whether it is still necessary to do so, but it seems other files in testing/web-platform/tests/css/css-text/text-transform/reference also have such suffix, so it's probably better to follow that convention.
In general, you should check that the feature check should fail when the code change hasn't been applied, to confirm that the test is really testing the feature.
I addressed the comments of the last review. I renamed the reference tests and removed the full-size-kana transformation, so they actually can be used as reference when the transformation is applied. I also removed the extra spaces and unneeded fonts, etc.
Attachment #9016926 - Attachment is obsolete: true
Attachment #9016936 - Flags: review?(xidorn+moz)
Comment on attachment 9016936 [details] [diff] [review]
Bug-1498148-Implement-text-transform-full-size-kana..patch

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

Looks good, thanks!

It occured to me that you also need to run "./mach devtools-css-db" to update properties-db.js, otherwise there will be a test complaining about it.

Maybe you should send an "intent to implement and ship" about this feature to dev-platform (or I can do that for you). See https://wiki.mozilla.org/ExposureGuidelines#Email_templates for more details.
Attachment #9016936 - Flags: review?(xidorn+moz) → review+
Run "./mac devtools-css-db" and added `devtools/shared/css/generated/properties-db.js`. Thanks for the tip :)

About the notification to dev-platform mailing list, it certainly sounds interesting but I'd appreciate if you could that for me since it'd be my first time (I'm actually not subscribed to this mailing list yet) and I think I lack background about the motivations for this feature. I'll pay attention to your email and use it as reference for future occasions (if there's any).
Attachment #9016936 - Attachment is obsolete: true
Attachment #9016943 - Flags: review?(xidorn+moz)
Attachment #9016943 - Flags: review?(xidorn+moz) → review+
Thanks for the multiple reviews and sent the "intend to implement and ship" email. Treeherder looks fine. I will mark the bug as checkin-needed.
Keywords: checkin-needed
Keywords: dev-doc-needed
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a00b402fb8e8
Implement text-transform: full-size-kana. r=xidorn
Keywords: checkin-needed
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13504 for changes under testing/web-platform/tests
oops, it seems you forgot to change the filename of reference file in 002... I don't know if pushing a follow-up helps.
Upstream PR was closed without merging
Right, thanks for the pointer (it wasn't easy to spot).
Attachment #9016943 - Attachment is obsolete: true
Attachment #9016977 - Flags: review?(xidorn+moz)
Rereading the treeherder (https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ce187436ee651b0d5ff04417af976c01d8da341) is obvious the test was failing. My bad.
Attachment #9016977 - Flags: review?(xidorn+moz) → review+
Flags: needinfo?(xidorn+moz)
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Awesome, thanks for implementing this.

I left a comment about the tests in github as well, but maybe it will be more easily discovered here.

The "tip" in a comment in the test is not correct. copy/paste should copy the original untransformed character, so Uniview won't help.

Spec reference for that claim:
https://drafts.csswg.org/css-text-3/#text-transform-property

  This property transforms text for styling purposes.
  It has no effect on the underlying content,
  and must not affect the content of a plain text copy & paste operation.

----

Also, I have a similar test already waiting in another PR, which might be a little bit easier to verify visually. Review appreciated: https://github.com/web-platform-tests/wpt/pull/13461
(In reply to Florian Rivoal from comment #37)
> The "tip" in a comment in the test is not correct. copy/paste should copy
> the original untransformed character, so Uniview won't help.

I see. Feel free to remove the comment once it's merged. I suppose this also applies to many other tests in the text-transform directory.

> Also, I have a similar test already waiting in another PR, which might be a
> little bit easier to verify visually. Review appreciated:
> https://github.com/web-platform-tests/wpt/pull/13461

Oh, hmmm, maybe we shouldn't add two sets of tests... I would rather we don't confuse the sync bot, and the two sets of tests don't look much different (other than that there is a negative test being added here in addition), so maybe you can close that PR?

(It would have been great if that was merged before we get to implement this here but... people are quicker than I thought to pick good-first-bug :)
> Feel free to remove the comment once it's merged.

It's easier to remove now. After merging it will need another separate review.

> I suppose this also applies to many other tests in the text-transform directory.

Haven't checked. Do many have this kind of copy/paste suggestion? If so, they are indeed all wrong.

> The two sets of tests don't look much different (other than that there is a negative test being added here in addition), so maybe you can close that PR?

I like mine better, they are easier for a human to review :) Showing both small and large and asking the reviewer to make sure we match the large seems less likely to lead to mistakes than just asking if two characters match. Someone who is not familiar with Japanese (and maybe even someone who is) might fail to notice the size difference, and just agree that they look the same and call it a pass even when not justified.

My favorite outcome is to keep my tests, remove your -001, keep (and renumber) your -002. But if you disagree, that's not a hill I'm interested in dying on.
(In reply to Florian Rivoal from comment #39)
> > Feel free to remove the comment once it's merged.
> 
> It's easier to remove now. After merging it will need another separate
> review.
> 
> > I suppose this also applies to many other tests in the text-transform directory.
> 
> Haven't checked. Do many have this kind of copy/paste suggestion? If so,
> they are indeed all wrong.

Yes, see https://searchfox.org/mozilla-central/search?q=Tip%3A%20To%20identify%20the%20characters%20where%20differences%20occur

They can be removed in a single PR after this lands.

> > The two sets of tests don't look much different (other than that there is a negative test being added here in addition), so maybe you can close that PR?
> 
> I like mine better, they are easier for a human to review :) Showing both
> small and large and asking the reviewer to make sure we match the large
> seems less likely to lead to mistakes than just asking if two characters
> match. Someone who is not familiar with Japanese (and maybe even someone who
> is) might fail to notice the size difference, and just agree that they look
> the same and call it a pass even when not justified.
> 
> My favorite outcome is to keep my tests, remove your -001, keep (and
> renumber) your -002. But if you disagree, that's not a hill I'm interested
> in dying on.

The problem is that the PR there is managed by the WPT sync bot, to stop it we need to backout the patch, change it and reland, which is more cumbersome than changing your PR... So whatever we are going to do, let's wait for the upstreaming PR to get merged first, what do you think?
>> Do many have this kind of copy/paste suggestion?
> Yes
Indeed, that's plenty.

> They can be removed in a single PR after this lands.

Let's do that.

> The problem is that the PR there is managed by the WPT sync bot, to stop it we need to backout the patch, change it and reland, which is more cumbersome than changing your PR...

Let's land this patch first, and then I'll rebase my PR against it, chaging it from a PR that adds the test to a PR that makes the test easier to read by humans reviewers.
I agree with conclusions (land the patch and then merge Florian's tests). I like Florian's tests better. OTOH, I think it'd be handy to keep the negative test (to verify that not new unauthorized kanas are added to the list) but it'd be necessary to make it in the same style as Florian's tests (small kana, text-transformed kana, full-size kana).
https://hg.mozilla.org/mozilla-central/rev/410f1ce46d58
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Note to the docs team:

I've added a note to the Fx 64 rel notes to cover this:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#CSS

when documenting this, you'll need to add the value to the text-transform ref page, add an example maybe, and update the compat data.

Updated https://developer.mozilla.org/en-US/docs/Web/CSS/text-transform - updated interactive example, syntax, BCD, and live example.

Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1541668
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: