Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Description • 15 years ago

nsTextStore::GetACPFromPoint is not implemented yet. It is used for mouse handling of TIPs and some others. We can implement it easy by bug 492233's patch.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 1 • 15 years ago

Oh, this is not used by MS-IME of Vista. But this is useful for the mouse event handling, other TIPs can use this.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 3 • 15 years ago

I don't know what text services use this API now. The API is implemented from MSDN.

http://msdn.microsoft.com/en-us/library/ms538418%28VS.85%29.aspx

And the tests in TestWinTSF.cpp work fine. I think that this is ok.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 4 • 15 years ago

Comment on attachment 378535 [details] [diff] [review]
Patch v1.0.1

This patch's gonna be broken by bug 528396.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 5 • 11 years ago

See bug 970860. Some TIPs may use this method for avoiding a bug of TSF.

Even if the point is out of the window which is returned by GetWnd(), we should return TS_E_NOLAYOUT while layout hasn't been updated.

Additionally, such TIPs should specify the point of out of the window for not wasting the running cost when they use this method for checking if the layout is updated. So, we should return TS_E_INVALIDPOINT if the point is out of the window before actually looking for the character at the position.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 6 • 9 years ago

ITextStore::GetACPFromPoint() may retrieve offset of insertion point. I.e., caret position if the point is clicked.

For implementing this feature, NS_QUERY_CHARACTER_AT_POINT should return the insertion offset too.

Although, this patch isn't e10s-aware, but it's okay for now.

Updating the font-size of <textarea> and <textbox> in the test is necessary for testing the point of immediately before after the target character rect.

However, at testing the left side of characters which is the top of a line, it needs enough padding-left. However, I couldn't specify it for each platform. So, charAtPt3 test is skipped if the point is left side of start of a line.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 9 years ago

Comment on attachment 8585949 [details] [diff] [review]
part.1 NS_QUERY_CHARACTER_AT_POINT should also return insertion offset for the point

I don't understand what this is about.
What is mInsertionOffset? What does it refer to, and what is mOffset then?

Some comment talks about insertion point, but that is misleading, since
insertion point in general refer to XBL's or Shadow DOM's insertion points.

We need some comments here explaining the different offsets.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 9 • 9 years ago

(In reply to Olli Pettay [:smaug] (review queue is getting too long. New requests after Easter, please) from comment #8)
> Comment on attachment 8585949 [details] [diff] [review]
> part.1 NS_QUERY_CHARACTER_AT_POINT should also return insertion offset for
> the point
> 
> I don't understand what this is about.
> What is mInsertionOffset? What does it refer to, and what is mOffset then?

mOffset is the character's offset at the refPoint. mInsertionOffset is the caret offset if user clicks at the refPoint. I.e., if it's clicked in right-half of a character's bounding box, the offset is +1.
 
> Some comment talks about insertion point, but that is misleading, since
> insertion point in general refer to XBL's or Shadow DOM's insertion points.

Hmm, the term is used by documents of Apple... As far as I know, there is no term for it in W3C's spec...

> We need some comments here explaining the different offsets.

Okay. How about mCaretOffset? (although, even it does NOT refer actual caret's offset...)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 10 • 9 years ago

Or, should I just explain it in TextEvents.h and nsIQueryContentEventResult.idl?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> > We need some comments here explaining the different offsets.
> 
> Okay. How about mCaretOffset? (although, even it does NOT refer actual
> caret's offset...)
Ah, so mInsertionOffset is the offset where caret would be if user clicked?

Could we rename mOffset to mCharacterOffset and then mInsertionOffset might be ok.
Though, mCharacterOffset and mTentativeCaretOffset might work too.
Either way, and with comments.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 12 • 9 years ago

(In reply to Olli Pettay [:smaug] (review queue is getting too long. New requests after Easter, please) from comment #11)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> > > We need some comments here explaining the different offsets.
> > 
> > Okay. How about mCaretOffset? (although, even it does NOT refer actual
> > caret's offset...)
> Ah, so mInsertionOffset is the offset where caret would be if user clicked?

Yes.

> Could we rename mOffset to mCharacterOffset and then mInsertionOffset might
> be ok.

mOffset may be caret offset if the event is NS_QUERY_CARET_OFFSET :-(

> Though, mCharacterOffset and mTentativeCaretOffset might work too.

So, mOffset and mTentativeCaretOffset could be...

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 9 years ago

Comment on attachment 8587897 [details] [diff] [review]
part.1 NS_QUERY_CHARACTER_AT_POINT should also return tentative caret offset for the point

Could you file a followup to initialize all 
WidgetQueryContentEvent.mReply.* to some sane values when WidgetQueryContentEvent is created.
The current setup is a bit scary looking, but this patch doesn't change that.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 16 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #15)
> Comment on attachment 8587897 [details] [diff] [review]
> part.1 NS_QUERY_CHARACTER_AT_POINT should also return tentative caret offset
> for the point
> 
> Could you file a followup to initialize all 
> WidgetQueryContentEvent.mReply.* to some sane values when
> WidgetQueryContentEvent is created.
> The current setup is a bit scary looking, but this patch doesn't change that.

Okay.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 17 • 9 years ago

Comment on attachment 8587899 [details] [diff] [review]
part.2 Implement ITextStoreACP::GetACPFromPoint()

Although, I've not found TIP which uses this API. However, before we enable TSF on release build, we should implement this API for TIP developers.

	Masatoshi Kimura [:emk]
	Comment 18 • 9 years ago

Comment on attachment 8587899 [details] [diff] [review]
part.2 Implement ITextStoreACP::GetACPFromPoint()

Review of attachment 8587899 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsTextStore.cpp
@@ +106,5 @@
>  
> +static const nsCString
> +GetACPFromPointFlagName(DWORD aFlags)
> +{
> +  nsAutoCString description;

Could you make this an out parameter passed by reference? It is not efficient to return nsAutoCString from a function because it will copy the internal 64-unit buffer.
Or please change this function to a subclass of nsAutoString like NS_ConvertUTF16toUTF8.

@@ +3204,5 @@
> +  }
> +
> +  bool useTentativeCaretOffset =
> +    (dwFlags & GXFPF_ROUND_NEAREST) ||
> +    charAtPt.mReply.mOffset == WidgetQueryContentEvent::NOT_FOUND;

If GXFPF_NEAREST is set, GXFPF_ROUND_NEAREST is not set, and pt is after a character bounding box (such as point 2 screen coordinates in the MSDN example), this method will set pacp to the caret offset (it will be 2 rather 1 in the MSDN example). Is it a correct behavior? Or am I missing something?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 19 • 9 years ago

(In reply to Masatoshi Kimura [:emk] from comment #18)
> Comment on attachment 8587899 [details] [diff] [review]
> part.2 Implement ITextStoreACP::GetACPFromPoint()
> 
> Review of attachment 8587899 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsTextStore.cpp
> @@ +106,5 @@
> >  
> > +static const nsCString
> > +GetACPFromPointFlagName(DWORD aFlags)
> > +{
> > +  nsAutoCString description;
> 
> Could you make this an out parameter passed by reference? It is not
> efficient to return nsAutoCString from a function because it will copy the
> internal 64-unit buffer.

Hmm, out param isn't useful for logging code.

> Or please change this function to a subclass of nsAutoString like
> NS_ConvertUTF16toUTF8.

I like this better.

> > +  bool useTentativeCaretOffset =
> > +    (dwFlags & GXFPF_ROUND_NEAREST) ||
> > +    charAtPt.mReply.mOffset == WidgetQueryContentEvent::NOT_FOUND;
> 
> If GXFPF_NEAREST is set, GXFPF_ROUND_NEAREST is not set, and pt is after a
> character bounding box (such as point 2 screen coordinates in the MSDN
> example), this method will set pacp to the caret offset (it will be 2 rather
> 1 in the MSDN example). Is it a correct behavior? Or am I missing something?

Oh, you're right. I misunderstood the flags.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 20 • 9 years ago

Rewrote with new understanding.

Unfortunately, dwFlags is only GXFPF_NEAREST and the point isn't contained in any character's bonding box, we don't have a good API to retrieve the nearest character from the point. Therefore, this patch guesses it from the tentative caret offset. If the point hits right half of a character, the result is wrong, but this is the "better"　than other ideas which I found. For example, following picture is bottom-most line's characters:

0        1         2
|        |         |
+--------+---------+
     0         1

  *                    : guessed as 0
       *               : guessed as 1 (wrong)
            *          : guessed as 1
                 *     : guessed as 1 (because of the last character)

Therefore, I guess that, it's okay for now. If we'll fix a lot of issues around NS_QUERY_* in e10s mode, I'll revisit this again.

	Masatoshi Kimura [:emk]
	Comment 21 • 9 years ago

Comment on attachment 8590880 [details] [diff] [review]
part.2 Implement ITextStoreACP::GetACPFromPoint()

Review of attachment 8590880 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsTextStore.cpp
@@ +3267,5 @@
> +             ("TSF: 0x%p   nsTextStore::GetACPFromPoint() FAILED due to "
> +              "LockedContent() failure", this));
> +      return E_FAIL;
> +    }
> +    if (lockedContent.Text().Length() >= offset) {

Isn't it |offset >= lockedContent.Text().Length()|?
r=me with this fixed (or an explanation of the reason why I'm wrong).

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 22 • 9 years ago

(In reply to Masatoshi Kimura [:emk] from comment #21)
> Comment on attachment 8590880 [details] [diff] [review]
> part.2 Implement ITextStoreACP::GetACPFromPoint()
> > +    if (lockedContent.Text().Length() >= offset) {
> 
> Isn't it |offset >= lockedContent.Text().Length()|?

Good catch! Thank you!!

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 23 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf74485ea6a
https://hg.mozilla.org/integration/mozilla-inbound/rev/b617cfc2df87

	Carsten Book [:Tomcat]
	Comment 24 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/3cf74485ea6a
https://hg.mozilla.org/mozilla-central/rev/b617cfc2df87