1076657 - [OS X] need to support input method handling within vertical text

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[John Daggett (:jtd)]

When text is edited in vertical text mode, the input method handling code needs to be adjusted to do the "right" thing.

中野さん、よろしく！

Comment 1

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

First of all, I think that we need to add a feature to WidgetQueryContentEvent for retrieving caret position is in vertical layout or horizontal layout...

Comment 2

[Jonathan Kew [:jfkthame]]

With the patch in bug 1074735, the IME selection underlines (for raw text, converted text, etc) should render properly in vertical layout. However, we'll also want to handle positioning of IME windows (suggestions, etc) differently for vertical vs horizontal, and if the IME itself has a vertical UI option, we should use that.

Comment 3

[Jonathan Kew [:jfkthame]]

Positioning of the IME windows depends on querying the content (via a WidgetQueryContentEvent) for the screen position of the first character of the active input range. The result of this query is currently incorrect for vertical text, which leads to (fairly minor) mis-positioning of IME windows. The fix for this, AFAICS, belongs in ContentEventHandler::OnQueryTextRect, which is what implements that query.

Once that is fixed, we get the correct screen rect for the first character of the active range. The next issue is that we currently position the IME suggestions window immediately *below* this position, which is great for horizontal text -- but not so good for vertical, as it will obscure the rest of the active range. When writing-mode is vertical, we probably want to position IME windows *beside* the first character instead of below.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - Fix ContentEventHandler::OnQueryTextRect for vertical textframes.

This fixes the result of OnQueryTextRect for vertical mode, which gives us more accurate and consistent IME window positioning; however, as noted above, this is not sufficient by itself because we actually want to position the IME window on a different side of the text rect when we're working vertically. So a further patch is still needed here.

Comment 5

[Jonathan Kew [:jfkthame]]

I should also note that the comments above are based on our behavior on OS X; I haven't looked into how the IME window placement is handled on other platforms yet, and whether it depends on ContentEventHandler::OnQueryTextRect in the same way.

Comment 6

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

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> I should also note that the comments above are based on our behavior on OS
> X; I haven't looked into how the IME window placement is handled on other
> platforms yet, and whether it depends on
> ContentEventHandler::OnQueryTextRect in the same way.

Yes. On all platforms, they depend on NS_QUERY_TEXT_RECT event. For deciding to fix this on all platforms, I guess:

1. Add a boolean value to WidgetQueryContentEvent like mInVertialText.
2. Make NS_QUERY_TEXT_RECT set the value of (perhaps) the last character.
3. Make NS_QUERY_CARET_RECT set the value of the caret position.
4. Make NS_QUERY_SELECTED_TEXT set the value of (perhaps) the last character.
5. On some platforms whose IME queries if caret is in virtical context should return the value with NS_QUERY_SELECTED_TEXT.
6. On some platforms who has API to set candidate window position explicitly, use the mInVirtualText at calling the API.

If we will set candidate window position besides at #6, we might need if the text is RTL or LTR too (If it's in horizontal text, the bidi value. If it's in vertical text, tb-rl or tb-lr value).

5: nsTextStore (TSATTRID_Text_VerticalWriting and TSATTRID_Text_VerticalWriting at RequestSupportedAttrs(), RequestAttrsAtPosition(), RequestAttrsTransitioningAtPosition() and RetrieveRequestedAttrs()?), TextInputHandler (NSTextInputClient drawsVerticallyForCharacterAtIndex?) although, we need to confirm each IME behavior, though.
6: nsIMM32Handler, nsGTKIMModule

jfkthame, do you really work on widget for this too?

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - Define a new NS_QUERY_TEXT_IS_VERTICAL event, and implement it in ContentEventHandler.

Here's a patch that implements a new query event to determine whether the text is vertical. Though maybe it would be better to add this to the result of the existing queries, as suggested in comment 6; this was written before seeing that comment.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - Vertical writing mode support for Cocoa IMEs.

And then with this patch, the behavior on OS X seems to be pretty good.

Comment 9

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

Comment on attachment 8500120 [details] [diff] [review]
pt 2 - Define a new NS_QUERY_TEXT_IS_VERTICAL event, and implement it in ContentEventHandler.

It looks reasonable for Mac (and perhaps, TSF on Windows too).

You can make this e10s-aware easy. Now, TabParent caches some information of composition string and composition string is always a text node. I.e., all of a composition string is whether vertical or horizontal. TabParent should store it.

See the patch making NS_QUERY_TEXT_RECT e10s-aware (only composition string range):
https://bugzilla.mozilla.org/attachment.cgi?id=8487852&action=diff

Comment 10

[Jonathan Kew [:jfkthame]]

Thanks for the pointer - I haven't been following the e10s/IME work at all.

Masayuki, would you be able to take over the widget side of this, as I'm sure you are much more familiar with the code and APIs involved on the various platforms?

Comment 11

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

Comment on attachment 8500121 [details] [diff] [review]
pt 3 - Vertical writing mode support for Cocoa IMEs.

How can I test this with Nightly?

writing-mode: vertical-rl hasn't worked yet? (Of course, I enabled vertical-text pref)

Comment 12

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

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> Thanks for the pointer - I haven't been following the e10s/IME work at all.
> 
> Masayuki, would you be able to take over the widget side of this, as I'm
> sure you are much more familiar with the code and APIs involved on the
> various platforms?

Yeah, I'll take it.

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #11)
> Comment on attachment 8500121 [details] [diff] [review]
> pt 3 - Vertical writing mode support for Cocoa IMEs.
> 
> How can I test this with Nightly?
> 
> writing-mode: vertical-rl hasn't worked yet? (Of course, I enabled
> vertical-text pref)

There's a build-time switch that isn't enabled yet on trunk, so in addition to setting the pref, you need a local (or tryserver) build where you #define WRITING_MODE_VERTICAL_ENABLED (in layout/generic/WritingModes.h).

With a build where that's enabled, you should be able to get some simple examples to work, although there's still plenty of layout stuff that will be broken.

Comment 14

[Jonathan Kew [:jfkthame]]

Oh, and for testing this, you'll also want your build to include bug 1074735, which hasn't landed on inbound yet.

Comment 15

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

Thank you for the information. I'll add them in my queue.

Comment 16

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

Comment on attachment 8500121 [details] [diff] [review]
pt 3 - Vertical writing mode support for Cocoa IMEs.

Hmm, I've tried with the patches a couple of times. However, I cannot test this because I cannot create a good testcase for this.

I tested with:
http://jsfiddle.net/y93zdnn1/3/

However, the <input> element is hidden (clipped?) and when I try to set focus and input on the contenteditable editor, my build works too slow. There are a lot of warnings that is Overflowed nscoord_MAX in conversion to nscoord width: file nsRect.h, line 82.

Do you have a good test case for this?

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: simple testcase for trying cocoa ime support

Here's a simple testcase that should be sufficient for experimenting with vertical-text IME support. (It's designed to avoid some of the other current layout bugs that can make vertical elements end up offscreen or clipped, etc., so you can simply try editing a line of vertical text.)

Note that arrow keys within the text will behave unintuitively (they'll be logical rather than physical); that's bug 1077515. But interaction with the IME should work OK with the patches here.

Comment 18

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

Comment on attachment 8500121 [details] [diff] [review]
pt 3 - Vertical writing mode support for Cocoa IMEs.

Sorry for the delay to reply.

This patch works almost fine to me.

However, there are some issues (but it should be fixed later):

1. Both Kotoeri and ATOK move its candidate window too near the first character. I guess that we should expand the rect for preventing a candidate window hiding underline.

2. Google Japanese Input isn't aware to vertical writing applications. Therefore, its candidate window and suggest window hides composing string completely. (Probably, the top-left corner of the first character is the top-left position of each window)

Comment 19

[siqinbilige]

fallow by Comment 13, I build the firefox on windows 7.
1 hg clone https://hg.mozilla.org/mozilla-central
2 #define WRITING_MODE_VERTICAL_ENABLED 1
3 ./mach build
4 ./mach run
but, it is not work with vertical-lr or vertical-rl.
what was wrong with me ?

Comment 20

[Makoto Kato [:m_kato]]

(In reply to siqinbilige from comment #19)
> fallow by Comment 13, I build the firefox on windows 7.
> 1 hg clone https://hg.mozilla.org/mozilla-central
> 2 #define WRITING_MODE_VERTICAL_ENABLED 1
> 3 ./mach build
> 4 ./mach run
> but, it is not work with vertical-lr or vertical-rl.
> what was wrong with me ?

You need layout.css.vertical-text.enabled=true by prefs

Comment 21

[siqinbilige]

(In reply to Makoto Kato (:m_kato) from comment #20)
> You need layout.css.vertical-text.enabled=true by prefs

It works, thank you.

Comment 22

[Jonathan Kew [:jfkthame]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #12)
> (In reply to Jonathan Kew (:jfkthame) from comment #10)
> > Thanks for the pointer - I haven't been following the e10s/IME work at all.
> > 
> > Masayuki, would you be able to take over the widget side of this, as I'm
> > sure you are much more familiar with the code and APIs involved on the
> > various platforms?
> 
> Yeah, I'll take it.

:masayuki, do you have time to work on this at the moment? We're planning to enable vertical writing-modes soon in bug 1099032, and would like to have IME support in place. Thanks!

Comment 23

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

Hmm, unfortunately, no. I'm working on new IME/Keyboard API for JS-IME/Keyboard (especially for Firefox OS)...

Comment 24

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

Although, I don't have much time to hack actually, however, I investigated and filed bugs for Windows and Linux (GTK). Please land the patches posted in this bug. They are necessary for other platforms.

Comment 25

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

And I'm not familiar with IME API of Android...

Comment 26

[Jonathan Kew [:jfkthame]]

Masayuki, thanks for your investigations and bugs filed, that's helpful. I'll try to get the patches here updated as necessary and landed soon.

Comment 27

[Jonathan Kew [:jfkthame]]

Comment on attachment 8500120 [details] [diff] [review]
pt 2 - Define a new NS_QUERY_TEXT_IS_VERTICAL event, and implement it in ContentEventHandler.

I think we can manage without a new NS_QUERY_TEXT_IS_VERTICAL event, if we just add the writing mode to the reply from the existing NS_QUERY_TEXT_RECT (and maybe to other events, if needed by other platforms).

Comment 28

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - Add writing-mode to the reply to NS_QUERY_TEXT_RECT event

There's already a writing-mode field in the reply record (added by bug 1077515 for SELECTED_TEXT), so we can use it when handling the TEXT_RECT query as well.

Comment 29

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - Support drawsVerticallyForCharacterAtIndex method in Cocoa childView and IME input handler

Then this seems to be sufficient to get vertical-IME support on OS X. It relies on the fact that the IME code queries for the text rect before checking drawsVertically, so we can just use the cached writing-mode value there.

Comment 30

[Jonathan Kew [:jfkthame]]

Comment on attachment 8500121 [details] [diff] [review]
pt 3 - Vertical writing mode support for Cocoa IMEs.

Obsoleting older version that relied on a new query event.

Comment 31

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

Comment on attachment 8561409 [details] [diff] [review]
pt 2 - Add writing-mode to the reply to NS_QUERY_TEXT_RECT event

>diff --git a/dom/events/ContentEventHandler.cpp b/dom/events/ContentEventHandler.cpp
>--- a/dom/events/ContentEventHandler.cpp
>+++ b/dom/events/ContentEventHandler.cpp
>@@ -998,16 +998,17 @@ ContentEventHandler::OnQueryTextRect(Wid
> 
>   if (firstFrame == lastFrame) {
>     rect.IntersectRect(rect, frameRect);
>   } else {
>     rect.UnionRect(rect, frameRect);
>   }
>   aEvent->mReply.mRect = LayoutDevicePixel::FromUntyped(
>       rect.ToOutsidePixels(mPresContext->AppUnitsPerDevPixel()));
>+  aEvent->mReply.mWritingMode = lastFrame->GetWritingMode();
>   aEvent->mSucceeded = true;
>   return NS_OK;
> }

I feel odd using the last frame. However, if selection crosses the boundary of writing mode and replaced by inputting character causes that the new writing mode is larger offset's direction...
http://jsfiddle.net/d_toybox/z2j28uw5/

Comment 32

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

Comment on attachment 8561411 [details] [diff] [review]
pt 3 - Support drawsVerticallyForCharacterAtIndex method in Cocoa childView and IME input handler

>+   * NOTE: the current implementation ignores the character offset, and instead
>+   * returns a cached value from the most recent FirstRectForCharacterRange()
>+   * or SelectedRange() call. This appears to work because one of those methods
>+   * is always called prior to the drawsVertically query, and makes this a
>+   * lightweight method that does not potentially need to forward a message to
>+   * the parent process, but if the usage pattern ever changes we might need
>+   * to revisit this.
>+  bool DrawsVerticallyForCharacterAtIndex(uint32_t aCharIndex);

I don't think that this is right thing. If we behave so, any IMEs which tests us needs to call firstRectForCharacterRange. And also some third party's IME may not work as so already.

I think that the method should cache the writing mode value at selection. When selection is changed, the cached value should be marked dirty. I.e., if it should query selected text only when the cache is not valid or the index is out of selection. The latter case must be very rare case, so, we don't need to worry the runtime cost.

Comment 33

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - Support drawsVerticallyForCharacterAtIndex method in Cocoa childView and IME input handler

Here's a version that handles the index to drawsVerticallyForCharacter... by keeping track of the range when it's set by QUERY_SELECTED_TEXT or QUERY_TEXT_RECT. If the drawsVertically query does NOT correspond to the cached value, we'll call QUERY_TEXT_RECT afresh to update it. With Apple's IMEs, that never seems to happen (the NS_WARNING never fires), but if some third-party IME works differently this should handle it OK.

Comment 34

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

Comment on attachment 8562028 [details] [diff] [review]
pt 3 - Support drawsVerticallyForCharacterAtIndex method in Cocoa childView and IME input handler

Almost okay to me.

However, please do following in IMEInputHandler::OnSelectionChange():

mRangeForWritingMode.location = NSNotFound;

And if mRangeForWritingMode.location is NSNotFound in IMEInputHandler::DrawsVerticallyForCharacterAtIndex(), please call IMEInputHandler::SelectedRange(). Then, mRangeForWritingMode is updated with the latest information.

I'd like to check the next patch. Anyway, thanks a lot for your work.

Comment 35

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - Support drawsVerticallyForCharacterAtIndex method in Cocoa childView and IME input handler

Thanks for your comments. Updated patch as suggested.

Comment 36

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

Comment on attachment 8562096 [details] [diff] [review]
pt 3 - Support drawsVerticallyForCharacterAtIndex method in Cocoa childView and IME input handler

Thanks!!

Comment 37

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc68af1ab0bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/16e9a0ec959f
https://hg.mozilla.org/integration/mozilla-inbound/rev/adb7e9606576

Comment 38

[Jonathan Kew [:jfkthame]]

Narrowing the scope of this bug's summary to OS X only, to match the patches actually landed here; the IME issues for other platforms will be separate bugs that also block bug 789103.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fc68af1ab0bf
https://hg.mozilla.org/mozilla-central/rev/16e9a0ec959f
https://hg.mozilla.org/mozilla-central/rev/adb7e9606576