[TSF] Support vertical writing-mode

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: masayuki, Assigned: jfkthame)

Tracking

(Blocks 2 bugs, {inputmethod})

Trunk
mozilla38
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
Assignee

Comment 1

4 years ago
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.
Attachment #8561722 - Flags: review?(masayuki)
Assignee

Updated

4 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee

Comment 2

4 years ago
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 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().
Attachment #8561722 - Flags: review?(masayuki) → review-
Assignee

Updated

4 years ago
Attachment #8561722 - Attachment is obsolete: true
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!
Attachment #8562102 - Flags: review?(masayuki) → review+
Assignee

Comment 6

4 years ago
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.
No longer blocks: 1076657
Target Milestone: --- → mozilla38
https://hg.mozilla.org/mozilla-central/rev/c54ce201030c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.