Bug 185375 – Add support for Intl NumberFormat formatToParts

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Robin Whittleton]

I’d love to see formatToParts supported - at the moment we’re having to add a polyfill for Safari (and Edge, although native support is now there in Firefox and Chrome). This lets us use Intl’s numberFormat to produce currency strings with all the global display inconsistencies, but also lets us support target markets in customisations they want to do (e.g. use a custom currency marker or replace the decimals with a dash when it’s a round number).

MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NumberFormat/formatToParts
ECMA 402 issue: https://github.com/tc39/ecma402/issues/30

[Andy VanWagoner]

Created attachment 340092 [details]
Patch

[EWS Watchlist]

Attachment 340092 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy VanWagoner]

Created attachment 340094 [details]
Patch

[EWS Watchlist]

Comment on attachment 340094 [details]
Patch

Attachment 340094 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7639651

New failing tests:
js/intl-numberformat.html

[EWS Watchlist]

Created attachment 340101 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 340094 [details]
Patch

Attachment 340094 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7639661

New failing tests:
js/intl-numberformat.html

[EWS Watchlist]

Created attachment 340102 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 340094 [details]
Patch

Attachment 340094 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7639669

New failing tests:
jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout
jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout-ftl-no-cjit
apiTests

[EWS Watchlist]

Comment on attachment 340094 [details]
Patch

Attachment 340094 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7639795

New failing tests:
js/intl-numberformat.html

[EWS Watchlist]

Created attachment 340108 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 340094 [details]
Patch

Attachment 340094 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7639721

New failing tests:
js/intl-numberformat.html

[EWS Watchlist]

Created attachment 340110 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Andy VanWagoner]

Created attachment 340145 [details]
Separate tests for formatToParts, disabled on macOS

[Andy VanWagoner]

Comment on attachment 340145 [details]
Separate tests for formatToParts, disabled on macOS

Since this feature relies on ICU v59, expect it to fail in macOS for now.

[EWS Watchlist]

Comment on attachment 340145 [details]
Separate tests for formatToParts, disabled on macOS

Attachment 340145 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7645109

New failing tests:
js/intl-numberformat-format-to-parts.html

[EWS Watchlist]

Created attachment 340155 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 340145 [details]
Separate tests for formatToParts, disabled on macOS

Attachment 340145 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7645521

New failing tests:
jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout
jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-ftl-no-cjit
apiTests

[EWS Watchlist]

Comment on attachment 340145 [details]
Separate tests for formatToParts, disabled on macOS

Attachment 340145 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7645496

New failing tests:
js/intl-numberformat-format-to-parts.html

[EWS Watchlist]

Created attachment 340158 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Andy VanWagoner]

Created attachment 340166 [details]
Patch

[EWS Watchlist]

Comment on attachment 340166 [details]
Patch

Attachment 340166 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7648248

New failing tests:
js/intl-numberformat-format-to-parts.html

[EWS Watchlist]

Created attachment 340176 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 340166 [details]
Patch

Attachment 340166 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7649039

New failing tests:
js/intl-numberformat-format-to-parts.html

[EWS Watchlist]

Created attachment 340183 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Andy VanWagoner]

Created attachment 340191 [details]
Set test expectations properly

[Yusuke Suzuki]

Comment on attachment 340191 [details]
Set test expectations properly

View in context: https://bugs.webkit.org/attachment.cgi?id=340191&action=review

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:628
> +        for (const auto &candidate : fields) {
> +            if (candidate.beginIndex <= currentIndex && currentIndex < candidate.endIndex && (!field.size() || candidate.size() < field.size()))
> +                field = candidate;
> +            if (currentIndex < candidate.beginIndex && candidate.beginIndex < nextStartIndex)
> +                nextStartIndex = candidate.beginIndex;
> +        }

I think if we sort this fields before iterating, we can iteratively accesses fields. In that case, time is O(NlogN), correct?

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:635
> +        parts->push(&exec, part);

Do not use `push()` since it is used for ArrayPush and it is observable to users (it uses [[Put]]).
Use putDirectIndex instead.

> Source/JavaScriptCore/runtime/IntlNumberFormat.h:34
> +#define JSC_ICU_HAS_FORMAT_DOUBLE_FOR_FIELDS (U_ICU_VERSION_MAJOR_NUM >= 59)

You can make this `#define HAVE_ICU_FORMAT_DOUBLE_FOR_FIELDS` and use it as `#if HAVE(ICU_FORMAT_DOUBLE_FOR_FIELDS)`.

> Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:86
> +        JSFunction* formatToPartsFunction = JSFunction::create(vm, globalObject, 1, vm.propertyNames->formatToParts.string(), IntlNumberFormatPrototypeFuncFormatToParts);
> +        putDirectWithoutTransition(vm, vm.propertyNames->formatToParts, formatToPartsFunction, static_cast<unsigned>(PropertyAttribute::DontEnum));

Use JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION instead.

> Source/cmake/WebKitFeatures.cmake:128
> +    WEBKIT_OPTION_DEFINE(ENABLE_INTL_NUMBER_FORMAT_TO_PARTS "Toggle Intl.NumberFormat.prototype.formatToParts support" PRIVATE OFF)

Add this option to OptionsJSCOnly.cmake and enable it for JSCOnly. (See ENABLE_INTL_PLURAL_RULES in OptionsJSCOnly.cmake).

[Yusuke Suzuki]

Comment on attachment 340191 [details]
Set test expectations properly

View in context: https://bugs.webkit.org/attachment.cgi?id=340191&action=review

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:631
> +        auto value = jsString(&exec, resultString.substring(currentIndex, nextIndex - currentIndex));

Use `jsSubstring(VM*, const String&, unsigned offset, unsigned length)`.

[Andy VanWagoner]

(In reply to Yusuke Suzuki from comment #26)
> Comment on attachment 340191 [details]
> Set test expectations properly
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340191&action=review
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:628
> > +        for (const auto &candidate : fields) {
> > +            if (candidate.beginIndex <= currentIndex && currentIndex < candidate.endIndex && (!field.size() || candidate.size() < field.size()))
> > +                field = candidate;
> > +            if (currentIndex < candidate.beginIndex && candidate.beginIndex < nextStartIndex)
> > +                nextStartIndex = candidate.beginIndex;
> > +        }
> 
> I think if we sort this fields before iterating, we can iteratively accesses
> fields. In that case, time is O(NlogN), correct?

The fields are nested, so order doesn't trivialize this loop anyway. There is one integer field spanning all of the group separators and digits up to the decimal separator.

I could do a sort first which is O(NlogN) itself, but given the bounds of N (Number.MAX_NUMBER has ~100), I'm not sure it's worth the additional code complexity. The time here is dominated by unum_formatDoubleForFields.

> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:635
> > +        parts->push(&exec, part);
> 
> Do not use `push()` since it is used for ArrayPush and it is observable to
> users (it uses [[Put]]).
> Use putDirectIndex instead.

Will do. `parts->putDirectIndex(&exec, parts->length(), part);` ?

> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.h:34
> > +#define JSC_ICU_HAS_FORMAT_DOUBLE_FOR_FIELDS (U_ICU_VERSION_MAJOR_NUM >= 59)
> 
> You can make this `#define HAVE_ICU_FORMAT_DOUBLE_FOR_FIELDS` and use it as
> `#if HAVE(ICU_FORMAT_DOUBLE_FOR_FIELDS)`.

That looks much nicer. Will do.

> 
> > Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:86
> > +        JSFunction* formatToPartsFunction = JSFunction::create(vm, globalObject, 1, vm.propertyNames->formatToParts.string(), IntlNumberFormatPrototypeFuncFormatToParts);
> > +        putDirectWithoutTransition(vm, vm.propertyNames->formatToParts, formatToPartsFunction, static_cast<unsigned>(PropertyAttribute::DontEnum));
> 
> Use JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION instead.

Will do.

> 
> > Source/cmake/WebKitFeatures.cmake:128
> > +    WEBKIT_OPTION_DEFINE(ENABLE_INTL_NUMBER_FORMAT_TO_PARTS "Toggle Intl.NumberFormat.prototype.formatToParts support" PRIVATE OFF)
> 
> Add this option to OptionsJSCOnly.cmake and enable it for JSCOnly. (See
> ENABLE_INTL_PLURAL_RULES in OptionsJSCOnly.cmake).

Will do.

[Andy VanWagoner]

Created attachment 340194 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 340191 [details]
Set test expectations properly

View in context: https://bugs.webkit.org/attachment.cgi?id=340191&action=review

>>> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:628
>>> +        }
>> 
>> I think if we sort this fields before iterating, we can iteratively accesses fields. In that case, time is O(NlogN), correct?
> 
> The fields are nested, so order doesn't trivialize this loop anyway. There is one integer field spanning all of the group separators and digits up to the decimal separator.
> 
> I could do a sort first which is O(NlogN) itself, but given the bounds of N (Number.MAX_NUMBER has ~100), I'm not sure it's worth the additional code complexity. The time here is dominated by unum_formatDoubleForFields.

Even if they are nested, you can sort an expected order by using beginIndex, and endIndex.

[    A   ]
[ B ]
     [ C ]

In this case, we can sort [A, B, C] if we use sort fields with beginIndex and then compare endIndex if beginIndex is the same.
Could you add FIXME, bug URL, and note about O(N^2) complexity?

>>> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:635
>>> +        parts->push(&exec, part);
>> 
>> Do not use `push()` since it is used for ArrayPush and it is observable to users (it uses [[Put]]).
>> Use putDirectIndex instead.
> 
> Will do. `parts->putDirectIndex(&exec, parts->length(), part);` ?

Holding an index, and call `parts->putDirectIndex(&exec, index++, part);`.

[Andy VanWagoner]

(In reply to Yusuke Suzuki from comment #30)
> Comment on attachment 340191 [details]
> Set test expectations properly
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340191&action=review
> 
> >>> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:628
> >>> +        }
> >> 
> >> I think if we sort this fields before iterating, we can iteratively accesses fields. In that case, time is O(NlogN), correct?
> > 
> > The fields are nested, so order doesn't trivialize this loop anyway. There is one integer field spanning all of the group separators and digits up to the decimal separator.
> > 
> > I could do a sort first which is O(NlogN) itself, but given the bounds of N (Number.MAX_NUMBER has ~100), I'm not sure it's worth the additional code complexity. The time here is dominated by unum_formatDoubleForFields.
> 
> Even if they are nested, you can sort an expected order by using beginIndex,
> and endIndex.
> 
> [    A   ]
> [ B ]
>      [ C ]
> 
> In this case, we can sort [A, B, C] if we use sort fields with beginIndex
> and then compare endIndex if beginIndex is the same.
> Could you add FIXME, bug URL, and note about O(N^2) complexity?

I spent some time trying to rework it to an O(N log N) algorithm, first sorting the fields, and then iterating through them, but I was having a rough time with it, so I'll add the FIXME and revisit another time: https://bugs.webkit.org/show_bug.cgi?id=185557

> 
> >>> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:635
> >>> +        parts->push(&exec, part);
> >> 
> >> Do not use `push()` since it is used for ArrayPush and it is observable to users (it uses [[Put]]).
> >> Use putDirectIndex instead.
> > 
> > Will do. `parts->putDirectIndex(&exec, parts->length(), part);` ?
> 
> Holding an index, and call `parts->putDirectIndex(&exec, index++, part);`.

k, I'll fix that.

[Andy VanWagoner]

Created attachment 340216 [details]
Patch

[Andy VanWagoner]

Created attachment 340328 [details]
rebased

[Andy VanWagoner]

Comment on attachment 340328 [details]
rebased

Fixed conflict and removed accidental change to license header.

[EWS Watchlist]

Comment on attachment 340328 [details]
rebased

Attachment 340328 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7679256

New failing tests:
css3/filters/backdrop/add-remove-add-backdrop-filter.html

[EWS Watchlist]

Created attachment 340337 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Andy VanWagoner]

Created attachment 340418 [details]
Patch

[Andy VanWagoner]

Are there any other changes I should make, or is does this look ready to commit?

[Yusuke Suzuki]

Comment on attachment 340418 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 340418 [details]
Patch

Rejecting attachment 340418 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 340418, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
LayoutTests/platform/ios-simulator/TestExpectations
patching file LayoutTests/platform/mac/TestExpectations
Hunk #1 FAILED at 1762.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac/TestExpectations.rej
patching file LayoutTests/platform/win/TestExpectations
patching file ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Yusuke Suzuki']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/7700846

[Yusuke Suzuki]

(In reply to WebKit Commit Bot from comment #40)
> Comment on attachment 340418 [details]
> Patch
> 
> Rejecting attachment 340418 [details] from commit-queue.
> 
> Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch',
> '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02',
> 'apply-attachment', '--no-update', '--non-interactive', 340418,
> '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit
> 
> Last 500 characters of output:
> LayoutTests/platform/ios-simulator/TestExpectations
> patching file LayoutTests/platform/mac/TestExpectations
> Hunk #1 FAILED at 1762.
> 1 out of 1 hunk FAILED -- saving rejects to file
> LayoutTests/platform/mac/TestExpectations.rej
> patching file LayoutTests/platform/win/TestExpectations
> patching file ChangeLog
> Hunk #1 succeeded at 1 with fuzz 3.
> 
> Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply',
> '--force', '--reviewer', u'Yusuke Suzuki']" exit_code: 1 cwd:
> /Volumes/Data/EWS/WebKit
> 
> Full output: http://webkit-queues.webkit.org/results/7700846

Could you rebase this?

[Andy VanWagoner]

Created attachment 340516 [details]
rebased

[WebKit Commit Bot]

Comment on attachment 340516 [details]
rebased

Clearing flags on attachment: 340516

Committed r231867: <https://trac.webkit.org/changeset/231867>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40306553>

[Robin Whittleton]

Thanks Andy for all the hard work, that was much quicker than I was expecting :)

[Andy VanWagoner]

(In reply to Robin Whittleton from comment #46)
> Thanks Andy for all the hard work, that was much quicker than I was
> expecting :)

Happy to help. It's still not enabled on macOS, so it could still take a long while before it ends up in a Safari release.

[Emanuele Feliziani]

This bug has been marked as RESOLVED FIXED for months, now, and the fix is already available in Safari Technology Preview. I was really hoping to see it included in Safari with macOS 10.14.4 and iOS 12.2, but it wasn't. Is there any ETA for this? What is holding this back?