Size initial letters as CSS Inline Layout specification says

RESOLVED FIXED in Firefox 51

Status

()

Core
Layout: Block and Inline
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: jeremychen, Assigned: jeremychen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(URL)

MozReview Requests

()

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

Attachments

(1 attachment)

Inspired by Xidorn in bug 1223880 comment 30, I try to render initial-letter's size as spec. says in this bug. Since initial-letter property is default pref off, I think it would be fine to make size argument implementation work as a separate bug.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #0)
> Inspired by Xidorn in bug 1223880 comment 30, I try to render
> initial-letter's size as spec. says in this bug. Since initial-letter
> property is default pref off, I think it would be fine to make size argument
> implementation work as a separate bug.

Re-read this again, sounds pretty wierd. :-/

I probably should say that "I have a local patch already, and the implementation is based on what Xidorn suggested in bug 1223880 comment 30."
Status: NEW → ASSIGNED
Summary: Size initial letters as CSS Inline Layout Spec. says → Size initial letters as CSS Inline Layout specification says
Comment hidden (mozreview-request)

Comment 3

10 months ago
mozreview-review
Comment on attachment 8782813 [details]
Bug 1296561 - calculate initial-letter's size according to specification.

https://reviewboard.mozilla.org/r/72858/#review70596

Unless I'm missing something, the code here doesn't account for the fact that the initial letter might be styled with a different font from the surrounding text -- in which case its cap-height may be quite different. I think that to do this computation correctly, you'll need to retrieve both the cap-height of the surrounding text (to add to the (N-1)\*lineHeight target), *and* the (possibly different) cap-height of the initial letter (to figure out the ratio to use when scaling the font).

::: layout/style/nsStyleContext.cpp:727
(Diff revision 1)
> +      float capHeightRatio =
> +        NSCoordToFloat(capHeight) / mutableStyleFont->mFont.size;
> +      mutableStyleFont->mFont.size =
> +        NSToCoordRound(((textReset->mInitialLetterSize - 1) * usedLineHeight + capHeight) /
> +                       capHeightRatio);

As division is relatively expensive, I'd prefer to invert the computation here:

    float invCapHtRatio = mutableStyleFont->mFont.size / capHeight;
    mutableStyleFont->mFont.size = (...) * invCapHtRatio;

That way we only have one division operation. (I'm not sure if we can trust the compiler to figure this out for itself.)
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> Comment on attachment 8782813 [details]
> Bug 1296561 - calculate initial-letter's size according to specification.
> 
> https://reviewboard.mozilla.org/r/72858/#review70596
> 
> Unless I'm missing something, the code here doesn't account for the fact
> that the initial letter might be styled with a different font from the
> surrounding text -- in which case its cap-height may be quite different. I
> think that to do this computation correctly, you'll need to retrieve both
> the cap-height of the surrounding text (to add to the (N-1)\*lineHeight
> target), *and* the (possibly different) cap-height of the initial letter (to
> figure out the ratio to use when scaling the font).
> 
> ::: layout/style/nsStyleContext.cpp:727
> (Diff revision 1)
> > +      float capHeightRatio =
> > +        NSCoordToFloat(capHeight) / mutableStyleFont->mFont.size;
> > +      mutableStyleFont->mFont.size =
> > +        NSToCoordRound(((textReset->mInitialLetterSize - 1) * usedLineHeight + capHeight) /
> > +                       capHeightRatio);

Yes, you're right. We should refer to the surrounding text.
I'll fix this in the next version.

> As division is relatively expensive, I'd prefer to invert the computation
> here:
> 
>     float invCapHtRatio = mutableStyleFont->mFont.size / capHeight;
>     mutableStyleFont->mFont.size = (...) * invCapHtRatio;
> 
> That way we only have one division operation. (I'm not sure if we can trust
> the compiler to figure this out for itself.)

Good point. Will do.
(Assignee)

Comment 5

10 months ago
mozreview-review
Comment on attachment 8782813 [details]
Bug 1296561 - calculate initial-letter's size according to specification.

https://reviewboard.mozilla.org/r/72858/#review70624

Clear review request.
Will request again once the next version is ready to review.
Comment hidden (mozreview-request)

Comment 7

10 months ago
mozreview-review
Comment on attachment 8782813 [details]
Bug 1296561 - calculate initial-letter's size according to specification.

https://reviewboard.mozilla.org/r/72858/#review71040

::: layout/style/nsStyleContext.cpp:735
(Diff revision 2)
> +      float invCapHeightRatio =
> +        containerSC->StyleFont()->mFont.size / NSCoordToFloat(capHeight);

This still doesn't look quite right, I think. You're getting the capHeight from the containerSC's font, which should be correct for adding to ((N-1) * usedLineHeight) to get the desired height of the initial letter.

But for invCapHeightRatio, you need the ratio of font size to capHeight for the actual initial letter's style, which might be different from the ratio in the container's style.

In other words, I think this depends on getting capHeights from *both* the container's fontMetrics (as an actual length) *and* the initial letter's fontMetrics (used as a ratio).
(Assignee)

Comment 8

10 months ago
mozreview-review-reply
Comment on attachment 8782813 [details]
Bug 1296561 - calculate initial-letter's size according to specification.

https://reviewboard.mozilla.org/r/72858/#review71040

> This still doesn't look quite right, I think. You're getting the capHeight from the containerSC's font, which should be correct for adding to ((N-1) * usedLineHeight) to get the desired height of the initial letter.
> 
> But for invCapHeightRatio, you need the ratio of font size to capHeight for the actual initial letter's style, which might be different from the ratio in the container's style.
> 
> In other words, I think this depends on getting capHeights from *both* the container's fontMetrics (as an actual length) *and* the initial letter's fontMetrics (used as a ratio).

original_capHeight_ratio = original_capHeight / original_font_size

[initial-letter's new font size] * original_capHeight_ratio = [initial-letter's new capHeight] = ((N-1) * [containerSC's usedLineHeight]) + [containerSC's capHeight]

[initial-letter's new font size] = (((N-1) * [containerSC's usedLineHeight]) + [containerSC's capHeight]) / original_capHeight_ratio

So, I should use original_capHeight_ratio instead of container's capHeight ratio.

Thank you for pointing this out. Will fix this tomorrow morning cuz I left my computer in the office. :-/
Comment hidden (mozreview-request)

Comment 10

10 months ago
mozreview-review
Comment on attachment 8782813 [details]
Bug 1296561 - calculate initial-letter's size according to specification.

https://reviewboard.mozilla.org/r/72858/#review71418
Attachment #8782813 - Flags: review?(jfkthame) → review+

Comment 11

10 months ago
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79de44e4ad08
calculate initial-letter's size according to specification. r=jfkthame

Comment 12

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/79de44e4ad08
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.