695639 - nsLayoutUtils::GetFontFacesForText does not use the proper offset into the textrun

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

Attachment: patch, add the frame's contentoffset when looking at the textrun

In nsLayoutUtils::GetFontFacesForText, we fail to re-add the appropriate ContentOffset after converting the original offset to a skipped offset; as a result, we get font information for the wrong range of characters in the textrun, in the case of a single run that's being rendered across multiple lines.

This is the cause of the issue described in Fanolian's review of the fontinfo add-on at https://addons.mozilla.org/en-US/firefox/addon/fontinfo/; the results vary depending on window width because this affects how the textrun is wrapped.

Comment 1

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

Comment on attachment 567992 [details] [diff] [review]
patch, add the frame's contentoffset when looking at the textrun

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

::: layout/base/nsLayoutUtils.cpp
@@ +4276,5 @@
>      PRUint32 skipStart = iter.ConvertOriginalToSkipped(fstart - offset);
>      PRUint32 skipEnd = iter.ConvertOriginalToSkipped(fend - offset);
>      aFontFaceList->AddFontsFromTextRun(textRun,
> +                                       offset + skipStart,
> +                                       skipEnd - skipStart,

This can't be right, 'offset' is a DOM offset, and you're treating it as a textrun offset here.

I think the problem is that you should be using the iterator returned from EnsureTextRun, which is set up with the right internal offsets for that text node.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch v2 - use the proper iterator to convert offsets

Ah, that makes much more sense - I was completely forgetting that EnsureTextRun could return something useful!

Comment 3

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

Comment on attachment 568322 [details] [diff] [review]
patch v2 - use the proper iterator to convert offsets

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

needs a test too...

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: patch, add chrome mochitest for this bug

Simple testcase using a string that wraps onto several lines; it fails without the patch here because it doesn't correctly find the Chinese font used to display the 你好 glyphs that are wrapped onto the second line.

Comment 5

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da826567f795 (mochitest)
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb302f2dd9c2 (patch)

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/da826567f795
https://hg.mozilla.org/mozilla-central/rev/fb302f2dd9c2