1307402 - Use a more precise bounding box for initial letter texts

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jeremy Chen ([e-mail])]

At present, non-floating :first-letter texts are rendered with LOOSE_INK_EXTENTS [1], which is a bit larger than what we would expect for an initial letter. Also, setting height of an initial letter through the maxHeight of font metric [2] doesn't make sense since most of the use of initial letter are capital. I'd like to fix them for initial letter texts.


[1] http://searchfox.org/mozilla-central/rev/3411332d29d377bf86405fc3ea72461ea0cb0295/gfx/thebes/gfxFont.h#1401-1404
[2] http://searchfox.org/mozilla-central/rev/3411332d29d377bf86405fc3ea72461ea0cb0295/layout/generic/nsFirstLetterFrame.cpp#261

Comment 1

[Jeremy Chen ([e-mail])]

Attachment: Bug 1307402 - use a more precise bounding box for initial letter texts.

Review commit: https://reviewboard.mozilla.org/r/83246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83246/

Comment 2

[Jeremy Chen ([e-mail])]

Attachment: before_apply_patch

Before applying the patch, an obvious additional gap between first line and second line is presented.

Comment 3

[Jeremy Chen ([e-mail])]

Attachment: after_apply_patch

After applying the patch, the gap disappears and a finely raised initial letter is shown.

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8797572 [details]
Bug 1307402 - use a more precise bounding box for initial letter texts.

https://reviewboard.mozilla.org/r/83246/#review81718

Yes, makes sense. r=me, with some minor tweaks suggested.

::: layout/generic/nsFirstLetterFrame.cpp:264
(Diff revision 1)
> +      aMetrics.BSize(lineWM) =
> +        kidMetrics.Size(lineWM).ConvertTo(wm, lineWM).BSize(wm)
> +        + bp.BStartEnd(wm);

The writing-mode conversion here is redundant. It doesn't result in matching writing modes throughout the statement -- the dimension is being computed in `wm` but then assigned to `BSize(lineWM)`. The mismatch is OK because the modes cannot be orthogonal (see the assertion just above), but for that same reason, the `ConvertTo()` shouldn't be needed at all.

In fact, I think you can just use `kidMetrics.BSize(lineWM)` directly here.

::: layout/generic/nsTextFrame.cpp:8779
(Diff revision 1)
>  
> +bool
> +nsTextFrame::IsInitialLetterChild() const
> +{
> +  nsIFrame* frame = GetParent();
> +  return frame->StyleTextReset()->mInitialLetterSize != 0.0f &&

In the existing IsFloatingFirstLetterChild(), we include a check that the parent is non-null before trying to dereference it -- probably worth doing that here as well?

::: layout/generic/nsTextFrame.cpp:9061
(Diff revision 1)
>  
>    uint32_t transformedOffset = provider.GetStart().GetSkippedOffset();
>  
>    // The metrics for the text go in here
>    gfxTextRun::Metrics textMetrics;
> -  gfxFont::BoundingBoxType boundingBoxType = IsFloatingFirstLetterChild() ?
> +  gfxFont::BoundingBoxType boundingBoxType = IsFloatingFirstLetterChild() || IsInitialLetterChild()

This line looks very long, please wrap it. (I'd probably break it after the "=", I think, and then adjust the following lines.)

Comment 5

[Jeremy Chen ([e-mail])]

Comment on attachment 8797572 [details]
Bug 1307402 - use a more precise bounding box for initial letter texts.

https://reviewboard.mozilla.org/r/83246/#review81718

> The writing-mode conversion here is redundant. It doesn't result in matching writing modes throughout the statement -- the dimension is being computed in `wm` but then assigned to `BSize(lineWM)`. The mismatch is OK because the modes cannot be orthogonal (see the assertion just above), but for that same reason, the `ConvertTo()` shouldn't be needed at all.
> 
> In fact, I think you can just use `kidMetrics.BSize(lineWM)` directly here.

Thank you for the pointer. Will fix this before landing.

> In the existing IsFloatingFirstLetterChild(), we include a check that the parent is non-null before trying to dereference it -- probably worth doing that here as well?

Yes, will do.

Comment 6

[Jeremy Chen ([e-mail])]

Attachment: Bug 1307402 - reformat if-else-statements in nsFirstLetterFrame.cpp to agree with mozilla coding style.

Review commit: https://reviewboard.mozilla.org/r/83290/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83290/

Comment 7

[Jeremy Chen ([e-mail])]

Comment on attachment 8797572 [details]
Bug 1307402 - use a more precise bounding box for initial letter texts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83246/diff/1-2/

Comment 8

[Jeremy Chen ([e-mail])]

Comment on attachment 8797624 [details]
Bug 1307402 - reformat if-else-statements in nsFirstLetterFrame.cpp to agree with mozilla coding style.

https://reviewboard.mozilla.org/r/83244/#review81758

Sorry, looks like I forgot to upload this part at first time.

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8797624 [details]
Bug 1307402 - reformat if-else-statements in nsFirstLetterFrame.cpp to agree with mozilla coding style.

https://reviewboard.mozilla.org/r/83290/#review81766

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c900a87c5e1f
reformat if-else-statements in nsFirstLetterFrame.cpp to agree with mozilla coding style. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/232fa9688147
use a more precise bounding box for initial letter texts. r=jfkthame

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c900a87c5e1f
https://hg.mozilla.org/mozilla-central/rev/232fa9688147