492394 - [TSF] Implement nsTextStore::GetACPFromPoint

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

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.

Comment 1

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

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

Comment 2

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

Attachment: Patch v1.0

Comment 3

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

Attachment: Patch v1.0.1

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.

Comment 4

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

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

This patch's gonna be broken by bug 528396.

Comment 5

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

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.

Comment 6

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

Attachment: part.1 NS_QUERY_CHARACTER_AT_POINT should also return insertion offset for the point

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.

Comment 7

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

Attachment: part.2 Implement ITextStoreACP::GetACPFromPoint()

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

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

(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...)

Comment 10

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

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

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 12

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

(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...

Comment 13

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

Attachment: part.1 NS_QUERY_CHARACTER_AT_POINT should also return tentative caret offset for the point

Comment 14

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

Attachment: part.2 Implement ITextStoreACP::GetACPFromPoint()

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 16

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

(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.

Comment 17

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

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.

Comment 18

[Masatoshi Kimura [:emk]]

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?

Comment 19

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

(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.

Comment 20

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

Attachment: part.2 Implement ITextStoreACP::GetACPFromPoint()

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.

Comment 21

[Masatoshi Kimura [:emk]]

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).

Comment 22

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

(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!!

Comment 23

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

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

Comment 24

[Carsten Book [:Tomcat]]

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