Closed Bug 1076657 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: jtd, Assigned: jfkthame)

References

(Blocks 2 open bugs)

Details

(Keywords: inputmethod)

Attachments

(4 files, 4 obsolete files)

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

中野さん、よろしく!
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...
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.
Depends on: 1074735
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.
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.
Attachment #8499999 - Flags: review?(smontagu)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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.
(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?
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.
Attachment #8500120 - Flags: feedback?(masayuki)
And then with this patch, the behavior on OS X seems to be pretty good.
Attachment #8500121 - Flags: feedback?(masayuki)
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
Attachment #8500120 - Flags: feedback?(masayuki)
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?
Flags: needinfo?(masayuki)
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)
Flags: needinfo?(masayuki)
(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.
(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.
Oh, and for testing this, you'll also want your build to include bug 1074735, which hasn't landed on inbound yet.
Thank you for the information. I'll add them in my queue.
Attachment #8499999 - Flags: review?(smontagu) → review+
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?
Flags: needinfo?(jfkthame)
Attachment #8500121 - Flags: feedback?(masayuki)
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.
Flags: needinfo?(jfkthame)
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)
Attachment #8500121 - Flags: feedback+
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 ?
(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
(In reply to Makoto Kato (:m_kato) from comment #20)
> You need layout.css.vertical-text.enabled=true by prefs

It works, thank you.
(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!
Flags: needinfo?(masayuki)
Hmm, unfortunately, no. I'm working on new IME/Keyboard API for JS-IME/Keyboard (especially for Firefox OS)...
Flags: needinfo?(masayuki)
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.
And I'm not familiar with IME API of Android...
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 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).
Attachment #8500120 - Attachment is obsolete: true
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.
Attachment #8561409 - Flags: review?(masayuki)
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.
Attachment #8561411 - Flags: review?(masayuki)
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.
Attachment #8500121 - Attachment is obsolete: true
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/
Attachment #8561409 - Flags: review?(masayuki) → review+
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.
Attachment #8561411 - Flags: review?(masayuki) → review-
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.
Attachment #8562028 - Flags: review?(masayuki)
Attachment #8561411 - Attachment is obsolete: true
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.
Attachment #8562028 - Flags: review?(masayuki) → review-
Thanks for your comments. Updated patch as suggested.
Attachment #8562096 - Flags: review?(masayuki)
Attachment #8562028 - Attachment is obsolete: true
Comment on attachment 8562096 [details] [diff] [review]
pt 3 - Support drawsVerticallyForCharacterAtIndex method in Cocoa childView and IME input handler

Thanks!!
Attachment #8562096 - Flags: review?(masayuki) → review+
No longer depends on: 1130936
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.
No longer depends on: 1130935, 1130937
Summary: need to support input method handling within vertical text → [OS X] need to support input method handling within vertical text
Depends on: 1140939
No longer depends on: 1140939
You need to log in before you can comment on or make changes to this bug.