241485 - do_BreakGetTextDimensions calling GetWidth inappropriately

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Win32, defect)

Description

[tor]

I'm seeing a case where with the following text:

  <ascii text><indic text><dash><indic text>

do_BreakGetTextDimensions is called with the substring of just the
<dash> with an ascii font (Times New Roman in my case).  It measures
that, then backs up further and calls GetWidth on the substring
consisting of the first indic section and the dash.

Attached is a testcase and a small patch that will output a message
when GetWidth is called with a string containing glyphs not in that
font.  You need to narrow the window fairly drastically to trigger
the problem.

Comment 1

[tor]

Attachment: testcase

Comment 2

[tor]

Attachment: problem demonstration patch

Comment 3

[rbs]

Attachment: testcase with explicit charset=UTF8

Comment 4

[rbs]

Attachment: proposed patch - propagated to all platforms

With DEBUG_VERIFY_FONT_HASGLYPH without the rest of the fix, resizing the
windows triggers the assertion.

Comment 5

[rbs]

I am not entirely satisfy with this bit of code:

+        else {
+          // Don't back up beyond the current font.
+          width = data->mWidth;
+          break;
+        }

What this does it to fall back to the width that the code started with. That is,
the width from the previous call. The desired width is really the width up to
the what is retuned by the current |numCharsFit|. This would involve switching
back to the previous fonts, and measuring any parts that they contribute from
&pstr[start] onwards.

Comment 6

[rbs]

Attachment: testcase - demonstration of excess width

Comment 7

[rbs]

...need the above patch, then resize to visualize the problem that I am
referring too. The width, as shown by the background-color, can be much larger
than the text.

Comment 8

[rbs]

Attachment: full patch that computes the correct width (Windows only for now)

This patch does what it takes to get the width up to where the string is
chopped.

Comment 9

[rbs]

Attachment: patch for all platforms

Comment 10

[rbs]

Comment on attachment 147260 [details] [diff] [review]
patch for all platforms

I think it is now ready for r/sr.

Comment 11

[rbs]

Comment on attachment 147260 [details] [diff] [review]
patch for all platforms

+	 else if (pstr + start > aSubstring) {

Should read '>=' everywhere (as in the Windows part). Corrected in my tree.

Comment 12

[tor]

Comment on attachment 147260 [details] [diff] [review]
patch for all platforms

> data->mOffsets->ReplaceElementAt((void*)&pstr[numCharsFit], k+1);

Why is this needed given that we're backing up?

Comment 13

[rbs]

Re: comment 12

It is for consistency. At the previous call, it was determined (by the tail |if
(data->mNumCharsFit != numCharsFit)| that the last font was used to represent 
offsets[k] .. offsets[k+1]-1. Since we are backing up beyond some chars of the
last font, the statement is meant to adjust the last offset accordingly.

Although the last font can represent more, we are chopping off some chars so as
to be at a word boundary.

[BTW, this gives an idea of the kind of gymnastics needed for bug 99457 -- where
enforcing such a chopping at word boundary is precisely what the user doesn't want.]

Comment 14

[rbs]

Comment on attachment 147260 [details] [diff] [review]
patch for all platforms

I am retracting this patch. I should be using 
|data->mOffsets->RemoveElementAt(data->mOffsets->Count()-1)|
inside the |for| loop and then, appending the very last offset again at the end
of the loop. This way, all the intermediate fonts' offsets will be removed
properly. 

Also, as I was testing that, I hit the VERIFY_FONT_HASGLYPH assertion somewhere
else (under "// Measure the text".  

These issues need further investigations before I submit another patch.

Comment 15

[rbs]

Attachment: patch for Windows only

This patch fixes all the issues I had. It also rename the poorly named 'i' to
'end' to improve the reading of that code.

Comment 16

[rbs]

Attachment: all-around patch

jshin, roland, simon, tor, would you guys care to take a look and/or give the
patch a try?

Comment 17

[rbs]

Attachment: better patch (fix mixmatch of copy-paste between platforms)

Comment 18

[rbs]

Any feedback guys? As this is a correctness issue, I would like to get the pacth
in 1.8b2.

Comment 19

[Jungshik Shin]

(In reply to comment #18)
> Any feedback guys? As this is a correctness issue, I would like to get the pacth
> in 1.8b2.

How about Xft and Mac OS? X11core is not the default build any more, but Xft
build is. Anyway, I'll take a look. 

Comment 20

[rbs]

Xft and the Mac OS don't have that variant of the function under consideration:
http://lxr.mozilla.org/seamonkey/source/gfx/public/nsIRenderingContext.h#605

Comment 21

[rbs]

Xft:
http://lxr.mozilla.org/seamonkey/source/gfx/src/cairo/nsFontMetricsXft.cpp#580

Comment 22

[rbs]

To clarify what this breakable GetTextDimensions does, suppose you are given a
string:

word1 word2 ... wordn

with break indices per this comment in GetTextDimensions:

  // Note: aBreaks[] is supplied to us so that the first word is located
  // at aString[0 .. aBreaks[0]-1] and more generally, the k-th word is
  // located at aString[aBreaks[k-1] .. aBreaks[k]-1]. Whitespace can
  // be included and each of them counts as a word in its own right.

in other words, break the string into fragments while keeping whitespace:

"word1", " ", "word2", ..., "wordn"


Now the breakable GetTextDimensions attempts to measure this string up to the
number of words that fit the available room sepecified by the caller. (You can
measure word by word, albeit less efficiently. That's another codepath that
nsTextFrame uses when it decides that the breakable GetTextDimensions isn't
present on a platform or isn't suitable in certain stylistic-related situations).

So the breakable GetTextDimensions measures several words at once. Consequently,
it has to chop some words when it realizes that they exceed the available room.
What happens is that in the course of measuring these words, several fonts are
used, say:

f1 | f2 |f3|  ...   |fm
wor|d1 w|or|d2 ... wordn
  
As you can see, a font can overlap across bits from different words. Now,
suppose that the second fragment of word2 has been measured, i.e. "or", but then
you realise that adding that fragment make the text up to there not to fit
anymore. Although the text up to "wor|d1 w" fits (otherwise you would have
stopped earlier), you must back-up. You must chop "w|or", i.e., the "or" that
you was about to add and its earlier fragment "w". To do so involves reversing
into f2, while making sure to stay at words boundaries. 

It is these details that the patch codifies. 

Comment 23

[rbs]

Comment on attachment 180463 [details] [diff] [review]
better patch (fix mixmatch of copy-paste between platforms)

Switching over jshin for the r. Jshin, is your understanding of this ok now?

Comment 24

[Jungshik Shin]

(In reply to comment #23)
> (From update of attachment 180463 [details] [diff] [review] [edit])
> Switching over jshin for the r. Jshin, is your understanding of this ok now?
yeah, sort of, but can you make a new patch with '-p -u8' option to 'cvs diff'?
It'll make it easier to go through the patch.

Comment 25

[rbs]

Attachment: diff -p -u8

Comment 26

[rbs]

I would have actually preferred to land this before the 1.8 branch is cut since
it is unrelated to cairo, which is being advocated from 1.9.

Comment 27

[Jungshik Shin]

Comment on attachment 182742 [details] [diff] [review]
diff -p -u8

r=jshin
really sorry for dragging my feet here.

Comment 28

[rbs]

Comment on attachment 182742 [details] [diff] [review]
diff -p -u8

Asking a=1.8b4. This is a correctness issue in gfx. The patch was proposed a
long time ago, but fell off the radars. This only shows how it might be
forgotten as more focus goes to cairo after the branch is cut. I have had the
patch in my tree for several months without problems.

Comment 29

[rbs]

Checked in the 1.9 trunk.

Comment 30

[Caleb]

I have Character Encoding > Auto-Detect > Universal set and the fonts on this page:
https://bugzilla.mozilla.org/show_bug.cgi?id=1156 look bigger than usual.

Setting it to UTF8 shows 2 characters from the reporter's name as invalid and
the fonts on the rest of the page are fine.

Comment 31

[rbs]

That's an unrelated problem. FYI, that page is detected as Hebrew on my box
(since the reporter's name has those two Hebrew characters), and so, the default
Hebrew fonts get used on the page. Maybe, just two characters shouldn't be
enough to determine the fate of a whole page... But it avoids the reporter's
name being "invalid". Anyway, as I said, it is not related to this bug. You
could set your preferred Hebrew fonts (or any language detected) and see the
effects of your settings.

Comment 32

[Asa Dotzler [:asa]]

we'll look at this in our 1.8b4 blocker nominations.

Comment 33

[rbs]

Comment 0 and comment 22 give good summary of the problem and the bird's eye
view of fix.

Comment 34

[rbs]

Checked in the 1.8 branch.

Comment 35

[Peter Weilbacher]

Attachment: fix for OS/2 build break on branch

Could someone please check in this trivial patch for the 1.8 branch to fix an
OS/2 build break? (The same change was already checked in to trunk by mkaply on
2005-08-16.)

Comment 36

[rbs]

Comment on attachment 194947 [details] [diff] [review]
fix for OS/2 build break on branch

Done. Typo fixed on the 1.8 branch.