[Webkit-unassigned] [Bug 33089] Implement alphabetic CSS3 list-style-types
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 7 10:52:28 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=33089
--- Comment #11 from Daniel Bates <dbates at webkit.org> 2010-01-07 10:52:26 PST ---
(In reply to comment #10)
> (From update of attachment 46019 [details])
> > // disc | circle | square | decimal | decimal-leading-zero | lower-roman |
> > // upper-roman | lower-greek | lower-alpha | lower-latin | upper-alpha |
> > - // upper-latin | hebrew | armenian | georgian | cjk-ideographic | hiragana |
> > - // katakana | hiragana-iroha | katakana-iroha | none | inherit
> > + // upper-latin | afar | ethiopic-halehame-aa-et | ethiopic-halehame-aa-er |
>> [...]
> > + // upper-greek | upper-norwegian | hebrew | armenian | georgian |
> > + // cjk-ideographic | hiragana | katakana | hiragana-iroha | katakana-iroha |
> > + // none | inherit
>
> So lame that you have to do this. This comments can be useful, but it's always
> a pain to have one more place you have to edit! My apologies.
Maybe we should remove this comment? or better yet, either refer to section
CSS_PROP_LIST_STYLE_TYPE in file CSSValueKeywords.in or to the enum
EListStyleType in RenderStyleConstants.h.
I have another patch (coming shortly - depends on this one) to implement all
the numeric CSS3 list-style-types. So, this comment will get even longer.
> > +afar
> > +ethiopic-halehame-aa-et
> > +ethiopic-halehame-aa-er
> > +amharic
> > [...]
> > +ethiopic-abegede-ti-et
> > +upper-greek
> > +upper-norwegian
>
> Is there an order here? Could we sort these alphabetically so the order isn't
> seemingly-random?
I'll re-order the whole section CSS_PROP_LIST_STYLE_TYPE in file
CSSValueKeywords.in alphabetically.
For your reference, they are ordered according to how they appear in the spec.
<http://www.w3.org/TR/css3-lists/#alphabetic>, which is grouped by alias and
sorted alphabetically by the group representative. For instance:
For example, the first two groups in the spec are:
[afar], ethiopic-halehame-aa-et, ethiopic-halehame-aa-er
And
[amharic], ethiopic-halehame-am-et
where [X] is the representative of the group.
>
> > - "upper-alpha", "upper-latin", "hebrew", "armenian", "georgian", "cjk-ideographic", "hiragana", "katakana",
> > - "hiragana-iroha", "katakana-iroha", "inline", "block", "list-item", "run-in", "compact", "inline-block",
> > [...]
> > + "inline-block", "table", "inline-table", "table-row-group", "table-header-group", "table-footer-group", "table-row",
>
> Same question about sort order.
Will re-order alphabetically.
>
> > + return 0x1366;
>
> In most cases, we use names for characters rather than just putting the number
> in. That's typically done by adding named constants to CharacterNames.h that
> exactly match the names from the Unicode specification. The exception would be
> in a table where there are so many characters it is not helpful. This seems
> like a case where the name would be better.
>
> > + static const UChar ethiopicHalehameAaErAlphabet[18] = {
> > + 0x1200, 0x1208, 0x1210, 0x1218, 0x1228, 0x1230, 0x1260, 0x1270, 0x1290,
> > + 0x12A0, 0x12A8, 0x12C8, 0x12D0, 0x12E8, 0x12F0, 0x1308, 0x1338, 0x1348
> > + };
>
> How confident are we that these tables are correct? I was concerned when I
> considered doing this, because the CSS draft had a lot of cautions about not
> knowing if the tables were correct.
I am confident that I created the tables with the same Unicode code points as
in the spec., with the exception for upper-greek (which I added the missing
characters and notified Ian Hickson) (*).
The only ones in the spec. (at this time) that are missing characters are for
hiragana-iroha, hiragana, katakana-iroha, katakana, and upper-greek (not
listed, see (*)).
For your reference, the tables for hiragana-iroha, hiragana, katakana-iroha,
katakana are currently in in the tree.
>
> > + return toAlphabetic(value, ethiopicHalehameAaErAlphabet, 18);
>
> It's unfortunate to repeat the constants like "18" in the call sites like this.
> I'd rather see us use a sizeof approach so the constant isn't repeated twice. I
> believe someone had a patch to add a "number of array elements" macro or
> function template to WTF. And I think there's a way to do this with a function
> template so we don't even have to type the sizeof each time, although I'm not
> sure. If all else fails I suggest using a macro.
I'll look into this.
> > - switch (style()->listStyleType()) {
> > + const EListStyleType type = style()->listStyleType();
>
> For brevity, WebKit code doesn't use const for the values local variables, even
> though we could for many; almost all. Please don't do it here.
Will change.
>
> > + const UChar suffix = listMarkerSuffix(type);
>
Will change.
>
> > - const UChar periodSpace[2] = { '.', ' ' };
> > - context->drawText(style()->font(), TextRun(periodSpace, 2), marker.location() + IntSize(width, 0));
> > + UChar suffixSpace[2];
> > + suffixSpace[0] = suffix;
> > + suffixSpace[1] = ' ';
>
> You could use array initialization syntax here as the old code did. It's good
> that you removed the const, by the way.
Will change.
>
> > - const UChar spacePeriod[2] = { ' ', '.' };
> > - TextRun spacePeriodRun(spacePeriod, 2);
> > - int width = font.width(spacePeriodRun);
> > - context->drawText(style()->font(), spacePeriodRun, marker.location());
> > + UChar spaceSuffix[2];
> > + spaceSuffix[0] = ' ';
> > + spaceSuffix[1] = suffix;
> > + TextRun spaceSuffixRun(spaceSuffix, 2);
> > + int width = font.width(spaceSuffixRun);
> > + context->drawText(style()->font(), spaceSuffixRun, marker.location());
>
Will change.
>
> > - const UChar periodSpace[2] = { '.', ' ' };
> > - int periodSpaceWidth = font.width(TextRun(periodSpace, 2));
> > - width = itemWidth + periodSpaceWidth;
> > + UChar suffixSpace[2];
> > + suffixSpace[0] = listMarkerSuffix(type);
> > + suffixSpace[1] = ' ';
>
Will change.
>
> > + const EListStyleType type = style()->listStyleType();
>
Will remove const.
>
> > - const UChar periodSpace[2] = { '.', ' ' };
> > - int periodSpaceWidth = font.width(TextRun(periodSpace, 2));
> > - return IntRect(x(), y() + font.ascent(), itemWidth + periodSpaceWidth, font.height());
> > + UChar suffixSpace[2];
> > + suffixSpace[0] = listMarkerSuffix(type);
> > + suffixSpace[1] = ' ';
> > + int suffixSpaceWidth = font.width(TextRun(suffixSpace, 2));
> > + return IntRect(x(), y() + font.ascent(), itemWidth + suffixSpaceWidth, font.height());
>
Will change.
>
> > unsigned _box_direction : 1; // EBoxDirection (CSS3 box_direction property, flexible box layout module)
> > - // 32 bits
> > + // 33 bits
>
> There is another comment a few lines later than also needs to be updated. The
> one that says "39 bits". And then one further down, the one that says "48
> bits".
Will update comments.
> > + Afar, EthiopicHalehameAaEt, EthiopicHalehameAaEr, Amharic,
> > + EthiopicHalehameAmEt, AmharicAbegede, EthiopicAbegedeAmEt,
> > + CjkEarthlyBranch, CjkHeavenlyStem, Ethiopic, EthiopicHalehameGez,
> > + EthiopicAbegede, EthiopicAbegedeGez, HangulConsonant, Hangul,
> > + LowerNorwegian, Oromo, EthiopicHalehameOmEt, Sidama,
> > + EthiopicHalehameSidEt, Somali, EthiopicHalehameSoEt, Tigre,
> > + EthiopicHalehameTig, TigrinyaEr, EthiopicHalehameTiEr,
> > + TigrinyaErAbegede, EthiopicAbegedeTiEr, TigrinyaEt,
> > + EthiopicHalehameTiEt, TigrinyaEtAbegede, EthiopicAbegedeTiEt,
> > + UpperGreek, UpperNorwegian,
> > Hebrew, Armenian, Georgian, CJKIdeographic,
> > Hiragana, Katakana, HiraganaIroha, KatakanaIroha, NoneListStyle
>
> At a certain point these lists are far better if sorted, and one item per line.
Will sort alphabetically and put one item per line.
>
> > + /*
> > + The following styles are ordered as they appear in section 4.4. of the
> > + Draft 7 November 2002 draft of the CSS3 Lists module <http://www.w3.org/TR/css3-lists/#alphabetic>.
> > + */
>
> Ah, the answer to my question about ordering. I'm not sure what's best.
>
> By the way, we almost always prefer "//" comments everywhere, even in CSS and
> test cases.
>From my understanding, we can't use "//"-style comments in CSS as per section
4.1.9 of the CSS 2.1 spec. <http://www.w3.org/TR/CSS2/syndata.html#comments>.
So, we have to use C-style comments (i.e. /* ... */).
> A test case is much more valuable if it can be a dumpAsText test rather than a
> render-tree-based test, since those usually need different results
> per-platform. That having been said, I can't think of a way to do this at the
> moment. We should try to think of a way, though, because I think it makes
> things easier to maintain and test later. It's also much better to have a test
> where we can see if we're passing or failing even if we don't have fonts that
> cover all the characters. Worth thinking on this too.
I included a render-tree dump, which includes the text for the list marker
character. So, this can be tested using DRT.
Ideally, we should add a dumpListMarkersAsText() command to DRT so that we can
just use dumpAsText. This is best addressed in a separate bug.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list