Closed Bug 1130935 Opened 9 years ago Closed 9 years ago

[IMM] Support vertical writing mode

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: inputmethod)

Attachments

(11 files, 3 obsolete files)

8.36 KB, patch
emk
: review+
Details | Diff | Splinter Review
8.60 KB, patch
emk
: review+
smaug
: review+
Details | Diff | Splinter Review
7.48 KB, patch
emk
: review+
Details | Diff | Splinter Review
5.28 KB, patch
emk
: review+
Details | Diff | Splinter Review
3.11 KB, patch
emk
: review+
Details | Diff | Splinter Review
5.07 KB, patch
emk
: review+
Details | Diff | Splinter Review
11.86 KB, patch
emk
: review+
Details | Diff | Splinter Review
14.16 KB, patch
smaug
: review+
Details | Diff | Splinter Review
13.47 KB, patch
emk
: review+
Details | Diff | Splinter Review
5.59 KB, patch
emk
: review+
Details | Diff | Splinter Review
812 bytes, patch
smontagu
: review+
Details | Diff | Splinter Review
It seems that in IMM, we need to set vertical writing font whose names are starting with "@" to composition string.

See ImmSetCompositionFont function of MSDN:
https://msdn.microsoft.com/en-us/library/windows/desktop/dd318579%28v=vs.85%29.aspx

> Remarks
> 
> This function causes a IMN_SETCOMPOSITIONFONT command to be sent to an application.
> Even if the application never uses the composition window, it must set the
> appropriate font to ensure that characters are displayed properly. This is
> especially true for vertical writing.
We are intentionally banning fonts whose name is starting with "@".
https://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxGDIFontList.cpp?rev=e5edf734c837&mark=691-693#684
Moreover, it will only work with GDI backend.
Absolutely, in gfx, but cannot we create dummy GDI context only for the purpose? I guess that specifying "@" font names directly is enough for this.

# Even though, we will enable TSF mode, we still need to support IMM-IME...
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #2)
> Absolutely, in gfx, but cannot we create dummy GDI context only for the
> purpose? I guess that specifying "@" font names directly is enough for this.

I don't know because I don't know why MSDN says "Even if the application never uses the composition window, it must set the appropriate font to ensure that characters are displayed properly". Currently we never call ImmSetCompositionFont. Why is this needed to enable writing-mode? What problem about IMM IME do you try to solve?
(In reply to Masatoshi Kimura [:emk] from comment #3)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #2)
> > Absolutely, in gfx, but cannot we create dummy GDI context only for the
> > purpose? I guess that specifying "@" font names directly is enough for this.
> 
> I don't know because I don't know why MSDN says "Even if the application
> never uses the composition window, it must set the appropriate font to
> ensure that characters are displayed properly". Currently we never call
> ImmSetCompositionFont. Why is this needed to enable writing-mode? What
> problem about IMM IME do you try to solve?

If we don't specify "@" fonts (if it's the only cause), candidate window overlaps composition string and the contents of candidate list window is horizontal. (i.e., items are listed top to bottom, not left to right)
OK, I read FAKEIME sample and got it.

FAKEIME checks if lfEscapement == 2700 to determine the UI layout. It will affect not only composition string but also candidate window, etc. It was not obvious at all from the MSDN doc...

It will fundamentally depend on the IME implementation, but we could expect that other IMEs will follow FAKEIME's hack.
(In reply to Masatoshi Kimura [:emk] from comment #5)
> OK, I read FAKEIME sample and got it.
> 
> FAKEIME checks if lfEscapement == 2700 to determine the UI layout. It will
> affect not only composition string but also candidate window, etc. It was
> not obvious at all from the MSDN doc...

Yeah, I'm not surprised that turns out to be the key to getting vertical layout: I found the same thing with TSF in bug 1130936, it's necessary to return 2700 for TSATTRID_Text_Orientation. Just returning TRUE for TSATTRID_Text_VerticalWriting resulted in sideways-rotated glyphs laid out horizontally. :) Probably using an @-named font without setting lfEscapement would do the same thing...
No longer blocks: 1076657
No longer blocks: enable-writing-mode-dev
FYI:

Starting Mozilla 40, TSF mode is enabled in default settings even in release builds. So, for most users, this is not related directly because most Japanese IME are already implemented as TIP (Text Input Processor) for TSF. However, some IMEs are still implemented as IME for IMM. E.g., Japanist 2003 is not so major but I confirmed that some users still use it since it only supports Japanese special keyboard layout, Oyayubi-Shift.
> since it only supports Japanese special keyboard layout, Oyayubi-Shift.

Oops, I meant "since it's the only IME supporting Japanese special keyboard layout, Oyayubi-Shift.
Temporarily, we should just adjust candidate window position for now. We should put off to fix what we specify @ font to IME context.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Oh, I misremembered ImmSetCompositionFont(). Its argument includes LOGFONT, i.e., not handle to a font. So, I guess I can fix it easy.
First of all, we need a method to check active IME property which indicates if the IME supports vertical writing mode.

The pref is mainly useful for debug, but it's usable for some IME which lies to us. (Basically, we should make such IME in blacklist, but it's better to fix the bug with this workaround by user self.)
Attachment #8600333 - Flags: review?(VYV03354)
Next, set the composition font for enabling/disabling vertical UI at receiving WM_STARTCOMPOSITION. When a windowless plugin gets focus, we need to assume that proper composition font is a horizontal font (current behavior).

This patch can specify composition fonts by prefs. This may be useful for debug if we'll have some reports of vertical UI of specific IME broken. (See next patch)

AdjustCompositionFont() doesn't update if neither the writing mode nor active IME hasn't been changed.
Attachment #8600336 - Flags: review?(VYV03354)
In my environments, ::ImmGetCompositionFont() returns "System". And all Japanese IMEs except Japanist 2003 works with "@System" in vertical writing mode.

The candidate window and suggest window of Japanist 2003 are broken by specifying "@System". With specifying "@MS PGothic", we can fix it. So, we should add this workaround for Japanist 2003 users.

Note that Japanist 2003 is IME (not TIP), so, even in TSF mode, this is handled by nsIMM32Handler. And this supports Oyayubi-Shift keyboard layout. So, this is important IME for some users.
Attachment #8600338 - Flags: review?(VYV03354)
At setting IME candidate window position with ::ImmSetCandidateWindow(), we need the writing mode information at composition string.

So, GetCharacterRectOfSelectedTextAt() and GetCaretRect() of nsIMM32Handler should return writing mode if the caller needs it.

Then, NS_QUERY_CARET_RECT should return writing mode information from ContentEventHandler too.
Attachment #8600340 - Flags: review?(bugs)
Attachment #8600340 - Flags: review?(VYV03354)
nsIMM32Handler::SetIMERelatedWindowsPos() should treat writing mode.

First, candidate window shouldn't overlap with target clause. Therefore, we should include all rects to rcArea of CANDIDATEFORM for excluding the area.

The reasons why candidate window should be position are explained by each comment in the patch, please check it.
Attachment #8600341 - Flags: review?(VYV03354)
Some IMEs need to be notified current writing mode even if there is no composition since they may show suggest window or some hint window. So, nsIMM32Handler should notify IME of writing mode change at every selection change.

So, IMENotification should notify widget of selection change with writing mode (and selection range for improving performance in PuppetWidget).

I think that we should optimize the performance of querying selection range and writing mode and all native IME handlers should cache the selection range if they sometimes need it for reducing runtime cost. But it's out of scope of this bug, though.
Attachment #8600345 - Flags: review?(bugs)
Adjust composition font at every selection change. This fixes NaviBar position of ATOK when different writing mode editor gets focus.
Attachment #8600347 - Flags: review?(VYV03354)
Google Japanese Input doesn't support vertical writing mode. Therefore, it behaves very odd with vertical text.

In such case, Google Japanese Input may move its owning windows to the edge of editor rect. However, we don't return correct editor rect at receiving IMR_QUERYCHARPOSITION. For making "better" odd behavior, we should set correct editor rect.

Then, the windows are moved from the focused window edge to the focused editor edge.
Attachment #8600350 - Flags: review?(VYV03354)
Google Japanese Input doesn't support vertical writing mode UI with candidate window, etc. However, it claims that it supports vertical writing mode because only its composition window supports vertical writing mode for usual applications which don't draw composition string themselves.

So, we need to judge that Google Japanese Input doesn't support vertical writing mode.

If some users needed to use IMM mode even on Win8 or later, we would provide this hack for them. However, unfortunately, Google Japanese Input is installed as TIP on Win8 or later. Then, we cannot distinguish which TIP is active in CUAS only with IMM API. I believe that most users use only one IME in daily use. So, I think that providing a way to override TIP name with specific string is useful for such users even if they must be minority.
Attachment #8600352 - Flags: review?(VYV03354)
I found a bug of the preceding patches when switching IME and starting composition without selection change.

Even if writing mode isn't changed and just IME is changed, we should forcibly update the composition font because some IME may need specific font or not support vertical writing mode.
Attachment #8600358 - Flags: review?(VYV03354)
That's all.

I tested with WinXP, Win7 and Win8.1. I don't test well with non-Japanese IMEs. However, non-Japanese IMEs don't support vertical writing mode UI, but supports for deciding its position. So, I found problems only with Japanese IMEs.

* Japanist 2003 - candidate window is broken in vertical writing mode if "@System" is specified
* ATOK 2011 - 2015 - candidate window position of horizontal mode immediately after focus is moved from vertical writing mode is wrong, but I couldn't find any workaround for it. In TSF mode, ATOK works perfectly and on XP, ATOK isn't TIP. So, this bug affects only very minor users.
* Google Japanese Input - as I said above, this doesn't support vertical writing mode. And also doesn't support CSF_EXCLUDE of CANDIDATEFORM at calling ::ImmSetCandidateWindow(). Therefore, it needs a lot of hack. With the patches, its windows are positioned under the first character of target clause. This is the "best" behavior of its odd behaviors which I've seen.
Comment on attachment 8600333 [details] [diff] [review]
part.1 Add a method to check if current IME supports vertical writing mode

Review of attachment 8600333 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsIMM32Handler.cpp
@@ +17,5 @@
>  #include "mozilla/TextEvents.h"
>  
> +#ifndef IME_PROP_ACCEPT_WIDE_VKEY
> +#define IME_PROP_ACCEPT_WIDE_VKEY 0x20
> +#endif

Why is this necessary?

@@ +140,5 @@
>      sWM_MSIME_MOUSE = ::RegisterWindowMessage(RWM_MOUSE);
>    }
> +  sAssumeVerticalWritingModeNotSupported =
> +    Preferences::GetBool(
> +      "intl.imm.vertical_writing.always_assume_not_supported", false);

Please use Preferences::AddBoolVarCache.
Attachment #8600333 - Flags: review?(VYV03354) → review+
Comment on attachment 8600336 [details] [diff] [review]
part.2 Set proper composition font when writing mode is changed

Review of attachment 8600336 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpref/init/all.js
@@ +3053,5 @@
> +// The font name for vertical writing mode must start with '@'.  On the other
> +// hand, the font name for horizontal writing mode must not start with '@'.
> +// FYI: Changing these prefs requires to restart.
> +pref("intl.imm.vertical_writing.composition_font", "");
> +pref("intl.imm.horizontal_writing.composition_font", "");

Is this pref needed? That is, do you know any IME broken in horizontal layout?
Comment on attachment 8600338 [details] [diff] [review]
part.3 Add hack for Japanist because its candidate window is broken with @System font

Review of attachment 8600338 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsIMM32Handler.cpp
@@ +2251,5 @@
> +      }
> +      memcpy(logFont.lfFaceName, sVerticalFontForJapanist.get(),
> +             sVerticalFontForJapanist.Length() * sizeof(wchar_t));
> +    } else {
> +      memcpy(logFont.lfFaceName, sCompositionFontV.get(), 

nit: extra trailing space
Attachment #8600338 - Flags: review?(VYV03354) → review+
Comment on attachment 8600340 [details] [diff] [review]
part.4 nsIMM32Handler::GetCharacterRectOfSelectedTextAt() should return wrting mode if it's necessary

Review of attachment 8600340 [details] [diff] [review]:
-----------------------------------------------------------------

I only reviewed widget/windows.
Attachment #8600340 - Flags: review?(VYV03354) → review+
Comment on attachment 8600341 [details] [diff] [review]
part.5 nsIMM32Handler should compute candidate window position with writing mode

Review of attachment 8600341 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsIMM32Handler.cpp
@@ +2117,5 @@
> +      // the position must be the safest position to prevent the candidate
> +      // window to overlap with the target clause.
> +      candForm.dwStyle = CFS_CANDIDATEPOS;
> +      candForm.ptCurrentPos.x = targetClauseRect.x;
> +      candForm.ptCurrentPos.y = targetClauseRect.YMost();

candForm.rcArea is not set here. Is this OK?
Attachment #8600347 - Flags: review?(VYV03354) → review+
Attachment #8600350 - Flags: review?(VYV03354) → review+
Attachment #8600352 - Flags: review?(VYV03354) → review+
Attachment #8600358 - Flags: review?(VYV03354) → review+
Thank you, Kimura-san!

(In reply to Masatoshi Kimura [:emk] from comment #22)
> Comment on attachment 8600333 [details] [diff] [review]
> part.1 Add a method to check if current IME supports vertical writing mode
> 
> Review of attachment 8600333 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsIMM32Handler.cpp
> @@ +17,5 @@
> >  #include "mozilla/TextEvents.h"
> >  
> > +#ifndef IME_PROP_ACCEPT_WIDE_VKEY
> > +#define IME_PROP_ACCEPT_WIDE_VKEY 0x20
> > +#endif
> 
> Why is this necessary?

When I build the patch with VS2013, I hit this...

> > +  sAssumeVerticalWritingModeNotSupported =
> > +    Preferences::GetBool(
> > +      "intl.imm.vertical_writing.always_assume_not_supported", false);
> 
> Please use Preferences::AddBoolVarCache.

Do you think that this should be modified dynamically? Using varcache increasing the size of hashtable. So, I don't think that it's worthwhile to use it with this pref. How about you?

(In reply to Masatoshi Kimura [:emk] from comment #23)
> > +// The font name for vertical writing mode must start with '@'.  On the other
> > +// hand, the font name for horizontal writing mode must not start with '@'.
> > +// FYI: Changing these prefs requires to restart.
> > +pref("intl.imm.vertical_writing.composition_font", "");
> > +pref("intl.imm.horizontal_writing.composition_font", "");
> 
> Is this pref needed? That is, do you know any IME broken in horizontal
> layout?

Ah, good point. But if user customizes the vertical writing mode's composition font, isn't he/she wants to customize the horizontal font too for consistency?

(In reply to Masatoshi Kimura [:emk] from comment #26)
> > +      // the position must be the safest position to prevent the candidate
> > +      // window to overlap with the target clause.
> > +      candForm.dwStyle = CFS_CANDIDATEPOS;
> > +      candForm.ptCurrentPos.x = targetClauseRect.x;
> > +      candForm.ptCurrentPos.y = targetClauseRect.YMost();
> 
> candForm.rcArea is not set here. Is this OK?

Yes. https://msdn.microsoft.com/ja-jp/dd317738?f=255&MSPPError=-2147217396
When CFS_CANDIDATEPOS is set, rcArea isn't used.
Flags: needinfo?(VYV03354)
Attachment #8600340 - Flags: review?(bugs) → review+
Comment on attachment 8600345 [details] [diff] [review]
part.6 Selection change notification should have selection range and writing mode information

># HG changeset patch
># User Masayuki Nakano <masayuki@d-toybox.com>
># Parent  8177b1005fe7bb683ee6c809edfa8eb1cb392950
>Bug 1130935 part.6 Selection change notification should have selection range and writing mode information r=
>
>diff --git a/dom/events/IMEContentObserver.cpp b/dom/events/IMEContentObserver.cpp
>--- a/dom/events/IMEContentObserver.cpp
>+++ b/dom/events/IMEContentObserver.cpp
>@@ -27,16 +27,17 @@
> #include "nsIPresShell.h"
> #include "nsISelectionController.h"
> #include "nsISelectionPrivate.h"
> #include "nsISupports.h"
> #include "nsIWidget.h"
> #include "nsPresContext.h"
> #include "nsThreadUtils.h"
> #include "nsWeakReference.h"
>+#include "WritingModes.h"
> 
> namespace mozilla {
> 
> using namespace widget;
> 
> NS_IMPL_CYCLE_COLLECTION_CLASS(IMEContentObserver)
> 
> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(IMEContentObserver)
>@@ -242,16 +243,22 @@ IMEContentObserver::UnregisterObservers(
>   }
> 
>   if (mUpdatePreference.WantPositionChanged() && mDocShell) {
>     mDocShell->RemoveWeakScrollObserver(this);
>     mDocShell->RemoveWeakReflowObserver(this);
>   }
> }
> 
>+nsPresContext*
>+IMEContentObserver::GetPresContext() const
>+{
>+  return mESM ? mESM->GetPresContext() : nullptr;
>+}
>+
> void
> IMEContentObserver::Destroy()
> {
>   // WARNING: When you change this method, you have to check Unlink() too.
> 
>   UnregisterObservers(false);
> 
>   mEditor = nullptr;
>@@ -326,22 +333,49 @@ public:
>     : mDispatcher(aDispatcher)
>     , mCausedByComposition(aCausedByComposition)
>   {
>     MOZ_ASSERT(mDispatcher);
>   }
> 
>   NS_IMETHOD Run()
>   {
>-    if (mDispatcher->GetWidget()) {
>-      IMENotification notification(NOTIFY_IME_OF_SELECTION_CHANGE);
>-      notification.mSelectionChangeData.mCausedByComposition =
>-         mCausedByComposition;
>-      mDispatcher->GetWidget()->NotifyIME(notification);
>+    nsCOMPtr<nsIWidget> widget = mDispatcher->GetWidget();
>+    nsPresContext* presContext = mDispatcher->GetPresContext();
>+    if (!widget || !presContext) {
>+      return NS_OK;
>     }
>+
>+    // XXX Cannot we cache some information for reducing the cost to compute
>+    //     selection offset and writing mode?
>+    WidgetQueryContentEvent selection(true, NS_QUERY_SELECTED_TEXT, widget);
>+    ContentEventHandler handler(presContext);
>+    handler.OnQuerySelectedText(&selection);
>+    if (NS_WARN_IF(!selection.mSucceeded)) {
>+      return NS_OK;
>+    }
>+
>+    // The widget might be destroyed during querying the content since it
>+    // causes flushing layout.
>+    widget = mDispatcher->GetWidget();
Why you need to re-get the widget? You're keeping a strong ref to it.
>+  // IMENotification cannot store this class directly since this has some
>+  // constructors.  Therefore, it stores mWritingMode and recreate the
recreates

>   static void Write(Message* aMsg, const paramType& aParam)
>   {
>     WriteParam(aMsg,
>       static_cast<mozilla::widget::IMEMessageType>(aParam.mMessage));
>     switch (aParam.mMessage) {
>       case mozilla::widget::NOTIFY_IME_OF_SELECTION_CHANGE:
>+        WriteParam(aMsg, aParam.mSelectionChangeData.mOffset);
>+        WriteParam(aMsg, aParam.mSelectionChangeData.mLength);
>+        WriteParam(aMsg, aParam.mSelectionChangeData.mWritingMode);
>+        WriteParam(aMsg, aParam.mSelectionChangeData.mReversed);
Here the order is offset, length, writingMode, reversed


>+                         &aResult->mSelectionChangeData.mOffset) &&
>+               ReadParam(aMsg, aIter,
>+                         &aResult->mSelectionChangeData.mLength) &&
>+               ReadParam(aMsg, aIter,
>+                         &aResult->mSelectionChangeData.mReversed) &&
>+               ReadParam(aMsg, aIter,
>+                         &aResult->mSelectionChangeData.mWritingMode) &&

But here offset, length, reversed, writingMode
Attachment #8600345 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #28)
> Comment on attachment 8600345 [details] [diff] [review]
> part.6 Selection change notification should have selection range and writing
> mode information
> >   NS_IMETHOD Run()
> >   {
> >-    if (mDispatcher->GetWidget()) {
> >-      IMENotification notification(NOTIFY_IME_OF_SELECTION_CHANGE);
> >-      notification.mSelectionChangeData.mCausedByComposition =
> >-         mCausedByComposition;
> >-      mDispatcher->GetWidget()->NotifyIME(notification);
> >+    nsCOMPtr<nsIWidget> widget = mDispatcher->GetWidget();
> >+    nsPresContext* presContext = mDispatcher->GetPresContext();
> >+    if (!widget || !presContext) {
> >+      return NS_OK;
> >     }
> >+
> >+    // XXX Cannot we cache some information for reducing the cost to compute
> >+    //     selection offset and writing mode?
> >+    WidgetQueryContentEvent selection(true, NS_QUERY_SELECTED_TEXT, widget);
> >+    ContentEventHandler handler(presContext);
> >+    handler.OnQuerySelectedText(&selection);
> >+    if (NS_WARN_IF(!selection.mSucceeded)) {
> >+      return NS_OK;
> >+    }
> >+
> >+    // The widget might be destroyed during querying the content since it
> >+    // causes flushing layout.
> >+    widget = mDispatcher->GetWidget();
> Why you need to re-get the widget? You're keeping a strong ref to it.

Because the dispatcher (IMEContentObserver) may lose focus during dispatching NS_QUERY_SELECTED_TEXT which may cause flushing pending layout. If the focused editor already lost focus, the selection change notification shouldn't be notified to IME because IME may be focused in another editor.

> >   static void Write(Message* aMsg, const paramType& aParam)
> >   {
> >     WriteParam(aMsg,
> >       static_cast<mozilla::widget::IMEMessageType>(aParam.mMessage));
> >     switch (aParam.mMessage) {
> >       case mozilla::widget::NOTIFY_IME_OF_SELECTION_CHANGE:
> >+        WriteParam(aMsg, aParam.mSelectionChangeData.mOffset);
> >+        WriteParam(aMsg, aParam.mSelectionChangeData.mLength);
> >+        WriteParam(aMsg, aParam.mSelectionChangeData.mWritingMode);
> >+        WriteParam(aMsg, aParam.mSelectionChangeData.mReversed);
> Here the order is offset, length, writingMode, reversed
> 
> 
> >+                         &aResult->mSelectionChangeData.mOffset) &&
> >+               ReadParam(aMsg, aIter,
> >+                         &aResult->mSelectionChangeData.mLength) &&
> >+               ReadParam(aMsg, aIter,
> >+                         &aResult->mSelectionChangeData.mReversed) &&
> >+               ReadParam(aMsg, aIter,
> >+                         &aResult->mSelectionChangeData.mWritingMode) &&
> 
> But here offset, length, reversed, writingMode

Good point! Thanks.
Flags: needinfo?(bugs)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 5/2 - 5/6) from comment #27)
> > Why is this necessary?
> 
> When I build the patch with VS2013, I hit this...

OK.
 
> > Please use Preferences::AddBoolVarCache.
> 
> Do you think that this should be modified dynamically? Using varcache
> increasing the size of hashtable. So, I don't think that it's worthwhile to
> use it with this pref. How about you?

I would like to make prefs "live" as much as possible, but up to you.

> > Is this pref needed? That is, do you know any IME broken in horizontal
> > layout?
> 
> Ah, good point. But if user customizes the vertical writing mode's
> composition font, isn't he/she wants to customize the horizontal font too
> for consistency?

Then, is the separate pref needed?

> > candForm.rcArea is not set here. Is this OK?
> 
> Yes. https://msdn.microsoft.com/ja-jp/dd317738?f=255&MSPPError=-2147217396
> When CFS_CANDIDATEPOS is set, rcArea isn't used.

OK.
Flags: needinfo?(VYV03354)
Attachment #8600341 - Flags: review?(VYV03354) → review+
Flags: needinfo?(bugs)
Attachment #8602657 - Flags: review?(bugs) → review+
(In reply to Masatoshi Kimura [:emk] from comment #31)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 5/2 -
> 5/6) from comment #27)
> > > Please use Preferences::AddBoolVarCache.
> > 
> > Do you think that this should be modified dynamically? Using varcache
> > increasing the size of hashtable. So, I don't think that it's worthwhile to
> > use it with this pref. How about you?
> 
> I would like to make prefs "live" as much as possible, but up to you.

Okay, I'll keep not using varcache for this. If somebody will complain about this behavior, I don't mind to change it.

> > > Is this pref needed? That is, do you know any IME broken in horizontal
> > > layout?
> > 
> > Ah, good point. But if user customizes the vertical writing mode's
> > composition font, isn't he/she wants to customize the horizontal font too
> > for consistency?
> 
> Then, is the separate pref needed?

Okay, I'll recreate it. And I'll rewrite the Japanist hack too.
Comment on attachment 8602657 [details] [diff] [review]
part.6 Selection change notification should have selection range and writing mode information

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbbd9f8c7b7c

Hmm, this patch causes new orange of:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_tabopen_reflows.js?mark=139-139#118
http://mxr.mozilla.org/mozilla-central/source/layout/generic/test/test_backspace_delete.xul?mark=242-243#231

The cause must be forcibly flushing layout at selection change. I'm not sure the direct causes of them. Although, I guess the latter case detects another bug around the bidi delete/backspace command handling.

Anyway, it's safer to delay to run selection change notification after flushing pending layout if there is. Do you know how to do that? The points are:

* How to check if there may be pending layout.
* How to catch when the pending layout is flushed.
Flags: needinfo?(bugs)
Attachment #8602657 - Flags: review-
Let's use same font face to customizing the composition font for both writing modes.
Attachment #8600336 - Attachment is obsolete: true
Attachment #8600336 - Flags: review?(VYV03354)
Attachment #8603729 - Flags: review?(VYV03354)
Same for the hack of Japanist 2003 too.
Attachment #8600338 - Attachment is obsolete: true
Attachment #8603730 - Flags: review?(VYV03354)
Smaug:

FYI: Querying content could be performed from a selection change notification synchronously if native IME requests it. (And already PuppetWidget does it in e10s mode) So, The cases of new oranges may already occur in current builds.
Comment on attachment 8603729 [details] [diff] [review]
part.2 Set proper composition font when writing mode is changed

Review of attachment 8603729 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsIMM32Handler.cpp
@@ +2136,5 @@
> +{
> +  aLogFont.lfEscapement = aLogFont.lfOrientation = 0;
> +  if (NS_WARN_IF(aFontFace.Length() > LF_FACESIZE - 1)) {
> +    memcpy(aLogFont.lfFaceName, L"System", 6 * sizeof(wchar_t));
> +    aLogFont.lfFaceName[6] = 0;

memcpy(aLogFont.lfFaceName, L"System", sizeof(L"System"));
You don't have to terminate lfFaceName manually because literal strings will contain a trailing NUL character (sizeof(L"System") == 7 * sizeof(wchar_t)).

@@ +2151,5 @@
> +{
> +  aLogFont.lfEscapement = aLogFont.lfOrientation = 2700;
> +  if (NS_WARN_IF(aFontFace.Length() > LF_FACESIZE - 2)) {
> +    memcpy(aLogFont.lfFaceName, L"@System", 7 * sizeof(wchar_t));
> +    aLogFont.lfFaceName[7] = 0;

Ditto.
Attachment #8603729 - Flags: review?(VYV03354) → review-
Attachment #8603729 - Flags: review- → review+
Attachment #8603730 - Flags: review?(VYV03354) → review+
Comment on attachment 8602657 [details] [diff] [review]
part.6 Selection change notification should have selection range and writing mode information

Okay, let's disable test_backspace_delete.xul for now because it probably detects existing bug of bidi.
Flags: needinfo?(bugs)
Attachment #8602657 - Flags: review-
Let's disable the test for now.
Attachment #8605322 - Flags: review?(smontagu)
Attachment #8605322 - Flags: review?(smontagu) → review+
I pushed trivial mingw fixup for char16_t/wchar_t conflict.
(In reply to Jacek Caban from comment #43)
> I pushed trivial mingw fixup for char16_t/wchar_t conflict.

Thanks, but what's the "wwc"? I think that we should use static_cast for conforming our coding rules.
That's a helper to avoid such casts when not needed:
http://hg.mozilla.org/mozilla-central/file/0273e9c967ec/xpcom/string/nsString.h#l178

I will change it to casts if you prefer, but that would require reinterpret_cast.
Thank you for your explanation. No problem because it's our internal helper method.
Depends on: 1166436
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: