Closed Bug 482138 Opened 11 years ago Closed 11 years ago
Text Frame has to draw composition string by TIP specified style
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.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=4005&action=diff Latest patch is here. I'll finish the work soon.
This patch could be reviewed if there are no problems.
Attachment #367614 - Attachment is obsolete: true
maybe, this is final.
Attachment #367798 - Attachment is obsolete: true
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.
Ah, on mac, the spellchecker underline should be thicker dotted line...
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.
+ /** + * 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.
testing on tryserver.
Attachment #368425 - Attachment is obsolete: true
Attachment #373562 - Attachment is obsolete: true
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.
+ * 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.
> > 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.
+ 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 on attachment 374034 [details] [diff] [review] Patch v2.3 Thanks!
http://hg.mozilla.org/mozilla-central/rev/7dd1622f62d9 Thank you, roc.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.