Issue 87: Add arabic numbers to custom counter style #89
Conversation
| @@ -62,6 +62,7 @@ public class IdentValue implements FSDerivedValue { | |||
|
|
|||
| public final static IdentValue ABSOLUTE = addValue("absolute"); | |||
| public final static IdentValue ALWAYS = addValue("always"); | |||
| public final static IdentValue ARABIC = addValue("arabic-indic"); | |||
There was a problem hiding this comment.
for coherence, should be named, ARABIC_INDIC .
Additionally, I think you should add it to the LIST_STYLE_TYPE BitSet: https://github.com/openhtmltopdf/openhtmltopdf/blob/main/openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/parser/property/PrimitivePropertyBuilders.java#L73 .
And a visual test should be added for good measure (see VisualRegressionTest.java , you can have a look at the various PR that add a new test file + expected pdf)
There was a problem hiding this comment.
I have updated the code and added a visual text, but the pdf looks wrong (a # appears instead of the correct number). The code itself seems to work correctly as I'm obtaining the correct number when debugging on CounterFunction.createCounterText(getListStyleType(function), value); but not on the file rendered.
I attach the PDF with the incorrect content:
arabic-indic-counter-style.pdf
There was a problem hiding this comment.
Fixed, the arabic font was missing in the template and also the TestSupport.WITH_ARABIC in the visual test.
| @@ -371,6 +371,8 @@ void finish(AbstractStructualElement parent) { | |||
| listType = "UpperAlpha"; | |||
| } else if (listStyleType == IdentValue.LOWER_ALPHA) { | |||
| listType = "LowerAlpha"; | |||
| } else if (listStyleType == IdentValue.ARABIC_INDIC) { | |||
| listType = "ArabicIndic"; | |||
There was a problem hiding this comment.
To check if we should simply go to the fallback like the others, I'm not sure the PDF spec support this list type.
There was a problem hiding this comment.
So here you propose to remove this code block and just go to fallback for arabic?
There was a problem hiding this comment.
yes, but first we should check the pdf specs about what values are permitted.
There was a problem hiding this comment.
Understood, so would you check it on your side?
There was a problem hiding this comment.
From
https://developer.adobe.com/document-services/docs/assets/5b15559b96303194340b99820d3a70fa/PDF_ISO_32000-2.pdf -> Table 382:
The possible values are:
None No numbering system; Lbl elements (if present) contain arbitrary
text not subject to any numbering scheme
Unordered (PDF 2.0) Unordered list with unspecified bullets
Disc Solid circular bullet
Circle Open circular bullet
Square Solid square
Ordered (PDF 2.0) Ordered lists with unspecified numbering
Decimal Decimal Arabic numerals (1–9, 10–99, … )
UpperRoman Uppercase Roman numerals (I, II, III, IV, … )
LowerRoman Lowercase Roman numerals (i, ii, iii, iv, … )
UpperAlpha Uppercase letters (A, B, C, .. )
LowerAlpha Lowercase letters (a, b, c, … )
technically, Ordered would have been a better fallback. But for sure, "ArabicIndic" is not a valid value.
| @@ -1546,6 +1546,11 @@ public void testFormFieldOnSecondPage() throws IOException { | |||
| assertTrue(vt.runTest("form-control-on-second-page")); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testArabicIndicCounterStyle() throws IOException { | |||
| assertTrue(vt.runTest("arabic-indic-counter-style", TestSupport.WITH_ARABIC)); | |||
There was a problem hiding this comment.
I would add a test that check at least from 0 to 11 ?
There was a problem hiding this comment.
The test is already done, each div appears in a new page, from page 1 to 11
No description provided.