Closed Bug 1307402 Opened 4 years ago Closed 4 years ago

Use a more precise bounding box for initial letter texts

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: chenpighead, Assigned: chenpighead)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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
Summary: Use TIGHT_HINTED_OUTLINE_EXTENTS for the bounding box of initial-letter texts → Use a more precise bounding box for initial letter texts
Attached image before_apply_patch
Before applying the patch, an obvious additional gap between first line and second line is presented.
Attached image after_apply_patch
After applying the patch, the gap disappears and a finely raised initial letter is shown.
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.)
Attachment #8797572 - Flags: review?(jfkthame) → review+
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 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 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
Attachment #8797624 - Flags: review?(jfkthame) → review+
Pushed by jichen@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/c900a87c5e1f
https://hg.mozilla.org/mozilla-central/rev/232fa9688147
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.