Use a more precise bounding box for initial letter texts

RESOLVED FIXED in Firefox 52

Status

()

Core
Layout: Text
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: jeremychen, Assigned: jeremychen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

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

Attachments

(4 attachments)

(Assignee)

Description

11 months ago
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
(Assignee)

Updated

11 months ago
Summary: Use TIGHT_HINTED_OUTLINE_EXTENTS for the bounding box of initial-letter texts → Use a more precise bounding box for initial letter texts
Comment hidden (mozreview-request)
(Assignee)

Comment 2

11 months ago
Created attachment 8797574 [details]
before_apply_patch

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

Comment 3

11 months ago
Created attachment 8797575 [details]
after_apply_patch

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

Comment 4

11 months ago
mozreview-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

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+
(Assignee)

Comment 5

11 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

11 months ago
mozreview-review
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

11 months ago
mozreview-review
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+

Comment 10

11 months ago
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

Comment 11

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c900a87c5e1f
https://hg.mozilla.org/mozilla-central/rev/232fa9688147
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.