Last Comment Bug 695639 - nsLayoutUtils::GetFontFacesForText does not use the proper offset into the textrun
: nsLayoutUtils::GetFontFacesForText does not use the proper offset into the te...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jonathan Kew (:jfkthame)
:
:
Mentors:
Depends on: 736332
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-19 03:10 PDT by Jonathan Kew (:jfkthame)
Modified: 2012-03-15 17:45 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, add the frame's contentoffset when looking at the textrun (1.03 KB, patch)
2011-10-19 03:10 PDT, Jonathan Kew (:jfkthame)
roc: review-
Details | Diff | Splinter Review
patch v2 - use the proper iterator to convert offsets (1.81 KB, patch)
2011-10-19 23:50 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
patch, add chrome mochitest for this bug (3.90 KB, patch)
2011-10-20 09:46 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review

Description Jonathan Kew (:jfkthame) 2011-10-19 03:10:17 PDT
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.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-19 14:51:57 PDT
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) 2011-10-19 23:50:22 PDT
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!
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-20 01:26:58 PDT
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) 2011-10-20 09:46:15 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.