[TSF] Implement nsTextStore::GetACPFromPoint

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod})

Trunk
mozilla40
x86
Windows Vista
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

20.18 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.71 KB, patch
emk
: review+
Details | Diff | Splinter Review
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.
Blocks: 88831
Whiteboard: Needed for IME mouse handling
Oh, this is not used by MS-IME of Vista. But this is useful for the mouse event handling, other TIPs can use this.
Status: NEW → ASSIGNED
Whiteboard: Needed for IME mouse handling
Posted patch Patch v1.0.1 (obsolete) — Splinter Review
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.
Attachment #378520 - Attachment is obsolete: true
Attachment #378535 - Flags: review?(VYV03354)
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.
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.
Attachment #378535 - Attachment is obsolete: true
Attachment #8585949 - Flags: superreview?(bugs)
Attachment #8585949 - Flags: review?(bugs)
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.
Attachment #8585949 - Flags: superreview?(bugs)
Attachment #8585949 - Flags: superreview-
Attachment #8585949 - Flags: review?(bugs)
Attachment #8585949 - Flags: review-
(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...)
Or, should I just explain it in TextEvents.h and nsIQueryContentEventResult.idl?
(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.
Flags: needinfo?(bugs)
(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...
Flags: needinfo?(bugs)
Flags: needinfo?(bugs)
Attachment #8587897 - Flags: superreview?(bugs) → superreview+
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.
Attachment #8587897 - Flags: review?(bugs) → review+
(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 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.
Attachment #8587899 - Flags: review?(VYV03354)
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?
(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.
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.
Attachment #8587899 - Attachment is obsolete: true
Attachment #8587899 - Flags: review?(VYV03354)
Attachment #8590880 - Flags: review?(VYV03354)
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).
Attachment #8590880 - Flags: review?(VYV03354) → review+
(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!!
https://hg.mozilla.org/mozilla-central/rev/3cf74485ea6a
https://hg.mozilla.org/mozilla-central/rev/b617cfc2df87
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.