Add initial-letter support to the style system

RESOLVED FIXED in Firefox 50

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: jeremychen, Assigned: jeremychen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

10 months ago
These patches have been ready for review for a while in Bug 1223880. Since it might take a bit more time to complete the rendering parts, and style system are changing rapidly these days. I'd like to land this support first.
(Assignee)

Updated

10 months ago
Blocks: 1223880
(Assignee)

Comment 1

10 months ago
The implementation is based on the spec. https://drafts.csswg.org/css-inline/#sizing-drop-initials.
(Assignee)

Comment 2

10 months ago
Created attachment 8774224 [details]
Bug 1289007 - part1: parse and compute initial-letter property.

Review commit: https://reviewboard.mozilla.org/r/66764/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66764/
Attachment #8774224 - Flags: review?(cam)
Attachment #8774225 - Flags: review?(cam)
(Assignee)

Comment 3

10 months ago
Created attachment 8774225 [details]
Bug 1289007 - part0: remove redundant codes in ParsePropertyByFunction.

Review commit: https://reviewboard.mozilla.org/r/66766/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66766/
Comment on attachment 8774224 [details]
Bug 1289007 - part1: parse and compute initial-letter property.

https://reviewboard.mozilla.org/r/66764/#review63542

Looking mostly good, but I'd like to see another version with these issues addressed.  Also, probably better to fold the second part that adds the pref into this patch.

::: layout/style/nsCSSParser.cpp:9786
(Diff revision 1)
>    return true;
>  }
>  
> +// normal | [<number> <integer>?]
> +bool
> +CSSParserImpl::ParseInitialLetter(nsCSSProperty aPropID)

I think we don't need to pass the nsCSSProperty in, since we know what it is.  (I see there are some other parser functions that do this, like ParseAlignJustifySelf, but they don't need to either.)

::: layout/style/nsCSSParser.cpp:9789
(Diff revision 1)
> +// normal | [<number> <integer>?]
> +bool
> +CSSParserImpl::ParseInitialLetter(nsCSSProperty aPropID)
> +{
> +  nsCSSValue value;
> +  printf_stderr("\n\n[jeremy] here we are in ParseInitialLetter!\n\n");

Drop this.

::: layout/style/nsCSSParser.cpp:9794
(Diff revision 1)
> +  printf_stderr("\n\n[jeremy] here we are in ParseInitialLetter!\n\n");
> +  // 'inherit', 'initial', 'unset', 'none', and 'normal' must be alone
> +  if (!ParseSingleTokenVariant(value, VARIANT_INHERIT | VARIANT_NORMAL,
> +                               nullptr)) {
> +    nsCSSValue first, second;
> +    if (!ParseNonNegativeNumber(first)) {

The spec says this number must not be negative, but I wonder what the behaviour should be if we specify 0.  (It also sounds like if we omit the optional <integer>, then we can't specify 0 for the <number>, since the <integer> must not be 0.)

Might be worth reporting a spec issue?

::: layout/style/nsCSSParser.cpp:9798
(Diff revision 1)
> +    if (ExpectSymbol(';', true)) {
> +      AppendValue(aPropID, first);
> +      return true;
> +    }

This will make us fail if we have:

  initial-letter: 1 !important;

and also will consume the ";", which other code will be checking for.

So we generally return true if we encounter something unexpected while parsing a property value if what we have parsed so far is valid.  That will mean that we would still return true for:

  initial-letter: 1 xxx;

but then up in ParseProperty will reject the property if we don't find a ";" or "!important".

::: layout/style/nsCSSParser.cpp:9803
(Diff revision 1)
> +    if (ExpectSymbol(';', true)) {
> +      AppendValue(aPropID, first);
> +      return true;
> +    }
> +
> +    if (!ParseNonNegativeInteger(second)) {

This needs to reject 0.  You could call ParseSingleTokenOneOrLargerVariant, or make a helper like ParseNonNegativeInteger.

::: layout/style/nsCSSPropList.h:2276
(Diff revision 1)
> +CSS_PROP_TEXTRESET(
> +    initial-letter,
> +    initial_letter,
> +    InitialLetter,
> +    CSS_PROPERTY_PARSE_FUNCTION |
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |

It's not clear to me whether this should work when applied to ::first-line.

::: layout/style/nsCSSPropList.h:2277
(Diff revision 1)
> +    initial-letter,
> +    initial_letter,
> +    InitialLetter,
> +    CSS_PROPERTY_PARSE_FUNCTION |
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
> +        CSS_PROPERTY_APPLIES_TO_PLACEHOLDER,

I don't think this should apply to ::-moz-placeholder.  We can't use floats inside input::-moz-placeholder anyway.

::: layout/style/nsRuleNode.cpp:4978
(Diff revision 1)
>    SetValue(*aRuleData->ValueForUnicodeBidi(), text->mUnicodeBidi, conditions,
>             SETVAL_ENUMERATED | SETVAL_UNSET_INITIAL,
>             parentText->mUnicodeBidi,
>             NS_STYLE_UNICODE_BIDI_NORMAL);
>  
> +  // initial-letter: normal, number, pair(number|integer), initial

Here you mention "pair" but we're using eCSSUnit_Array for the nsCSSValue, rather than eCSSUnit_Pair.  Either is fine for the storage here, but if stay with Array, maybe write something else that doesn't mention "pair", like "array (number, integer?)" or something.

::: layout/style/nsRuleNode.cpp:4987
(Diff revision 1)
> +    const nsCSSValue firstValue = initialLetterValue->GetArrayValue()->Item(0);
> +    const nsCSSValue secondValue = initialLetterValue->GetArrayValue()->Item(1);

Make these references so that we don't copy the nsCSSValues.

::: layout/style/nsRuleNode.cpp:4998
(Diff revision 1)
> +    text->mInitialLetterSize = firstValue.GetFloatValue();
> +    text->mInitialLetterSink = secondValue.GetIntValue();
> +  } else if (initialLetterValue->GetUnit() == eCSSUnit_Number) {
> +    text->mInitialLetterIsNormal = false;
> +    text->mInitialLetterSize = initialLetterValue->GetFloatValue();
> +    text->mInitialLetterSink = floor(text->mInitialLetterSize);

The spec is misleading, because it says "floored to the nearest positive whole number", which doesn't sound like what "floor" means. :-)  And the example in that section of the spec shows that 2.51 rounds to 3.  So make this round, and please file a spec issue to make it clearer that it should round.

I also wonder what should happen with |initial-letter: 0.1|.  If we just round, we'll get 0, which is invalid if specified explicitly for the <integer>.  So maybe we should be rounding, but clamping to a minimum of 1?  Another spec issue I guess. :-)

::: layout/style/nsStyleStruct.h:1980
(Diff revision 1)
>  
>    nsStyleTextOverflow mTextOverflow;    // [reset] enum, string
>  
>    uint8_t mTextDecorationLine;          // [reset] see nsStyleConsts.h
>    uint8_t mUnicodeBidi;                 // [reset] see nsStyleConsts.h
> +  uint8_t mInitialLetterSink;           // [reset]

If we are storing the sink value as an uint8_t, please clamp the setting of this value to 255 in ComputeTextResetData.

::: layout/style/nsStyleStruct.h:1982
(Diff revision 1)
>  
>    uint8_t mTextDecorationLine;          // [reset] see nsStyleConsts.h
>    uint8_t mUnicodeBidi;                 // [reset] see nsStyleConsts.h
> +  uint8_t mInitialLetterSink;           // [reset]
> +
> +  bool mInitialLetterIsNormal : 1;      // [reset]

If it turns out 0 is an invalid value for the <number>, you could use that value in mInitialLetterSink to mean normal.

::: layout/style/nsStyleStruct.cpp:3623
(Diff revision 1)
> +    return nsChangeHint_RepaintFrame |
> +           nsChangeHint_SchedulePaint;

This of course dpeends on how you end up implementing the layout for initial-letter, but it's probably the case that this will need to reflow when these values change.  So let's move it to the mUnicodeBidi checking block at the top, and if later patches can return less expensive change hints we can change it then.

::: layout/style/test/property_database.js:3119
(Diff revision 1)
> +    other_values: [ "2", "2.5", "3.7 2", "4 3" ],
> +    invalid_values: [ "-3", "3.7 -2", "25%", "16px" ]

Please add a value like "1 0" to invalid_values.  And once we decide whether "0" and "0 1" are valid or invalid, let's add them too.
Attachment #8774224 - Flags: review?(cam) → review-
Comment on attachment 8774225 [details]
Bug 1289007 - part0: remove redundant codes in ParsePropertyByFunction.

https://reviewboard.mozilla.org/r/66766/#review63560

Please fold this into the main patch.  Also, add the pref to testing/profiles/prefs_general.js so that our tests run with this enabled.
Attachment #8774225 - Flags: review?(cam)
https://reviewboard.mozilla.org/r/66764/#review63564

::: layout/style/test/property_database.js:3114
(Diff revision 1)
>      type: CSS_TYPE_LONGHAND,
>      initial_values: [ "auto" ],
>      other_values: [ "normal", "disabled", "active", "inactive" ],
>      invalid_values: [ "none", "enabled", "1px" ]
>    },
> +  "initial-letter": {

By the way, you'll need to add this data for initial-letter inside an |if (IsCSSPropertyPrefEnabled("layout..."))|.
(Assignee)

Comment 7

10 months ago
https://reviewboard.mozilla.org/r/66764/#review63542

> I think we don't need to pass the nsCSSProperty in, since we know what it is.  (I see there are some other parser functions that do this, like ParseAlignJustifySelf, but they don't need to either.)

You're right. Will fix this in the next version.
I also take a look at other parser functions. It turns out that for those with a nsCSSProperty as input parameter, they were meant to be there. Most of them were due to sharing a same parsing function. Same use case for ParseAlignJustifySelf which is shared by eCSSProperty_align_self and eCSSProperty_justify_self. I'll add a patch to make switch-cases which share a same parsing function stick together.

> Drop this.

Will do.

> The spec says this number must not be negative, but I wonder what the behaviour should be if we specify 0.  (It also sounds like if we omit the optional <integer>, then we can't specify 0 for the <number>, since the <integer> must not be 0.)
> 
> Might be worth reporting a spec issue?

Good point. Thank you for the feedback. Filed https://github.com/w3c/csswg-drafts/issues/341

> This will make us fail if we have:
> 
>   initial-letter: 1 !important;
> 
> and also will consume the ";", which other code will be checking for.
> 
> So we generally return true if we encounter something unexpected while parsing a property value if what we have parsed so far is valid.  That will mean that we would still return true for:
> 
>   initial-letter: 1 xxx;
> 
> but then up in ParseProperty will reject the property if we don't find a ";" or "!important".

Lesson learned. Thank you for the explanation. :-)

> This needs to reject 0.  You could call ParseSingleTokenOneOrLargerVariant, or make a helper like ParseNonNegativeInteger.

Will do.

> It's not clear to me whether this should work when applied to ::first-line.

Yeah, we should exclude ::first-line.

> I don't think this should apply to ::-moz-placeholder.  We can't use floats inside input::-moz-placeholder anyway.

You're right. Will remove this in the next version.

> Here you mention "pair" but we're using eCSSUnit_Array for the nsCSSValue, rather than eCSSUnit_Pair.  Either is fine for the storage here, but if stay with Array, maybe write something else that doesn't mention "pair", like "array (number, integer?)" or something.

Will do.

> Make these references so that we don't copy the nsCSSValues.

Will do.

> The spec is misleading, because it says "floored to the nearest positive whole number", which doesn't sound like what "floor" means. :-)  And the example in that section of the spec shows that 2.51 rounds to 3.  So make this round, and please file a spec issue to make it clearer that it should round.
> 
> I also wonder what should happen with |initial-letter: 0.1|.  If we just round, we'll get 0, which is invalid if specified explicitly for the <integer>.  So maybe we should be rounding, but clamping to a minimum of 1?  Another spec issue I guess. :-)

Yeah, to be more specific, it should say "for values in [0,1), make it 1. Otherwise, floored to the nearest positive whole number" or something similar. I believe the example you mentioned is just for explanation of non-integer size. As to cases with only one non-integer argument, I guess editors/developers prefer sunken initial to drop initial. Not sure if they'd like to make it round.

Filed https://github.com/w3c/csswg-drafts/issues/341 (same issue as replied above) for further discussion. I think if the editors decide to make the first argument be "larger or eauql to 1", then most of the concerns you mentioned here would be gone. Will keep following the issue discussion.

> If we are storing the sink value as an uint8_t, please clamp the setting of this value to 255 in ComputeTextResetData.

Will do.

> If it turns out 0 is an invalid value for the <number>, you could use that value in mInitialLetterSink to mean normal.

Should I use an invalid value to initialize mInitialLetterSize as well?

> This of course dpeends on how you end up implementing the layout for initial-letter, but it's probably the case that this will need to reflow when these values change.  So let's move it to the mUnicodeBidi checking block at the top, and if later patches can return less expensive change hints we can change it then.

Will do.

> Please add a value like "1 0" to invalid_values.  And once we decide whether "0" and "0 1" are valid or invalid, let's add them too.

Will do.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #7)
> You're right. Will fix this in the next version.
> I also take a look at other parser functions. It turns out that for those
> with a nsCSSProperty as input parameter, they were meant to be there. Most
> of them were due to sharing a same parsing function. Same use case for
> ParseAlignJustifySelf which is shared by eCSSProperty_align_self and
> eCSSProperty_justify_self. I'll add a patch to make switch-cases which share
> a same parsing function stick together.

OK, thanks.  Yes I was wrong about ParseAlignJustifySelf (I should have realised from the function name), but grouping those calls into a single switch branch sounds good, thanks.

> > If it turns out 0 is an invalid value for the <number>, you could use that value in mInitialLetterSink to mean normal.
> 
> Should I use an invalid value to initialize mInitialLetterSize as well?

I suppose it doesn't matter too much, but having both mInitialLetterSink and mInitialLetterSize be 0 when the value is "normal" sounds OK, even if we only look at mInitialLetterSink to determine whether it is "normal".
(Assignee)

Comment 9

10 months ago
Looks like the discussion [1] leans to exclude initial-letter property's size argument from [0, 1), though the issue has not resolved yet. Note that the only vendor implementing this, WebKit, also excludes initial-letter's size argument from [0,1) (only accepts positive integer). Accordingly, I'm gonna be beforehand so the review process could be continued.

[1] https://github.com/w3c/csswg-drafts/issues/341
(Assignee)

Comment 10

10 months ago
Comment on attachment 8774225 [details]
Bug 1289007 - part0: remove redundant codes in ParsePropertyByFunction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66766/diff/1-2/
Attachment #8774225 - Attachment description: Bug 1289007 - part2: add preference guard. → Bug 1289007 - part0: remove redundant codes in ParsePropertyByFunction.
Attachment #8774225 - Flags: review?(cam)
Attachment #8774224 - Flags: review- → review?(cam)
(Assignee)

Comment 11

10 months ago
Comment on attachment 8774224 [details]
Bug 1289007 - part1: parse and compute initial-letter property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66764/diff/1-2/
Attachment #8774225 - Flags: review?(cam) → review+
Comment on attachment 8774225 [details]
Bug 1289007 - part0: remove redundant codes in ParsePropertyByFunction.

https://reviewboard.mozilla.org/r/66766/#review64404
Attachment #8774224 - Flags: review?(cam) → review+
Comment on attachment 8774224 [details]
Bug 1289007 - part1: parse and compute initial-letter property.

https://reviewboard.mozilla.org/r/66764/#review64406

Please set the pref to true in testing/profiles/prefs_general.js so that we run the tests that use the property_database.js entry for initial-letter.

r=me with that and the mInitialLetterIsNormal removal.

::: layout/style/nsStyleStruct.h:1982
(Diff revisions 1 - 2)
>  
>    uint8_t mTextDecorationLine;          // [reset] see nsStyleConsts.h
>    uint8_t mUnicodeBidi;                 // [reset] see nsStyleConsts.h
> -  uint8_t mInitialLetterSink;           // [reset]
> +  nscoord mInitialLetterSink;           // [reset]
>  
> -  bool mInitialLetterIsNormal : 1;      // [reset]
> +  bool mInitialLetterIsNormal : 1;      // [reset] whether initial-letter is normal

Since we "normal" is represented by mInitialLetterSink == 0, we can remove mInitialLetterIsNormal.
(In reply to Cameron McCormack (:heycam) from comment #13)
> Since we "normal" is represented by mInitialLetterSink == 0, we can remove
> mInitialLetterIsNormal.

But maybe add a comment next to mInitialLetterSink to mention that 0 means "normal" (like you did in nsRuleNode.cpp).
(Assignee)

Comment 15

10 months ago
Comment on attachment 8774225 [details]
Bug 1289007 - part0: remove redundant codes in ParsePropertyByFunction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66766/diff/2-3/
(Assignee)

Comment 16

10 months ago
Comment on attachment 8774224 [details]
Bug 1289007 - part1: parse and compute initial-letter property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66764/diff/2-3/
(Assignee)

Comment 17

10 months ago
https://reviewboard.mozilla.org/r/66764/#review64406

> Since we "normal" is represented by mInitialLetterSink == 0, we can remove mInitialLetterIsNormal.

Will do. Thank you for the reveiw. :-)
(Assignee)

Comment 18

10 months ago
https://reviewboard.mozilla.org/r/66764/#review64756

Take "inherit" into consideration while computing initial-letter property.
Add supported variant type to PropertySupportsVariant.
Make the first argument only accept "one or larger number".
(Assignee)

Comment 19

10 months ago
Comment on attachment 8774225 [details]
Bug 1289007 - part0: remove redundant codes in ParsePropertyByFunction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66766/diff/3-4/
(Assignee)

Comment 20

10 months ago
Comment on attachment 8774224 [details]
Bug 1289007 - part1: parse and compute initial-letter property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66764/diff/3-4/

Comment 21

10 months ago
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc66bb60d83d
part0: remove redundant codes in ParsePropertyByFunction. r=heycam
https://hg.mozilla.org/integration/autoland/rev/0aa0956567fc
part1: parse and compute initial-letter property. r=heycam

Comment 22

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc66bb60d83d
https://hg.mozilla.org/mozilla-central/rev/0aa0956567fc
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Comment 23

10 months ago
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #9)
> Looks like the discussion [1] leans to exclude initial-letter property's
> size argument from [0, 1), though the issue has not resolved yet. Note that
> the only vendor implementing this, WebKit, also excludes initial-letter's
> size argument from [0,1) (only accepts positive integer). Accordingly, I'm
> gonna be beforehand so the review process could be continued.
> 
> [1] https://github.com/w3c/csswg-drafts/issues/341

As a follow up note, this issue has been resolved by specifying that "Values less than one are invalid" for both arguments of initial-letter property. So, the implementation here agrees with the spec. now. :-)
You need to log in before you can comment on or make changes to this bug.