All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

P1
major
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Depends on: 2 bugs, {inputmethod})

Trunk
mozilla1.9.2a1
x86
Windows XP
inputmethod
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 16 obsolete attachments)

1.16 KB, image/png
Details
42.89 KB, patch
roc
: review+
Details | Diff | Splinter Review
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.
(Assignee)

Updated

10 years ago
Priority: -- → P1
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=4005&action=diff

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

Updated

10 years ago
Blocks: 338209
(Assignee)

Updated

10 years ago
Blocks: 59109
Created attachment 367022 [details] [diff] [review]
Patch v0.3 (work in progress)
Created attachment 367032 [details] [diff] [review]
Patch v0.3.1 (work in progress)
Attachment #367022 - Attachment is obsolete: true
Created attachment 367253 [details] [diff] [review]
Patch v0.4 (work in progress)
Attachment #367032 - Attachment is obsolete: true
Created attachment 367435 [details] [diff] [review]
Patch v0.5 (work in progress)
Attachment #367253 - Attachment is obsolete: true
Created attachment 367486 [details] [diff] [review]
Patch v0.6 (work in progress)
Attachment #367435 - Attachment is obsolete: true
Created attachment 367531 [details] [diff] [review]
Patch v0.7 (work in progress. experimental)
Attachment #367486 - Attachment is obsolete: true
Created attachment 367614 [details] [diff] [review]
Patch v0.8 (work in progress)
Attachment #367531 - Attachment is obsolete: true
Created attachment 367798 [details] [diff] [review]
Patch v0.9

This patch could be reviewed if there are no problems.
Attachment #367614 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #367798 - Attachment is patch: true
Attachment #367798 - Attachment mime type: application/octet-stream → text/plain
Created attachment 367960 [details] [diff] [review]
Patch v0.10

maybe, this is final.
Attachment #367798 - Attachment is obsolete: true
Created attachment 368214 [details] [diff] [review]
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.
Attachment #367960 - Attachment is obsolete: true
Attachment #368214 - Flags: superreview?(roc)
Attachment #368214 - Flags: review?(roc)
Created attachment 368216 [details]
wavy underline sample on Windows
Ah, on mac, the spellchecker underline should be thicker dotted line...
(Assignee)

Updated

10 years ago
Attachment #368214 - Attachment is obsolete: true
Attachment #368214 - Flags: superreview?(roc)
Attachment #368214 - Flags: review?(roc)
Created attachment 368425 [details] [diff] [review]
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.
Attachment #368279 - Attachment is obsolete: true
Attachment #368425 - Flags: superreview?(roc)
Attachment #368425 - Flags: review?(roc)
+    /**
+     * 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?
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.
(Assignee)

Updated

10 years ago
No longer blocks: 338209
Depends on: 338209
(Assignee)

Updated

10 years ago
Attachment #368425 - Flags: superreview?(roc)
Attachment #368425 - Flags: review?(roc)
(Assignee)

Updated

10 years ago
Depends on: 486708
Created attachment 373562 [details] [diff] [review]
Patch v2.0

testing on tryserver.
Attachment #368425 - Attachment is obsolete: true
Created attachment 373564 [details] [diff] [review]
Patch v2.1

cleaning-up
Attachment #373562 - Attachment is obsolete: true
Created attachment 373572 [details] [diff] [review]
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.
Attachment #373564 - Attachment is obsolete: true
Attachment #373572 - Flags: superreview?(roc)
Attachment #373572 - Flags: review?(roc)
+     * 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.
Created attachment 374010 [details] [diff] [review]
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.
Attachment #373572 - Attachment is obsolete: true
Attachment #374010 - Flags: superreview?(roc)
Attachment #374010 - Flags: review?(roc)
Attachment #373572 - Flags: superreview?(roc)
Attachment #373572 - Flags: review?(roc)
+            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.
Created attachment 374034 [details] [diff] [review]
Patch v2.3
Attachment #374010 - Attachment is obsolete: true
Attachment #374034 - Flags: superreview?(roc)
Attachment #374034 - Flags: review?(roc)
Attachment #374010 - Flags: superreview?(roc)
Attachment #374010 - Flags: review?(roc)
Comment on attachment 374034 [details] [diff] [review]
Patch v2.3

Thanks!
Attachment #374034 - Flags: superreview?(roc)
Attachment #374034 - Flags: superreview+
Attachment #374034 - Flags: review?(roc)
Attachment #374034 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/7dd1622f62d9

Thank you, roc.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Updated

10 years ago
Depends on: 489726
(Assignee)

Updated

10 years ago
Depends on: 489951
Depends on: 553975
Depends on: 554822
Depends on: 560071
Keywords: inputmethod
You need to log in before you can comment on or make changes to this bug.