1130936 - [TSF] Support vertical writing-mode

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

We're are returning false from nsTextStore::RetrieveRequestedAttrs(). We should retrieve actual value for it.
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsTextStore.cpp#2983

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Support vertical writing mode in nsTextStore for Windows TSF

For TSF, this appears to be sufficient to get vertical mode working in my (minimal) testing with the default Japanese IME on Windows 8. I haven't attempted to test with alternate IMEs ... I don't have any idea what's typically preferred by Japanese users. But this should provide a starting point, at least.

Comment 2

[Jonathan Kew [:jfkthame]]

Oh, the preceding patch depends on parts 1 and 2 from bug 1076657 also being landed, btw.

Try build available at https://treeherder.mozilla.org/#/jobs?repo=try&revision=95e4a3778782.

Comment 3

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

Comment on attachment 8561722 [details] [diff] [review]
Support vertical writing mode in nsTextStore for Windows TSF

Almost okay to me.

However, mWritingMode isn't good. It depends on the last offset of GetTextExt() which is called by TIP (a.k.a IME).

I think that nsTextStore::Selection should store:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsTextStore.h#360

Then, nsTextStore::CurrentSelection() should initialize it:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsTextStore.cpp#1881

When you need to retrieve the information, you can do it with:

Selection& currentSelection = CurrentSelection();
return currentSelection.WritingMode().IsVertical() ? foo : bar;

Note that, CurrentSelection() caches its data until text or selection changed in the focused editor. Therefore, even if it's necessary a lot of times, it may retrieve only once.

Some random thoughts (not scope of this bug):
* We might need a notification which changes the style (including writing-mode) for marking mSelection dirty.
* The candidate window of TIPs is positioned nearer than IE. And it's too near to me because the shadow of the window overlaps the text. I guess that we need some hack for GetTextExt().

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Support vertical writing mode in nsTextStore for Windows TSF

Comment 5

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

Comment on attachment 8562102 [details] [diff] [review]
Support vertical writing mode in nsTextStore for Windows TSF

>@@ -4888,18 +4904,20 @@ nsTextStore::Content::StartComposition(I
>   MOZ_ASSERT(aCompositionView);
>   MOZ_ASSERT(!mComposition.mView);
>   MOZ_ASSERT(aCompStart.mType == PendingAction::COMPOSITION_START);
> 
>   mComposition.Start(aCompositionView, aCompStart.mSelectionStart,
>     GetSubstring(static_cast<uint32_t>(aCompStart.mSelectionStart),
>                  static_cast<uint32_t>(aCompStart.mSelectionLength)));
>   if (!aPreserveSelection) {
>+    // XXX Do we need to set a new writing-mode here when setting a new
>+    // selection? Currently, we just preserve the existing value.
>     mSelection.SetSelection(mComposition.mStart, mComposition.mString.Length(),
>-                            false);
>+                            false, mSelection.GetWritingMode());

Ideally, your worry is correct. But as far as I know, there is no TIPs which sets selection to far point from current selection. So, this must not be a problem at least for now. When I have much time for this, I'll file and fix the bug.

>     void CollapseAt(uint32_t aOffset)
>     {
>+      // XXX This does not update the selection's mWritingMode.
>+      // If it is ever used to "collapse" to an entirely new location,
>+      // we may need to fix that.
>       mDirty = false;
>       mACP.acpStart = mACP.acpEnd = static_cast<LONG>(aOffset);
>       mACP.style.ase = TS_AE_END;
>       mACP.style.fInterimChar = FALSE;
>     }

Sure. But it must be rare case. I'll file and fix this too when I have much time.

Thank you for your great work!

Comment 6

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c54ce201030c

This should give the basic functionality; we may want to fine-tune candidate window positioning, etc., in followups, but at least it's usable.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c54ce201030c