482138 - [TSF] nsTextFrame has to draw composition string by TIP specified style

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

TIP can specify the selection text style of composition string. MS Natural Input is not specifying the attribute for each clauses. It always sets TF_ATTR_OTHER to them. Therefore, all composition string are drawn as raw inputted text. So, the users cannot see which clause is focused.

nsTextFrame has to draw the composition string by TIP specified style. For that, nsTextFrame needs to know the style via selection, editor and text event. So, the patch will be big, I need to fix this as soon as possible.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://bugzilla.mozilla.gr.jp/attachment.cgi?id=4005&action=diff

Latest patch is here. I'll finish the work soon.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v0.3 (work in progress)

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v0.3.1 (work in progress)

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

test build:

https://build.mozilla.org/tryserver-builds/2009-03-12_06:[e-mail]-attachment367032 [review]/

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v0.4 (work in progress)

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v0.5 (work in progress)

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v0.6 (work in progress)

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v0.7 (work in progress. experimental)

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v0.8 (work in progress)

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v0.9

This patch could be reviewed if there are no problems.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v0.10

maybe, this is final.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

O.K. This can be reviewed now.

This patch sends selection style which is specified by TIP. nsTextStore sets the style to each nsTextRange::mRangeStyle (nsTextRangeStyle). And nsIPrivateTextRange has same meaning struct (nsIPrivateTextRange::RangeStyle). The layout code uses the latter struct which was assigned from the former.

nsIMETextTxn registers the nsIPrivateTextRange instances to nsTypedSelection. nsTypedSelection sets them to each SelectionDetails in nsTypedSelection::LookUpSelection. nsTextFrame checks whether each IME selection style is specified or not. If it is specified, nsTextFrame uses the style to the selection rendering. Otherwise, it uses current implementation.

This patch supports new 2 features:

1. Wavy docoration line:

TIP can specify wavy style line to IME selection's underline. Therefore, we have to support it in this bug. Wavy line drawing logic is documented in nsCSSRendering::PaintDecorationLine and nsCSSRendering::GetTextDecorationRectInternal. And also this patch changes the spellchecker's underline style to wavy for bug 338209.

2. The decoration line is rendered in decent space as far as possible:

The selection decoration line should not be overlapped to other line text, and also is should not be clipped by bottom edge of input element. Therefore, if the decoration line is rendered for selection, the decoration line is lifted up as far as possible if the underline will be overflowed. But note that the underline can be overflowed at that time if there is no enough space.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: wavy underline sample on Windows

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, on mac, the spellchecker underline should be thicker dotted line...

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.1

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.2

Oh, the previous patch fails the TSF unit test. Because when text selection is changed and the text frame has overflowing selection (spell checker), the text frame may be dirty when nsContentEventHandler::Init is called. At that time, nsICaret::GetCaretCoordinates fails, because it calls nsIFrame::GetPointFromOffset which has to be called only when the frame is not dirty.

So, we need to call nsIPresShell::FlushPendingNotifications from nsContentEventHandler::Init.

Comment 17

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

+    /**
+     * Set an nsIPrivateTextRange instance which creates the range.
+     */
+    [noscript] void setPrivateTextRangeFor(in nsIDOMRange range,
+                      in nsIPrivateTextRange privateTextRange);

This is not clear. What does this actually do?

+        // this case.  And probably, users don't think the non-AAed wavy line.

I'm not sure what you wanted to say here.

+  gfxFloat suggestiveMaxRectHeight = PR_MAX(PR_MIN(ascent, descentLimit), 1.0);

"suggestedMaxRectHeight"

+      // Don't shrink the line height.  Because the thickness has some meanings.

"some meaning"

+      // Wavy line's top edge can overlap to baseline.  Because even if wavy
+      // line overlaps to the text on its baseline, that doesn't cause of
+      // unreadable.  So, we can use both the descent space and baseline.

"Because even if the wavy line overlaps the baseline of the text, that shouldn't cause unreadability".

An ASCII-art picture of what's going on in GetTextDecorationRectInternal for the DOUBLE and WAVY cases would be very helpful.

Could you add some documentation to nsIPrivateTextRange explaining what the start and end PRUint16 values mean? Seems like they should be PRUint32 too...

-                                  float*   aRelativeSize,
+                                  float* aRelativeSize,

Why this change?

Why is sSelectionUnderline only initialized once? It seems to me it should be initialized every time. The look and feel can change.

+// keep in synch with nsIPrivateTextRange.h

Can't nsIPrivateTextRange.h include this file and use these constants?

In each SelectionDetails do we really need a full nsIPrivateTextRange? Don't we just need the style data? I assume the end points match the SelectionDetails?

This patch is large and quite hard to follow. Could we break it up into a patch that adds the support for passing styles from the IME up to the text frame, then another patch that adds WAVY support, and another patch that lifts up the decoration line as necessary?

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

ok, I'll post WAVY support patch and lifts up the decoration line patch to bug 338209, first. And after fix the bug, I'll be back to this bug.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

testing on tryserver.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.1

cleaning-up

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.1.1

updating a comment.

This patch fixes a bug of resolving underline color in DrawSelectionDecorations of nsTextFrameThebes.cpp.

(In reply to comment #17)
> +    /**
> +     * Set an nsIPrivateTextRange instance which creates the range.
> +     */
> +    [noscript] void setPrivateTextRangeFor(in nsIDOMRange range,
> +                      in nsIPrivateTextRange privateTextRange);
> 
> This is not clear. What does this actually do?

That sets the painting style of the range. I rewrite the comment, please check it.

> +// keep in synch with nsIPrivateTextRange.h
> 
> Can't nsIPrivateTextRange.h include this file and use these constants?
> 
> In each SelectionDetails do we really need a full nsIPrivateTextRange? Don't we
> just need the style data? I assume the end points match the SelectionDetails?

I moved the range style struct to nsGUIEvent.h. And I combined the both structs of the previous patch. nsTextRangeStyleBase is only used for the member of nsTextRange. nsTextRangeStyle is used on XP part, it inherits nsTextRangeStyleBase and it can be used for nsRefPtr.

I need to separate to two classes of it. Because nsTextRange is used with nsTArray. nsTArray uses alloc/realloc instead of new. Therefore, the refcounting doesn't work fine.

Comment 22

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

+     * Set the selection's painting style of the range. The range must be same as
+     * a range of selections.  The textRangeStyle will be used by text frame
+     * when it is painting the selection.

 * Set the painting style for the range. The range must be a range in
 * the selection. The textRangeStyle will be used by text frame
 * when it is painting the selection.

> I need to separate to two classes of it. Because nsTextRange is used with
> nsTArray. nsTArray uses alloc/realloc instead of new. Therefore, the
> refcounting doesn't work fine.

I'm not sure what you mean. nsTArray will run the constructors and destructors for your objects, so it would be OK for nsTextRange to contain an nsRefPtr. But why is nsTextRangeStyle refcounted? We could just copy the nsTextRangeStyle wherever we need it, it's only 16 bytes.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.2

> > I need to separate to two classes of it. Because nsTextRange is used with
> > nsTArray. nsTArray uses alloc/realloc instead of new. Therefore, the
> > refcounting doesn't work fine.
> 
> I'm not sure what you mean. nsTArray will run the constructors and destructors
> for your objects, so it would be OK for nsTextRange to contain an nsRefPtr.

Ugh, you're right, maybe. This patch works fine, but the old code didn't work fine.

> why is nsTextRangeStyle refcounted? We could just copy the nsTextRangeStyle
> wherever we need it, it's only 16 bytes.

OK, this patch does so.

Comment 24

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

+            nsTextPaintStyle::GetUnderlineStyleIndexForSelectionType(aType);
+  PRBool weDefineSelectionUnderline =
+           aTextPaintStyle.GetSelectionUnderlineForPaint(
+                             index, &color, &relativeSize, &style);

Don't indent these so much.

+        newRange.mRangeStyle.mIsBoldLine = attr.fBoldLine ? 1 : PR_FALSE;

attr.fBoldLine != 0

+  nsAutoTArray<nsTextRangeStyle, BIG_TEXT_NODE_SIZE> prevailingSelectionStyles;

Turns out this array is pretty big, 64K I guess. That's not good on the stack. How about we get rid of this array and just make prevailingSelections contain a pointer to the SelectionDetails for the character? from there you can obtain the selection type and rangeStyle.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.3

Comment 26

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

Comment on attachment 374034 [details] [diff] [review]
Patch v2.3

Thanks!

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/7dd1622f62d9

Thank you, roc.