Closed
Bug 1307402
Opened 8 years ago
Closed 8 years ago
Use a more precise bounding box for initial letter texts
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
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
Assignee | ||
Updated•8 years 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•8 years ago
|
||
Before applying the patch, an obvious additional gap between first line and second line is presented.
Assignee | ||
Comment 3•8 years ago
|
||
After applying the patch, the gap disappears and a finely raised initial letter is shown.
Comment 4•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c900a87c5e1f https://hg.mozilla.org/mozilla-central/rev/232fa9688147
Status: NEW → RESOLVED
Closed: 8 years 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.
Description
•