734319 - nsTextFrame::DrawTextRunAndDecorations should limit decoration to the offset/length given

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[Cameron McCormack (:heycam)]

nsTextFrame::DrawTextRunAndDecorations currently restricts any line decorations that it draws to aClipEdges but not to aOffset/aLength.  I am calling into the nsTextFrame::PaintText with specific offset/lengths to paint substrings of a text frame and this is currently painting the decoration for the entire width of the frame.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Limit line decorations to the given offset/length when painting a text frame.

Measure the substring of the text run to determine how much decoration to paint.

Comment 2

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 604302
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=5d90a5a2a32d
Try run started, revision 5d90a5a2a32d. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=5d90a5a2a32d

Comment 3

[Cameron McCormack (:heycam)]

Forgot to #include <limits>...

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 604302 [details] [diff] [review]
Limit line decorations to the given offset/length when painting a text frame.

>+    nscoord width;
>+    if (aOffset == 0 && aLength == std::numeric_limits<PRUint32>::max()) {
>+      width = GetRect().width;

Is that really the normal-case value of aLength?

(And if it is, can you just write it as PR_UINT32_MAX ?)

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 604302 [details] [diff] [review]
Limit line decorations to the given offset/length when painting a text frame.

(In reply to David Baron [:dbaron] from comment #4)
> Is that really the normal-case value of aLength?

Hmm, I got confused with other local changes I've got in my tree.  Will check and revise.

Comment 6

[Mozilla RelEng Bot]

Try run for 5d90a5a2a32d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5d90a5a2a32d
Results (out of 14 total builds):
    success: 2
    failure: 12
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5d90a5a2a32d

Comment 7

[Mats Palmgren (inactive)]

Sorry for the drive-by review, but I don't think this is the right place
to do this.  If aOffset/aLength is a sub-range of the frame's text then
aClipEdges is expected to clip to that range.  It is the *caller's*
responsibility calculate this (if clipping text-decorations is desired).

Comment 8

[Cameron McCormack (:heycam)]

Mats, I guess I could compute the points for aClipEdges so that it clips to the characters that I want, but I already have a content offset/length.  Is it worth it to turn these into nscoords just so that they can be converted back to the offset/length pair to restrict the decoration rendering to?

Comment 9

[Cameron McCormack (:heycam)]

Or are there currently situations where we do need to render a substring of the frame's glyphs but say all of the decoration?

Comment 10

[Mats Palmgren (inactive)]

> Is it worth it to turn these into nscoords just so that they can be converted
> back to the offset/length pair to restrict the decoration rendering to?

What do you mean? there's no "converting back" going on in this method.
Drawing the text use aOffset/aLength, drawing the decorations use
nscoord that comes from GetRect().width, clipped to aClipEdges.

On the contrary, the suggested patch would slow down existing callers
that already provides a correct aOffset/aLength/aClipEdges by doing a
redundant mTextRun->GetAdvanceWidth().

Currently we calculate aClipEdges *once* and then pass that around to
a bunch of helper methods that does the drawing.  Calculating the clip
in each of the leaf methods would be much more expensive.
Consider for example:

PaintTextWithSelectionColors
  for (PRUint32 i = textStyle->mTextShadow->Length(); i > 0; --i) {
    PaintOneShadow
      DrawText
        DrawTextRunAndDecorations


> Or are there currently situations where we do need to render a substring of
> the frame's glyphs but say all of the decoration?

No, I don't think so.

Comment 11

[Cameron McCormack (:heycam)]

OK, fair enough, thanks.