1116480 - bounding box returned by a text run does not account for synthetic-italic glyphs that may project beyond the original glyph bounding box

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Kew [:jfkthame]]

See bug 1116315 for a screenshot and simple testcase based on the Gaia SMS app.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: gfxTextRun::Measure needs to account for fake-italic transform when returning the bounding box

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for textRun bounding box with fake italics

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8542680 [details] [diff] [review]
gfxTextRun::Measure needs to account for fake-italic transform when returning the bounding box

Review of attachment 8542680 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +2276,5 @@
> +    // bottom-left being skewed further left.
> +    if (mStyle.style != NS_FONT_STYLE_NORMAL && !mFontEntry->IsItalic()) {
> +        metrics.mBoundingBox.width += 0.3 * metrics.mBoundingBox.height;
> +        metrics.mBoundingBox.x -=
> +          0.3 * (metrics.mBoundingBox.height + metrics.mBoundingBox.y);

metrics.mBoundingBox.YMost()

While these seem correct, the derivation is non-trivial and I think we should make the code clearer. Also, I believe these are supposed to be nscoords so we need to do some floor/ceil. So write something like
  gfxFloat extendLeftEdge = ceil(0.3 * metrics.mBoundingBox.YMost());
  gfxFloat extendRightEdge = ceil(0.3 * -metrics.mBoundingBox.y);
  metrics.mBoundingBox.width += extendLeftEdge + extendRightEdge;
  metrics.mBoundingBox.x -= extendLeftEdge;

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: gfxTextRun::Measure needs to account for fake-italic transform when returning the bounding box

Updated according to comment 3. I also thought we should do something about the "magic number" used for the skew factor, which is currently defined in various places across the different backends; moved that to a single location in gfxFont.h.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8542944 [details] [diff] [review]
gfxTextRun::Measure needs to account for fake-italic transform when returning the bounding box

Review of attachment 8542944 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 6

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d39f67b09a5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7dbc64435bc

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

https://tbpl.mozilla.org/?tree=Try&rev=f7dbc64435bc

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d39f67b09a5d
https://hg.mozilla.org/mozilla-central/rev/f7dbc64435bc

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

According to my finding in bug 1116315 comment 17.