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

RESOLVED FIXED in mozilla10

Status

()

Core
Layout: Text
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Depends on: 1 bug)

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 567992 [details] [diff] [review]
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.
Attachment #567992 - Flags: review?(roc)
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.
Attachment #567992 - Flags: review?(roc) → review-
(Assignee)

Comment 2

6 years ago
Created attachment 568322 [details] [diff] [review]
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!
Attachment #567992 - Attachment is obsolete: true
Attachment #568322 - Flags: review?(roc)
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...
Attachment #568322 - Flags: review?(roc) → review+
(Assignee)

Comment 4

6 years ago
Created attachment 568423 [details] [diff] [review]
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.
Attachment #568423 - Flags: review?(roc)
Attachment #568423 - Flags: review?(roc) → review+
(Assignee)

Comment 5

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/da826567f795 (mochitest)
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb302f2dd9c2 (patch)
https://hg.mozilla.org/mozilla-central/rev/da826567f795
https://hg.mozilla.org/mozilla-central/rev/fb302f2dd9c2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 736332
You need to log in before you can comment on or make changes to this bug.