Currently, nsWidnow for Android dispatches both keyboard events and composition events. It should use TextEventDispatcher. Then, we can change XP level behavior easier.

Masayuki-san, we need this bug for supporting e10s IME on Android. Are you okay with me taking this bug?

Yeah, of course. Please go ahead!

We potentially dispatch key events during composition to provide compatibility for pages that only listen to key events.

We use nsIWidget::DispatchInputEvent to dispatch our keyboard events on the Gecko thread, which on Android is not the APZ controller thread. We should allow these events to pass instead of crashing.

Add a separate GeckoEditableSupport class, which implements TextEventDispatcherListener and uses TextEventDispatcher for IME operations. The new class is entirely separate from nsWindow to allow it to be independently used in the child process as well. Most of the code is copied from nsWindow::GeckoViewSupport, and adapted to use TextEventDispatcher. One caveat with the new class is it supports sending events through either TextEventDispatcher if available, or through nsWindow directly. The former is used when we have a focused editor; the latter is used when we don't have a focused editor (e.g. when the user presses a key outside of a text field).

Make nsWindow::WindowPtr available not just for classes inside nsWindow but for outside classes as well. Also, add support for RefPtr native objects to nsWindow::NativePtr.

Use the new GeckoEditableSupport class in nsWindow to replace the previous code in nsWindow::GeckoViewSupport. GeckoEditable native methods now go to GeckoEditableSupport instead of GeckoViewSupport. Several native methods in GeckoEditable are changed from dispatchTo="proxy" to dispatchTo="gecko", because we no longer need the special nsWindow::WindowEvent wrapper for our native calls.

Comment on attachment 8836814 [details] [diff] [review] 1. Allow dispatching key events during composition > +pref("dom.keyboardevent.dispatch_during_composition", true); Enabling this pref will dispatch keydown and keyup events every time. Is it okay? In my understanding, Firefox for Android dispatches them only when composition events are not listened by the web page. (But perhaps, using this pref won't cause any problems because other browsers dispatch them during composition.)

Comment on attachment 8836816 [details] [diff] [review] 3. Add GeckoEditableSupport class to support TextEventDispatcher (v1) >+#include "nsIContent.h" >+#include "nsISelection.h" >+ >+#include "mozilla/IMEStateManager.h" Hmm, I hope we'll get rid of referring these headers from here because these classes are no designed to cache remote process information. So, referring these classes doesn't make sense. >+#include "mozilla/TextComposition.h" Although, this caches some information of remote process, I don't assume that this is referred from widget. >+static unsigned int >+ConvertAndroidKeyCodeToDOMKeyCode(int androidKeyCode) Please use uint32_t for DOMKeyCode. >+ // Needs to confirm the behavior. If the key switches the open state >+ // of Japanese IME (or switches input character between Hiragana and >+ // Roman numeric characters), then, it might be better to use >+ // NS_VK_KANJI which is used for Alt+Zenkaku/Hankaku key on Windows. >+ case AKEYCODE_ZENKAKU_HANKAKU: return 0; >+ case AKEYCODE_EISU: return NS_VK_EISU; >+ case AKEYCODE_MUHENKAN: return NS_VK_NONCONVERT; >+ case AKEYCODE_HENKAN: return NS_VK_CONVERT; >+ case AKEYCODE_KATAKANA_HIRAGANA: return 0; Oh, I should do this... >+static bool >+IsModifierKey(int32_t keyCode) >+{ >+ using mozilla::java::sdk::KeyEvent; >+ return keyCode == KeyEvent::KEYCODE_ALT_LEFT || >+ keyCode == KeyEvent::KEYCODE_ALT_RIGHT || >+ keyCode == KeyEvent::KEYCODE_SHIFT_LEFT || >+ keyCode == KeyEvent::KEYCODE_SHIFT_RIGHT || >+ keyCode == KeyEvent::KEYCODE_CTRL_LEFT || >+ keyCode == KeyEvent::KEYCODE_CTRL_RIGHT || >+ keyCode == KeyEvent::KEYCODE_META_LEFT || >+ keyCode == KeyEvent::KEYCODE_META_RIGHT; >+} This is used only for checking if the keyboard event should cause keypress event. However, it's automatically checked in TextEventDispatcher. https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/widget/TextEventDispatcher.cpp#395,417-418 So, you can omit this method and the check in GeckoEditableSupport::OnKeyEvent(). >+static void >+InitKeyEvent(WidgetKeyboardEvent& event, >+ int32_t action, int32_t keyCode, int32_t scanCode, >+ int32_t metaState, int64_t time, int32_t unicodeChar, >+ int32_t baseUnicodeChar, int32_t domPrintableKeyValue, >+ int32_t repeatCount, int32_t flags) >+{ >+ const uint32_t domKeyCode = ConvertAndroidKeyCodeToDOMKeyCode(keyCode); >+ const int32_t charCode = unicodeChar ? unicodeChar : baseUnicodeChar; >+ >+ event.mModifiers = nsWindow::GetModifiers(metaState); >+ >+ if (event.mMessage == eKeyPress) { >+ // Android gives us \n, so filter out some control characters. >+ event.mIsChar = (charCode >= ' '); mIsChar isn't used anymore. So, ignoring this is OK (e.g., widget for Windows doesn't set this properly) if it can make this method simpler. >+ event.mCharCode = event.mIsChar ? charCode : 0; mCharCode value is automatically computed from mKeyValue by TextEventDispatcher. https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/widget/TextEventDispatcher.cpp#395,449,454-455,465-478 So, this class doesn't need to compute it anymore. >+ event.mKeyCode = event.mIsChar ? 0 : domKeyCode; TextEventDispatcher also clears mKeyCode value to 0 if it should be so. https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/widget/TextEventDispatcher.cpp#395,468-470 So, you can just set the value without any check. >+ // For keypress, if the unicode char already has modifiers applied, we >+ // don't specify extra modifiers. If UnicodeChar() != BaseUnicodeChar() >+ // it means UnicodeChar() already has modifiers applied. >+ // Note that on Android 4.x, Alt modifier isn't set when the key input >+ // causes text input even while right Alt key is pressed. However, >+ // this is necessary for Android 2.3 compatibility. >+ if (unicodeChar && unicodeChar != baseUnicodeChar) { >+ event.mModifiers &= ~(MODIFIER_ALT | MODIFIER_CONTROL >+ | MODIFIER_META); >+ } Unfortunately, this is still necessary. I should move this code to TextEventDispatcher though. >+ } else { >+ event.mIsChar = false; >+ event.mCharCode = 0; >+ event.mKeyCode = domKeyCode; So, those code should be handled by TextEventDispatcher. >+ ANPEvent pluginEvent; >+ pluginEvent.inSize = sizeof(pluginEvent); >+ pluginEvent.eventType = kKey_ANPEventType; >+ pluginEvent.data.key.action = event.mMessage == eKeyDown >+ ? kDown_ANPKeyAction : kUp_ANPKeyAction; >+ pluginEvent.data.key.nativeCode = keyCode; >+ pluginEvent.data.key.virtualCode = domKeyCode; >+ pluginEvent.data.key.unichar = charCode; >+ pluginEvent.data.key.modifiers = >+ (metaState & sdk::KeyEvent::META_SHIFT_MASK >+ ? kShift_ANPKeyModifier : 0) | >+ (metaState & sdk::KeyEvent::META_ALT_MASK >+ ? kAlt_ANPKeyModifier : 0); >+ pluginEvent.data.key.repeatCount = repeatCount; >+ event.mPluginEvent.Copy(pluginEvent); However, only this needs to be handled in here. So, you need to check if it's not for eKeyPress only for this block. >+ if (event.mKeyNameIndex == KEY_NAME_INDEX_USE_STRING && >+ domPrintableKeyValue) { >+ event.mKeyValue = char16_t(domPrintableKeyValue); >+ } For computing mCharCode and conforming to UI Events spec, mKeyValue should be non-empty string even if Ctrl or Meta key is pressed and it causes not inputting any character. So, please check if this does right thing. >+RefPtr<TextComposition> >+GeckoEditableSupport::GetComposition() >+{ >+ nsCOMPtr<nsIWidget> widget(GetWidget()); >+ return widget ? IMEStateManager::GetTextCompositionFor(widget) : nullptr; >+} So, we should get rid of this method... >+void >+GeckoEditableSupport::RemoveComposition(RemoveCompositionFlag aFlag) >+{ >+ if (!mDispatcher || !mDispatcher->IsComposing()) { >+ return; >+ } >+ >+ nsEventStatus status = nsEventStatus_eIgnore; >+ nsString emptyString; >+ >+ NS_ENSURE_SUCCESS_VOID(mDispatcher->BeginNativeInputTransaction()); >+ mDispatcher->CommitComposition( >+ status, aFlag == CANCEL_IME_COMPOSITION ? &emptyString : nullptr); Use |&EmptyString()|. >+ if (mDispatcher) { >+ RemoveComposition(); >+ NS_ENSURE_SUCCESS_VOID(mDispatcher->BeginNativeInputTransaction()); >+ mDispatcher->DispatchKeyboardEvent(msg, event, status); >+ } else { >+ status = widget->DispatchInputEvent(&event); >+ } This is odd and doesn't make sense because TextEventDispatcher::DispatchKeyboardEvent() guarantees same behavior on all platforms. So, you should use TextEventDispatcher of this |widget|. https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/widget/nsBaseWidget.cpp#1830,1833 >+ if (msg != eKeyDown || IsModifierKey(aKeyCode)) { >+ // Skip sending key press event. >+ return; >+ } So, now, you don't need this check because TextEventDispatcher does it before dispatching keypress events. >+ WidgetKeyboardEvent pressEvent(true, eKeyPress, widget); >+ InitKeyEvent(pressEvent, aAction, aKeyCode, aScanCode, aMetaState, aTime, >+ aUnicodeChar, aBaseUnicodeChar, aDomPrintableKeyValue, >+ aRepeatCount, aFlags); >+ >+ if (aIsSynthesizedImeKey) { >+ mIMEKeyEvents.AppendElement( >+ mozilla::UniquePtr<WidgetEvent>(pressEvent.Duplicate())); >+ } else if (mDispatcher) { >+ mDispatcher->MaybeDispatchKeypressEvents(pressEvent, status); >+ } else { >+ status = widget->DispatchInputEvent(&pressEvent); So, this should use TextEventDispatcher of |widget| too. >+void >+GeckoEditableSupport::SendIMEDummyKeyEvents(nsIWidget* aWidget) >+{ >+ nsEventStatus status = nsEventStatus_eIgnore; >+ MOZ_ASSERT(mDispatcher); >+ >+ WidgetKeyboardEvent downEvent(true, eKeyDown, aWidget); >+ downEvent.mTime = PR_Now() / 1000; >+ MOZ_ASSERT(downEvent.mKeyCode == 0); >+ NS_ENSURE_SUCCESS_VOID(mDispatcher->BeginNativeInputTransaction()); >+ mDispatcher->DispatchKeyboardEvent(eKeyDown, downEvent, status); >+ >+ WidgetKeyboardEvent upEvent(true, eKeyUp, aWidget); >+ upEvent.mTime = downEvent.mTime; >+ MOZ_ASSERT(upEvent.mKeyCode == 0); >+ mDispatcher->DispatchKeyboardEvent(eKeyUp, upEvent, status); >+} I don't understand why this method is still necessary because you turned the pref of TextEventDispatcher enabled at the first patch... >+void >+GeckoEditableSupport::AddIMETextChange(const IMETextChange& aChange) >+{ >+ mIMETextChanges.AppendElement(aChange); >+ >+ // We may not be in the middle of flushing, >+ // in which case this flag is meaningless. >+ mIMETextChangedDuringFlush = true; >+ >+ // Now that we added a new range we need to go back and >+ // update all the ranges before that. >+ // Ranges that have offsets which follow this new range >+ // need to be updated to reflect new offsets >+ const int32_t delta = aChange.mNewEnd - aChange.mOldEnd; >+ for (int32_t i = mIMETextChanges.Length() - 2; i >= 0; i--) { >+ IMETextChange& previousChange = mIMETextChanges[i]; >+ if (previousChange.mStart > aChange.mOldEnd) { >+ previousChange.mStart += delta; >+ previousChange.mOldEnd += delta; >+ previousChange.mNewEnd += delta; >+ } >+ } >+ >+ // Now go through all ranges to merge any ranges that are connected >+ // srcIndex is the index of the range to merge from >+ // dstIndex is the index of the range to potentially merge into >+ int32_t srcIndex = mIMETextChanges.Length() - 1; >+ int32_t dstIndex = srcIndex; >+ >+ while (--dstIndex >= 0) { >+ IMETextChange& src = mIMETextChanges[srcIndex]; >+ IMETextChange& dst = mIMETextChanges[dstIndex]; >+ // When merging a more recent change into an older >+ // change, we need to compare recent change's (start, oldEnd) >+ // range to the older change's (start, newEnd) >+ if (src.mOldEnd < dst.mStart || dst.mNewEnd < src.mStart) { >+ // No overlap between ranges >+ continue; >+ } >+ // When merging two ranges, there are generally four posibilities: >+ // [----(----]----), (----[----]----), >+ // [----(----)----], (----[----)----] >+ // where [----] is the first range and (----) is the second range >+ // As seen above, the start of the merged range is always the lesser >+ // of the two start offsets. OldEnd and NewEnd then need to be >+ // adjusted separately depending on the case. In any case, the change >+ // in text length of the merged range should be the sum of text length >+ // changes of the two original ranges, i.e., >+ // newNewEnd - newOldEnd == newEnd1 - oldEnd1 + newEnd2 - oldEnd2 >+ dst.mStart = std::min(dst.mStart, src.mStart); >+ if (src.mOldEnd < dst.mNewEnd) { >+ // New range overlaps or is within previous range; merge >+ dst.mNewEnd += src.mNewEnd - src.mOldEnd; >+ } else { // src.mOldEnd >= dst.mNewEnd >+ // New range overlaps previous range; merge >+ dst.mOldEnd += src.mOldEnd - dst.mNewEnd; >+ dst.mNewEnd = src.mNewEnd; >+ } I wonder, it might be possible to replace this with TextChangeDataBase::MergeWith(). https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/widget/nsBaseWidget.cpp#2389 Anyway, moving this code to TextChangeDataBase might make this class simpler. I'll check FlushIMEChanges() and following methods later.

Comment on attachment 8836816 [details] [diff] [review] 3. Add GeckoEditableSupport class to support TextEventDispatcher (v1) >+void >+GeckoEditableSupport::FlushIMEChanges(FlushChangesFlag aFlags) >+{ >+ // Only send change notifications if we are *not* masking events, >+ // i.e. if we have a focused editor, >+ NS_ENSURE_TRUE_VOID(!mIMEMaskEventsCount); >+ >+ nsCOMPtr<nsIWidget> widget(GetWidget()); >+ nsCOMPtr<nsISelection> imeSelection; >+ nsCOMPtr<nsIContent> imeRoot; >+ NS_ENSURE_TRUE_VOID(widget); >+ >+ // If we are receiving notifications, we must have selection/root content. >+ nsresult rv = IMEStateManager::GetFocusSelectionAndRoot( >+ getter_AddRefs(imeSelection), getter_AddRefs(imeRoot)); >+ NS_ENSURE_SUCCESS_VOID(rv); I hope that this is not necessary, see below. >+ // Make sure we still have a valid selection/root. We can potentially get >+ // a stale selection/root if the editor becomes hidden, for example. >+ NS_ENSURE_TRUE_VOID(imeRoot->IsInComposedDoc()); Perhaps, in such case, widget should receive NOTIFY_IME_OF_BLUR. So, I don't think widget needs to check with actual content. >+ for (const IMETextChange &change : mIMETextChanges) { >+ if (change.mStart == change.mOldEnd && >+ change.mStart == change.mNewEnd) { >+ continue; >+ } >+ >+ WidgetQueryContentEvent event(true, eQueryTextContent, widget); >+ >+ if (change.mNewEnd != change.mStart) { >+ event.InitForQueryTextContent(change.mStart, >+ change.mNewEnd - change.mStart); >+ widget->DispatchEvent(&event, status); Hmm, this could be too slow due to ContentEventHandler's design. Why don't you query all text and reuse it in this for loop? >+ NS_ENSURE_TRUE_VOID(event.mSucceeded); >+ NS_ENSURE_TRUE_VOID(event.mReply.mContentsRoot == imeRoot.get()); Same above. If this becomes false, without NOTIFY_IME_OF_BLUR, there should be a bug. >+ } >+ >+ if (shouldAbort()) { >+ return; >+ } FYI: ContentEventHandler will flush pending layout. So, I think this case can be only first time. >+ >+ textTransaction.AppendElement( >+ TextRecord{event.mReply.mString, change.mStart, >+ change.mOldEnd, change.mNewEnd}); >+ } >+ >+ int32_t selStart = -1; >+ int32_t selEnd = -1; >+ >+ if (mIMESelectionChanged) { >+ WidgetQueryContentEvent event(true, eQuerySelectedText, widget); >+ widget->DispatchEvent(&event, status); >+ >+ NS_ENSURE_TRUE_VOID(event.mSucceeded); >+ NS_ENSURE_TRUE_VOID(event.mReply.mContentsRoot == imeRoot.get()); Same above, should be a bug if this is false without focus change. >+ >+ if (shouldAbort()) { >+ return; >+ } >+ >+ selStart = int32_t(event.GetSelectionStart()); >+ selEnd = int32_t(event.GetSelectionEnd()); If you cache selection at NOTIFY_IME_OF_SELECTION_CHANGE, perhaps, you don't need to dispatch eQuerySelectedText here. Although, eQuerySelectedText may be handled by IMEContentObserver with its cache. >+void >+GeckoEditableSupport::FlushIMEText(FlushChangesFlag aFlags) >+{ >+ // Notify Java of the newly focused content >+ mIMETextChanges.Clear(); >+ mIMESelectionChanged = true; >+ >+ // Use 'INT32_MAX / 2' here because subsequent text changes might combine >+ // with this text change, and overflow might occur if we just use >+ // INT32_MAX. >+ IMENotification notification(NOTIFY_IME_OF_TEXT_CHANGE); >+ notification.mTextChangeData.mStartOffset = 0; >+ notification.mTextChangeData.mRemovedEndOffset = INT32_MAX / 2; >+ notification.mTextChangeData.mAddedEndOffset = INT32_MAX / 2; >+ NotifyIME(mDispatcher, notification); I guess that this notification isn't necessary for TextEventDispatcher. So, not using nsIWidget::NotifyIME() must be OK. >+void >+GeckoEditableSupport::UpdateCompositionRects() >+{ >+ nsCOMPtr<nsIWidget> widget(GetWidget()); >+ RefPtr<TextComposition> composition(GetComposition()); This should be replaced with something others... >+ NS_ENSURE_TRUE_VOID(mDispatcher && widget); >+ >+ if (!composition) { This should be same as TextEventDispatcher::IsComposing(). >+ return; >+ } >+ >+ nsEventStatus status = nsEventStatus_eIgnore; >+ uint32_t offset = composition->NativeOffsetOfStartComposition(); >+ WidgetQueryContentEvent textRects(true, eQueryTextRectArray, widget); >+ textRects.InitForQueryTextRectArray(offset, composition->String().Length()); This should be replaced with "Relative To Insertion Point" query like this. https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/widget/windows/IMMHandler.cpp#2220-2223 When there is composition string, the start of composition string is used as base of offset. Otherwise, selection start is used. So, this should be: > WidgetQueryContentEvent textRects(true, eQueryTextRectArray, widget); > WidgetQueryContentEvent::Options options; > options.mRelativeToInsertionPoint = true; > textRects.InitForQueryTextRectArray(0, composition->String().Length(), options); FYI: When selection is moved by compositionstart event handler, the start offset of composition string is the new selection start. This query is aware of such case. >+void >+GeckoEditableSupport::OnImeReplaceText(int32_t aStart, int32_t aEnd, >+ jni::String::Param aText) >+{ >+ RefPtr<TextComposition> composition(GetComposition()); >+ MOZ_ASSERT(!composition || !composition->IsEditorHandlingEvent()); >+ >+ nsString string(aText->ToString()); nit: nsAutoString. >+ const bool composing = !mIMERanges->IsEmpty(); >+ nsEventStatus status = nsEventStatus_eIgnore; >+ >+ if (!mIMEKeyEvents.IsEmpty() || !mDispatcher->IsComposing() || >+ !composition || |!mDispatcher->IsComposing()| and |!composition| should be same. >+ uint32_t(aStart) != composition->NativeOffsetOfStartComposition() || >+ uint32_t(aEnd) != composition->NativeOffsetOfStartComposition() + >+ composition->String().Length()) Does this work with the case when the web page moves selection at compositionstart? If not, I think that you don't need to check these offsets. Unfortunately, currently, nobody stores this information in widget. So, if you really need these offsets, perhaps, NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED should notify widget of these information later. >+ { >+ // Only start a new composition if we have key events, >+ // if we don't have an existing composition, or >+ // the replaced text does not match our composition. >+ RemoveComposition(); >+ >+ { >+ // Use text selection to set target position(s) for >+ // insert, or replace, of text. >+ WidgetSelectionEvent event(true, eSetSelection, widget); >+ event.mOffset = uint32_t(aStart); >+ event.mLength = uint32_t(aEnd - aStart); >+ event.mExpandToClusterBoundary = false; >+ event.mReason = nsISelectionListener::IME_REASON; >+ widget->DispatchEvent(&event, status); >+ } >+ >+ if (!mIMEKeyEvents.IsEmpty()) { >+ for (uint32_t i = 0; i < mIMEKeyEvents.Length(); i++) { >+ const auto event = mIMEKeyEvents[i]->AsKeyboardEvent(); >+ // widget for duplicated events is initially nullptr. >+ event->mWidget = widget; >+ >+ if (event->mMessage != eKeyPress) { >+ mDispatcher->DispatchKeyboardEvent( >+ event->mMessage, *event, status); >+ } else if (status == nsEventStatus_eConsumeNoDefault) { >+ // The previous key down event resulted in eConsumeNoDefault >+ // so we should not dispatch the current key press event. >+ MOZ_ASSERT(i > 0 && >+ mIMEKeyEvents[i - 1]->mMessage == eKeyDown); This |else if| block isn't necessary because TextEventDispatcher::MaybeDispatchKeypressEvents() checks status. >+ } else { >+ mDispatcher->MaybeDispatchKeypressEvents(*event, status); >+ } So, only with this |else| block is fine and better for consistency. >+ } else if (composition->String().Equals(string)) { >+ /* If the new text is the same as the existing composition text, >+ * the NS_COMPOSITION_CHANGE event does not generate a text >+ * change notification. However, the Java side still expects >+ * one, so we manually generate a notification. */ >+ IMETextChange dummyChange; >+ dummyChange.mStart = aStart; >+ dummyChange.mOldEnd = dummyChange.mNewEnd = aEnd; >+ AddIMETextChange(dummyChange); >+ } >+ >+ if (composing) { >+ mDispatcher->SetPendingComposition(string, mIMERanges); >+ mDispatcher->FlushPendingComposition(status); >+ // Ensure IME ranges are empty. >+ mIMERanges->Clear(); >+ } else { >+ mDispatcher->CommitComposition(status, &string); >+ } >+ >+ if (mInputContext.mMayBeIMEUnaware) { >+ SendIMEDummyKeyEvents(widget); This is odd. "keydown" event should be dispatched before composition event and "keyup" event (if there is) should be dispatched after composition event. However, looks like both of them are dispatched after composition event. This is invalid event order (declared by UI Events). >+void >+GeckoEditableSupport::OnImeUpdateComposition(int32_t aStart, int32_t aEnd) >+{ >+ >+ /* >+ Update the composition from aStart to aEnd using >+ information from added ranges. This is only used for >+ visual indication and does not affect the text content. >+ Only the offsets are specified and not the text content >+ to eliminate the possibility of this event altering the >+ text content unintentionally. >+ */ >+ nsString string; >+ RefPtr<TextComposition> composition(GetComposition()); >+ MOZ_ASSERT(!composition || !composition->IsEditorHandlingEvent()); >+ >+ if (!mDispatcher->IsComposing() || !composition || So, these check must be same result. >+ uint32_t(aStart) != composition->NativeOffsetOfStartComposition() || >+ uint32_t(aEnd) != composition->NativeOffsetOfStartComposition() + >+ composition->String().Length()) And also I worry about same thing mentioned above. >+nsresult >+GeckoEditableSupport::NotifyIME(TextEventDispatcher* aTextEventDispatcher, >+ const IMENotification& aNotification) >+{ >+ MOZ_ASSERT(mEditable); >+ >+ switch (aNotification.mMessage) { >+ case REQUEST_TO_COMMIT_COMPOSITION: { >+ ALOGIME("IME: REQUEST_TO_COMMIT_COMPOSITION"); >+ >+ RemoveComposition(COMMIT_IME_COMPOSITION); >+ AsyncNotifyIME(GeckoEditableListener:: >+ NOTIFY_IME_TO_COMMIT_COMPOSITION); Oh, this request is always handled asynchronously on Android. So, in this case, TextComposition commits composition in the focused editor before native IME actually receives the notification. That means that IME cannot commit composition with different string. So, if IME tries to retrieve commit text after this, there may be different string. We *might* need to return fake string if some IME wouldn't work fine. >+ case NOTIFY_IME_OF_FOCUS: { >+ ALOGIME("IME: NOTIFY_IME_OF_FOCUS"); >+ // Keep a strong reference to the window to keep 'this' alive. >+ nsCOMPtr<nsIWidget> widget(GetWidget()); >+ if (!widget) { >+ break; >+ } >+ >+ RefPtr<TextEventDispatcher> dispatcher(aTextEventDispatcher); Although, this should be grabbed by the caller. But it's OK. >+ case NOTIFY_IME_OF_SELECTION_CHANGE: { >+ ALOGIME("IME: NOTIFY_IME_OF_SELECTION_CHANGE"); >+ >+ PostFlushIMEChanges(); >+ mIMESelectionChanged = true; >+ break; So, storing selection range here must make some other code simpler. FYI: Other platform's IME handlers do so for improving performance and making the code simpler. >+ case NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED: { >+ ALOGIME("IME: NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED"); >+ >+ // Hardware keyboard support requires each string rect. >+ if (mIMEMonitorCursor) { >+ UpdateCompositionRects(); >+ } FYI: If it's in e10s mode, this is notified when all dispatched composition events are handled in the remote process. So, if remote process is too busy and a lot of composition events are dispatched from widget, this won't be received until user stops typing. Note that all of them should be fixed in this bug, but should be fixed as soon as possible (ideally, in this cycle).

Comment on attachment 8836816 [details] [diff] [review] 3. Add GeckoEditableSupport class to support TextEventDispatcher (v1) Review of attachment 8836816 [details] [diff] [review]: ----------------------------------------------------------------- Looks like Masayuki is already taking care of this review.

(In reply to Masayuki Nakano [:masayuki] from comment #9) > Comment on attachment 8836816 [details] [diff] [review] > 3. Add GeckoEditableSupport class to support TextEventDispatcher (v1) > > >+#include "nsIContent.h" > >+#include "nsISelection.h" > >+ > >+#include "mozilla/IMEStateManager.h" > > Hmm, I hope we'll get rid of referring these headers from here because these > classes are no designed to cache remote process information. So, referring > these classes doesn't make sense. > > >+#include "mozilla/TextComposition.h" > > Although, this caches some information of remote process, I don't assume > that this is referred from widget. For Android e10s, native IME code will run in the content process and communicate directly with Java code running in the main process. Therefore, all the code here will run in the content process, so we don't need IMEStateManager/TextComposition to do any caching. > >+ // For keypress, if the unicode char already has modifiers applied, we > >+ // don't specify extra modifiers. If UnicodeChar() != BaseUnicodeChar() > >+ // it means UnicodeChar() already has modifiers applied. > >+ // Note that on Android 4.x, Alt modifier isn't set when the key input > >+ // causes text input even while right Alt key is pressed. However, > >+ // this is necessary for Android 2.3 compatibility. > >+ if (unicodeChar && unicodeChar != baseUnicodeChar) { > >+ event.mModifiers &= ~(MODIFIER_ALT | MODIFIER_CONTROL > >+ | MODIFIER_META); > >+ } > > Unfortunately, this is still necessary. I should move this code to > TextEventDispatcher though. The new patch moves this to the Java layer, and uses a "keyPressMetaState" to specify a different set of modifiers for the keypress event, if needed. > >+RefPtr<TextComposition> > >+GeckoEditableSupport::GetComposition() > >+{ > >+ nsCOMPtr<nsIWidget> widget(GetWidget()); > >+ return widget ? IMEStateManager::GetTextCompositionFor(widget) : nullptr; > >+} > > So, we should get rid of this method... Unfortunately we still need TextComposition::NativeOffsetOfStartComposition. > >+void > >+GeckoEditableSupport::SendIMEDummyKeyEvents(nsIWidget* aWidget) > >+{ > >+ nsEventStatus status = nsEventStatus_eIgnore; > >+ MOZ_ASSERT(mDispatcher); > >+ > >+ WidgetKeyboardEvent downEvent(true, eKeyDown, aWidget); > >+ downEvent.mTime = PR_Now() / 1000; > >+ MOZ_ASSERT(downEvent.mKeyCode == 0); > >+ NS_ENSURE_SUCCESS_VOID(mDispatcher->BeginNativeInputTransaction()); > >+ mDispatcher->DispatchKeyboardEvent(eKeyDown, downEvent, status); > >+ > >+ WidgetKeyboardEvent upEvent(true, eKeyUp, aWidget); > >+ upEvent.mTime = downEvent.mTime; > >+ MOZ_ASSERT(upEvent.mKeyCode == 0); > >+ mDispatcher->DispatchKeyboardEvent(eKeyUp, upEvent, status); > >+} > > I don't understand why this method is still necessary because you turned the > pref of TextEventDispatcher enabled at the first patch... The pref only allows us to send key events during composition [1], instead of returning early and ignore the key event, but we still have to generate the key events. [1] http://searchfox.org/mozilla-central/rev/cac6cb6a10afb8ebb2ecfbeeedaff7c66f57dd75/widget/TextEventDispatcher.cpp#429 > I wonder, it might be possible to replace this with > TextChangeDataBase::MergeWith(). > https://dxr.mozilla.org/mozilla-central/rev/ > 0eef1d5a39366059677c6d7944cfe8a97265a011/widget/nsBaseWidget.cpp#2389 > > Anyway, moving this code to TextChangeDataBase might make this class simpler. Maybe as a follow-up, I'd rather not introduce new changes that could cause regressions. (In reply to Masayuki Nakano [:masayuki] from comment #10) > Comment on attachment 8836816 [details] [diff] [review] > 3. Add GeckoEditableSupport class to support TextEventDispatcher (v1) > > >+ // Make sure we still have a valid selection/root. We can potentially get > >+ // a stale selection/root if the editor becomes hidden, for example. > >+ NS_ENSURE_TRUE_VOID(imeRoot->IsInComposedDoc()); > > Perhaps, in such case, widget should receive NOTIFY_IME_OF_BLUR. So, I don't > think widget needs to check with actual content. For this test case [1], any key will hide the text field but we don't receive NOTIFY_IME_OF_BLUR. However, we do seem to gracefully handle subsequent failures, so I removed the checks. [1] http://searchfox.org/mozilla-central/rev/ea31c4b64f34a29415a69fb768f8051495547315/mobile/android/tests/browser/robocop/robocop_input.html#33 > >+ if (change.mNewEnd != change.mStart) { > >+ event.InitForQueryTextContent(change.mStart, > >+ change.mNewEnd - change.mStart); > >+ widget->DispatchEvent(&event, status); > > Hmm, this could be too slow due to ContentEventHandler's design. Why don't > you query all text and reuse it in this for loop? Most times we will have 1 or 2 text changes, so I think it's more efficient to query each one. > >+ > >+ if (shouldAbort()) { > >+ return; > >+ } > >+ > >+ selStart = int32_t(event.GetSelectionStart()); > >+ selEnd = int32_t(event.GetSelectionEnd()); > > If you cache selection at NOTIFY_IME_OF_SELECTION_CHANGE, perhaps, you don't > need to dispatch eQuerySelectedText here. Although, eQuerySelectedText may > be handled by IMEContentObserver with its cache. We want the most up-to-date selection the moment FlushIMEChanges is called, so I think it's easiest to dispatch eQuerySelectedText manually. > >+ // Use 'INT32_MAX / 2' here because subsequent text changes might combine > >+ // with this text change, and overflow might occur if we just use > >+ // INT32_MAX. > >+ IMENotification notification(NOTIFY_IME_OF_TEXT_CHANGE); > >+ notification.mTextChangeData.mStartOffset = 0; > >+ notification.mTextChangeData.mRemovedEndOffset = INT32_MAX / 2; > >+ notification.mTextChangeData.mAddedEndOffset = INT32_MAX / 2; > >+ NotifyIME(mDispatcher, notification); > > I guess that this notification isn't necessary for TextEventDispatcher. So, > not using nsIWidget::NotifyIME() must be OK. It's necessary for resetting the Java-side text cache. > >+ return; > >+ } > >+ > >+ nsEventStatus status = nsEventStatus_eIgnore; > >+ uint32_t offset = composition->NativeOffsetOfStartComposition(); > >+ WidgetQueryContentEvent textRects(true, eQueryTextRectArray, widget); > >+ textRects.InitForQueryTextRectArray(offset, composition->String().Length()); > > This should be replaced with "Relative To Insertion Point" query like this. > https://dxr.mozilla.org/mozilla-central/rev/ > 0eef1d5a39366059677c6d7944cfe8a97265a011/widget/windows/IMMHandler.cpp#2220- > 2223 > > When there is composition string, the start of composition string is used as > base of offset. Otherwise, selection start is used. So, this should be: > > > WidgetQueryContentEvent textRects(true, eQueryTextRectArray, widget); > > WidgetQueryContentEvent::Options options; > > options.mRelativeToInsertionPoint = true; > > textRects.InitForQueryTextRectArray(0, composition->String().Length(), options); > > FYI: When selection is moved by compositionstart event handler, the start > offset of composition string is the new selection start. This query is aware > of such case. This is all copied from existing code in nsWindow.cpp, so I think making additional changes is outside the scope of this bug. > >+ uint32_t(aStart) != composition->NativeOffsetOfStartComposition() || > >+ uint32_t(aEnd) != composition->NativeOffsetOfStartComposition() + > >+ composition->String().Length()) > > Does this work with the case when the web page moves selection at > compositionstart? If not, I think that you don't need to check these offsets. > > Unfortunately, currently, nobody stores this information in widget. So, if > you really need these offsets, perhaps, > NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED should notify widget of these > information later. We need this comparison because Java can give us any offset, including offsets outside of the current composition. > >+ if (mInputContext.mMayBeIMEUnaware) { > >+ SendIMEDummyKeyEvents(widget); > > This is odd. "keydown" event should be dispatched before composition event > and "keyup" event (if there is) should be dispatched after composition > event. However, looks like both of them are dispatched after composition > event. This is invalid event order (declared by UI Events). Changed to keydown before composition change, and keyup after. > >+ RemoveComposition(COMMIT_IME_COMPOSITION); > >+ AsyncNotifyIME(GeckoEditableListener:: > >+ NOTIFY_IME_TO_COMMIT_COMPOSITION); > > Oh, this request is always handled asynchronously on Android. So, in this > case, TextComposition commits composition in the focused editor before > native IME actually receives the notification. That means that IME cannot > commit composition with different string. So, if IME tries to retrieve > commit text after this, there may be different string. We *might* need to > return fake string if some IME wouldn't work fine. This is a no-op on the Java side (even though we send the notification to Java), so it should be okay.

Updated patch.

Sorry for the delay to review, I'll be back soon. (In reply to Jim Chen [:jchen] [:darchons] from comment #12) > Created attachment 8838352 [details] [diff] [review] > 3. Add GeckoEditableSupport class to support TextEventDispatcher (v2) > > (In reply to Masayuki Nakano [:masayuki] from comment #9) > > Comment on attachment 8836816 [details] [diff] [review] > > 3. Add GeckoEditableSupport class to support TextEventDispatcher (v1) > > > > >+#include "nsIContent.h" > > >+#include "nsISelection.h" > > >+ > > >+#include "mozilla/IMEStateManager.h" > > > > Hmm, I hope we'll get rid of referring these headers from here because these > > classes are no designed to cache remote process information. So, referring > > these classes doesn't make sense. > > > > >+#include "mozilla/TextComposition.h" > > > > Although, this caches some information of remote process, I don't assume > > that this is referred from widget. > > For Android e10s, native IME code will run in the content process and > communicate directly with Java code running in the main process. Therefore, > all the code here will run in the content process, so we don't need > IMEStateManager/TextComposition to do any caching. Thanks, it's interesting. But for consistency with other platforms, I'd like to hide them with ContentCache or TextEventDispatcher in the future. > > >+ // For keypress, if the unicode char already has modifiers applied, we > > >+ // don't specify extra modifiers. If UnicodeChar() != BaseUnicodeChar() > > >+ // it means UnicodeChar() already has modifiers applied. > > >+ // Note that on Android 4.x, Alt modifier isn't set when the key input > > >+ // causes text input even while right Alt key is pressed. However, > > >+ // this is necessary for Android 2.3 compatibility. > > >+ if (unicodeChar && unicodeChar != baseUnicodeChar) { > > >+ event.mModifiers &= ~(MODIFIER_ALT | MODIFIER_CONTROL > > >+ | MODIFIER_META); > > >+ } > > > > Unfortunately, this is still necessary. I should move this code to > > TextEventDispatcher though. > > The new patch moves this to the Java layer, and uses a "keyPressMetaState" > to specify a different set of modifiers for the keypress event, if needed. Hmm, I think that we should stop doing this hack as soon as possible because this is incompatible point from other browsers. So, we should add new flag like WidgetKeyboardEvent::mIsPrintable or something and expose actual modifier state to web apps. > > >+RefPtr<TextComposition> > > >+GeckoEditableSupport::GetComposition() > > >+{ > > >+ nsCOMPtr<nsIWidget> widget(GetWidget()); > > >+ return widget ? IMEStateManager::GetTextCompositionFor(widget) : nullptr; > > >+} > > > > So, we should get rid of this method... > > Unfortunately we still need TextComposition::NativeOffsetOfStartComposition. Yeah, but I think that we should notify the offset with a notification. Perhaps, NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED or something. And if widget needs it, it should be cached. > > >+void > > >+GeckoEditableSupport::SendIMEDummyKeyEvents(nsIWidget* aWidget) > > >+{ > > >+ nsEventStatus status = nsEventStatus_eIgnore; > > >+ MOZ_ASSERT(mDispatcher); > > >+ > > >+ WidgetKeyboardEvent downEvent(true, eKeyDown, aWidget); > > >+ downEvent.mTime = PR_Now() / 1000; > > >+ MOZ_ASSERT(downEvent.mKeyCode == 0); > > >+ NS_ENSURE_SUCCESS_VOID(mDispatcher->BeginNativeInputTransaction()); > > >+ mDispatcher->DispatchKeyboardEvent(eKeyDown, downEvent, status); > > >+ > > >+ WidgetKeyboardEvent upEvent(true, eKeyUp, aWidget); > > >+ upEvent.mTime = downEvent.mTime; > > >+ MOZ_ASSERT(upEvent.mKeyCode == 0); > > >+ mDispatcher->DispatchKeyboardEvent(eKeyUp, upEvent, status); > > >+} > > > > I don't understand why this method is still necessary because you turned the > > pref of TextEventDispatcher enabled at the first patch... > > The pref only allows us to send key events during composition [1], instead > of returning early and ignore the key event, but we still have to generate > the key events. > > [1] > http://searchfox.org/mozilla-central/rev/ > cac6cb6a10afb8ebb2ecfbeeedaff7c66f57dd75/widget/TextEventDispatcher.cpp#429 I don't understand "instead of returning early and ignore the key event". What did you mean? > > I wonder, it might be possible to replace this with > > TextChangeDataBase::MergeWith(). > > https://dxr.mozilla.org/mozilla-central/rev/ > > 0eef1d5a39366059677c6d7944cfe8a97265a011/widget/nsBaseWidget.cpp#2389 > > > > Anyway, moving this code to TextChangeDataBase might make this class simpler. > > Maybe as a follow-up, I'd rather not introduce new changes that could cause > regressions. Yeah, I agree with that. I should've made the comments clearer which should be fixed here or later. Keeping current behavior is the best for such big change, of course. > (In reply to Masayuki Nakano [:masayuki] from comment #10) > > Comment on attachment 8836816 [details] [diff] [review] > > 3. Add GeckoEditableSupport class to support TextEventDispatcher (v1) > > > > >+ // Make sure we still have a valid selection/root. We can potentially get > > >+ // a stale selection/root if the editor becomes hidden, for example. > > >+ NS_ENSURE_TRUE_VOID(imeRoot->IsInComposedDoc()); > > > > Perhaps, in such case, widget should receive NOTIFY_IME_OF_BLUR. So, I don't > > think widget needs to check with actual content. > > For this test case [1], any key will hide the text field but we don't > receive NOTIFY_IME_OF_BLUR. However, we do seem to gracefully handle > subsequent failures, so I removed the checks. > > [1] > http://searchfox.org/mozilla-central/rev/ > ea31c4b64f34a29415a69fb768f8051495547315/mobile/android/tests/browser/ > robocop/robocop_input.html#33 Wow, interesting. This should be a bug of IMEContentObserver (or focus manager) because not sending NOTIFY_IME_OF_BLUR means that IMEContentObserver keeps observing different editor from actual focused editor (unless if we can keep inputting the hidden editor). I'll file a bug after checking what happens in this case. > > >+ if (change.mNewEnd != change.mStart) { > > >+ event.InitForQueryTextContent(change.mStart, > > >+ change.mNewEnd - change.mStart); > > >+ widget->DispatchEvent(&event, status); > > > > Hmm, this could be too slow due to ContentEventHandler's design. Why don't > > you query all text and reuse it in this for loop? > > Most times we will have 1 or 2 text changes, so I think it's more efficient > to query each one. ContentEventHandler computes offset from start of the focused editor every time. So, if the offset is too big, one loop could take long time... But it's okay for this bug (for risk management). > > >+ > > >+ if (shouldAbort()) { > > >+ return; > > >+ } > > >+ > > >+ selStart = int32_t(event.GetSelectionStart()); > > >+ selEnd = int32_t(event.GetSelectionEnd()); > > > > If you cache selection at NOTIFY_IME_OF_SELECTION_CHANGE, perhaps, you don't > > need to dispatch eQuerySelectedText here. Although, eQuerySelectedText may > > be handled by IMEContentObserver with its cache. > > We want the most up-to-date selection the moment FlushIMEChanges is called, > so I think it's easiest to dispatch eQuerySelectedText manually. NOTIFY_IME_OF_SELECTION_CHANGE has same result as eQuerySelectedText. Although it might be delayed in e10s mode of desktop, otherwise, they should be always same (anyway, even in the former case, waiting NOTIFY_IME_OF_SELECTION should be safe because it's synced with NOTIFY_IME_OF_TEXT_CHANGE). (But of course, you should keep current behavior in this bug.) > > >+ // Use 'INT32_MAX / 2' here because subsequent text changes might combine > > >+ // with this text change, and overflow might occur if we just use > > >+ // INT32_MAX. > > >+ IMENotification notification(NOTIFY_IME_OF_TEXT_CHANGE); > > >+ notification.mTextChangeData.mStartOffset = 0; > > >+ notification.mTextChangeData.mRemovedEndOffset = INT32_MAX / 2; > > >+ notification.mTextChangeData.mAddedEndOffset = INT32_MAX / 2; > > >+ NotifyIME(mDispatcher, notification); > > > > I guess that this notification isn't necessary for TextEventDispatcher. So, > > not using nsIWidget::NotifyIME() must be OK. > > It's necessary for resetting the Java-side text cache. I meant that notifying only Java of this is okay ;-) > > > WidgetQueryContentEvent textRects(true, eQueryTextRectArray, widget); > > > WidgetQueryContentEvent::Options options; > > > options.mRelativeToInsertionPoint = true; > > > textRects.InitForQueryTextRectArray(0, composition->String().Length(), options); > > > > FYI: When selection is moved by compositionstart event handler, the start > > offset of composition string is the new selection start. This query is aware > > of such case. > > This is all copied from existing code in nsWindow.cpp, so I think making > additional changes is outside the scope of this bug. Yes. I just pointed what I worried. > > >+ uint32_t(aStart) != composition->NativeOffsetOfStartComposition() || > > >+ uint32_t(aEnd) != composition->NativeOffsetOfStartComposition() + > > >+ composition->String().Length()) > > > > Does this work with the case when the web page moves selection at > > compositionstart? If not, I think that you don't need to check these offsets. > > > > Unfortunately, currently, nobody stores this information in widget. So, if > > you really need these offsets, perhaps, > > NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED should notify widget of these > > information later. > > We need this comparison because Java can give us any offset, including > offsets outside of the current composition. Okay. > > >+ if (mInputContext.mMayBeIMEUnaware) { > > >+ SendIMEDummyKeyEvents(widget); > > > > This is odd. "keydown" event should be dispatched before composition event > > and "keyup" event (if there is) should be dispatched after composition > > event. However, looks like both of them are dispatched after composition > > event. This is invalid event order (declared by UI Events). > > Changed to keydown before composition change, and keyup after. Thanks, I'll check it. > > >+ RemoveComposition(COMMIT_IME_COMPOSITION); > > >+ AsyncNotifyIME(GeckoEditableListener:: > > >+ NOTIFY_IME_TO_COMMIT_COMPOSITION); > > > > Oh, this request is always handled asynchronously on Android. So, in this > > case, TextComposition commits composition in the focused editor before > > native IME actually receives the notification. That means that IME cannot > > commit composition with different string. So, if IME tries to retrieve > > commit text after this, there may be different string. We *might* need to > > return fake string if some IME wouldn't work fine. > > This is a no-op on the Java side (even though we send the notification to > Java), so it should be okay. So, I just worried about that IME assumes that contents are always as expected. On Windows and macOS, I hacked a lot for such IME :-(

(In reply to Masayuki Nakano [:masayuki] from comment #14) > > > > I don't understand why this method is still necessary because you turned the > > > pref of TextEventDispatcher enabled at the first patch... > > > > The pref only allows us to send key events during composition [1], instead > > of returning early and ignore the key event, but we still have to generate > > the key events. > > > > [1] > > http://searchfox.org/mozilla-central/rev/ > > cac6cb6a10afb8ebb2ecfbeeedaff7c66f57dd75/widget/TextEventDispatcher.cpp#429 > > I don't understand "instead of returning early and ignore the key event". > What did you mean? Sorry for the confusion. I was talking about why we still need `SendIMEDummyKeyEvents`. Turning on the "dom.keyboardevent.dispatch_during_composition" pref allows us to send events during composition. We still need `SendIMEDummyKeyEvents` to generate the events.

Comment on attachment 8838352 [details] [diff] [review] 3. Add GeckoEditableSupport class to support TextEventDispatcher (v2) >One caveat with the new class is it supports >sending events through either TextEventDispatcher if available, or >through nsWindow directly. The former is used when we have a focused >editor; the latter is used when we don't have a focused editor (e.g. >when the user presses a key outside of a text field). I think that this commit comment is outdated, isn't it? >+ switch (androidKeyCode) { >+ // KEYCODE_UNKNOWN (0) ... KEYCODE_HOME (3) >+ case AKEYCODE_BACK: return NS_VK_ESCAPE; I wonder, this is odd mapping, but not scope of this bug. (Anyway, I think that this key won't be exposed to the web.) >+ case AKEYCODE_EQUALS: return NS_VK_EQUALS; >+ case AKEYCODE_LEFT_BRACKET: return NS_VK_OPEN_BRACKET; >+ case AKEYCODE_RIGHT_BRACKET: return NS_VK_CLOSE_BRACKET; >+ case AKEYCODE_BACKSLASH: return NS_VK_BACK_SLASH; >+ case AKEYCODE_SEMICOLON: return NS_VK_SEMICOLON; >+ // KEYCODE_APOSTROPHE (75) >+ case AKEYCODE_SLASH: return NS_VK_SLASH; These key mapping is really different from Gecko for Desktop. But perhaps, we have no choice around this due to API limitation and new web apps should use KeyboardEvent.key instead. Anyway, out of scope of this bug. >+ case AKEYCODE_MUTE: return NS_VK_VOLUME_MUTE; This is really wrong. KEYCODE_MUTE is for microphone. So, this should be mapped to 0. Anyway, out of scope of this bug. >+ // Needs to confirm the behavior. If the key switches the open state >+ // of Japanese IME (or switches input character between Hiragana and >+ // Roman numeric characters), then, it might be better to use >+ // NS_VK_KANJI which is used for Alt+Zenkaku/Hankaku key on Windows. >+ case AKEYCODE_ZENKAKU_HANKAKU: return 0; Oh, I should do this... >+ case AKEYCODE_KATAKANA_HIRAGANA: return 0; And this. >+ case AKEYCODE_ASSIST: return NS_VK_HELP; I wonder, we should check this mapping of Chromium. >+ // the A key is the action key for gamepad devices. >+ case AKEYCODE_BUTTON_A: return NS_VK_RETURN; Hmm, hacky... >+static void >+InitKeyEvent(WidgetKeyboardEvent& event, int32_t action, int32_t keyCode, >+ int32_t scanCode, int32_t metaState, int64_t time, >+ int32_t domPrintableKeyValue, int32_t repeatCount, int32_t flags) nit: "a" prefix are missing. (It's okay to do it in another patch or another bug.) >+ const uint32_t domKeyCode = ConvertAndroidKeyCodeToDOMKeyCode(keyCode); nit: |const| but without "k" prefix. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Prefixes >+ event.mModifiers = nsWindow::GetModifiers(metaState); >+ event.mKeyCode = domKeyCode; >+ >+ if (event.mMessage != eKeyPress) { >+ ANPEvent pluginEvent; >+ pluginEvent.inSize = sizeof(pluginEvent); >+ pluginEvent.eventType = kKey_ANPEventType; >+ pluginEvent.data.key.action = event.mMessage == eKeyDown >+ ? kDown_ANPKeyAction : kUp_ANPKeyAction; >+ pluginEvent.data.key.nativeCode = keyCode; >+ pluginEvent.data.key.virtualCode = domKeyCode; >+ pluginEvent.data.key.unichar = domPrintableKeyValue; >+ pluginEvent.data.key.modifiers = >+ (metaState & sdk::KeyEvent::META_SHIFT_MASK >+ ? kShift_ANPKeyModifier : 0) | >+ (metaState & sdk::KeyEvent::META_ALT_MASK >+ ? kAlt_ANPKeyModifier : 0); >+ pluginEvent.data.key.repeatCount = repeatCount; >+ event.mPluginEvent.Copy(pluginEvent); >+ } I wonder, we still support plugins on Android? >+ event.mKeyNameIndex = ConvertAndroidKeyCodeToKeyNameIndex( >+ keyCode, action, domPrintableKeyValue); I feel this indent odd. > event.mKeyNameIndex = > ConvertAndroidKeyCodeToKeyNameIndex(keyCode, action, > domPrintableKeyValue); is usual style of us? >+ if (event.mKeyNameIndex == KEY_NAME_INDEX_USE_STRING && >+ domPrintableKeyValue) { >+ event.mKeyValue = char16_t(domPrintableKeyValue); nit: static_cast<char16_t> (easier to find, but up to you)? >+static jni::ObjectArray::LocalRef >+ConvertRectArrayToJavaRectFArray(const nsTArray<LayoutDeviceIntRect>& aRects, >+ const LayoutDeviceIntPoint& aOffset, >+ const CSSToLayoutDeviceScale aScale) >+{ >+ const size_t length = aRects.Length(); Same, |const| but without "k" prefix. >+void >+GeckoEditableSupport::RemoveComposition(RemoveCompositionFlag aFlag) >+{ >+ if (!mDispatcher || !mDispatcher->IsComposing()) { >+ return; >+ } >+ >+ nsEventStatus status = nsEventStatus_eIgnore; >+ >+ NS_ENSURE_SUCCESS_VOID(mDispatcher->BeginNativeInputTransaction()); Currently, we're replacing NS_ENSURE_* with NS_WARN_IF() and return. But it may cause a bug which checks reverted condition. So, keeping this is okay (and shouldn't do that in this bug). >+ mDispatcher->CommitComposition( >+ status, aFlag == CANCEL_IME_COMPOSITION ? &EmptyString() : nullptr); Odd indent too. I don't understand why here are 8 white spaces? >+void >+GeckoEditableSupport::OnKeyEvent(int32_t aAction, int32_t aKeyCode, >+ int32_t aScanCode, int32_t aMetaState, int32_t aKeyPressMetaState, >+ int64_t aTime, int32_t aDomPrintableKeyValue, int32_t aRepeatCount, >+ int32_t aFlags, bool aIsSynthesizedImeKey, >+ jni::Object::Param originalEvent) nit: "a" prefix. >+ nsCOMPtr<nsIWidget> widget(GetWidget()); FYI: Currently, out coding rule recomments |nsFoo foo = anotherFoo;|. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices But I don't mind about this, up to you because I see same code a lot of places and looks like that kungFuDeathGrip is always declared with this style. >+ RefPtr<TextEventDispatcher> dispatcher(mDispatcher ? mDispatcher.get() : >+ widget ? widget->GetTextEventDispatcher() : nullptr); nit: 8 white spaces as indent here too. >+ if (aIsSynthesizedImeKey) { >+ // Keys synthesized by Java IME code are saved in the mIMEKeyEvents >+ // array until the next IME_REPLACE_TEXT event, at which point >+ // these keys are dispatched in sequence. >+ mIMEKeyEvents.AppendElement( >+ mozilla::UniquePtr<WidgetEvent>(event.Duplicate())); nit: 8 white spaces as indent and do you really need |mozilla::| here? Here must be in mozilla::widget. >+ // Only send keypress after keydown. >+ if (msg != eKeyDown) { >+ return; >+ } >+ >+ WidgetKeyboardEvent pressEvent(true, eKeyPress, widget); >+ InitKeyEvent(pressEvent, aAction, aKeyCode, aScanCode, aKeyPressMetaState, >+ aTime, aDomPrintableKeyValue, aRepeatCount, aFlags); >+ >+ if (aIsSynthesizedImeKey) { >+ mIMEKeyEvents.AppendElement( >+ mozilla::UniquePtr<WidgetEvent>(pressEvent.Duplicate())); >+void >+GeckoEditableSupport::SendIMEDummyKeyEvent(nsIWidget* aWidget, EventMessage msg) >+{ >+ nsEventStatus status = nsEventStatus_eIgnore; >+ MOZ_ASSERT(mDispatcher); >+ >+ WidgetKeyboardEvent event(true, msg, aWidget); >+ event.mTime = PR_Now() / 1000; >+ MOZ_ASSERT(event.mKeyCode == 0); >+ NS_ENSURE_SUCCESS_VOID(mDispatcher->BeginNativeInputTransaction()); >+ mDispatcher->DispatchKeyboardEvent(msg, event, status); >+} >+void >+GeckoEditableSupport::AddIMETextChange(const IMETextChange& aChange) >+{ >+ mIMETextChanges.AppendElement(aChange); >+ >+ // We may not be in the middle of flushing, >+ // in which case this flag is meaningless. >+ mIMETextChangedDuringFlush = true; >+ >+ // Now that we added a new range we need to go back and >+ // update all the ranges before that. >+ // Ranges that have offsets which follow this new range >+ // need to be updated to reflect new offsets >+ const int32_t delta = aChange.mNewEnd - aChange.mOldEnd; nit: |const| without "k" prefix. >+void >+GeckoEditableSupport::FlushIMEChanges(FlushChangesFlag aFlags) >+{ >+ // Only send change notifications if we are *not* masking events, >+ // i.e. if we have a focused editor, >+ NS_ENSURE_TRUE_VOID(!mIMEMaskEventsCount); >+ >+ nsCOMPtr<nsIWidget> widget(GetWidget()); >+ NS_ENSURE_TRUE_VOID(widget); >+ >+ struct TextRecord { nit: |{| should be next line under "s" of "struct". >+ nsString text; nsAutoString? >+ int32_t start; >+ int32_t oldEnd; >+ int32_t newEnd; >+ }; >+ AutoTArray<TextRecord, 4> textTransaction; >+ if (mIMETextChanges.Length() > textTransaction.Capacity()) { >+ textTransaction.SetCapacity(mIMETextChanges.Length()); >+ } I think that you can call SetCapacity() without the check. It never shrinks the capacity. https://dxr.mozilla.org/mozilla-central/rev/5069348353f8fc1121e632e3208da33900627214/xpcom/ds/nsTArray-inl.h#118 (Look like that |ShrinkCapacity()| is called only when the array becomes empty by a call of Shift() or Compact() is called.) >+ auto shouldAbort = [=] (bool force) -> bool { I wonder, |focus| should have "a" prefix? But I don't find any declaration of lambda expression in out coding rules. >+ for (const IMETextChange &change : mIMETextChanges) { nit: const without "k" prefix. >+ if (change.mStart == change.mOldEnd && >+ change.mStart == change.mNewEnd) { nit: odd indent, second condition should start with same column as previous line. And perhaps, adding |IsChanged()| method to IMETextChange is easier to read (may be out of scope of this bug, though). >+ WidgetQueryContentEvent event(true, eQueryTextContent, widget); >+ >+ if (change.mNewEnd != change.mStart) { Perhaps, adding |NewLengh()| to IMETextChange and comparing with 0 is easier to read (may be out of scope of this bug, though). >+ event.InitForQueryTextContent(change.mStart, >+ change.mNewEnd - change.mStart); Then, you can use it here. >+ selStart = int32_t(event.GetSelectionStart()); >+ selEnd = int32_t(event.GetSelectionEnd()); static_cast<int32_t> (easier to find, but up to you)? >+ for (const TextRecord& record : textTransaction) { nit: const without "k" prefix. >+void >+GeckoEditableSupport::UpdateCompositionRects() >+ auto rects = ConvertRectArrayToJavaRectFArray( >+ textRects.mReply.mRectArray, >+ widget->WidgetToScreenOffset(), >+ widget->GetDefaultScale()); nit: odd indent here. >+void >+GeckoEditableSupport::OnImeReplaceText(int32_t aStart, int32_t aEnd, >+ jni::String::Param aText) >+ nsString string(aText->ToString()); nsAutoString? >+ const bool composing = !mIMERanges->IsEmpty(); nit: const without "k" prefix. >+ if (!mIMEKeyEvents.IsEmpty() || !mDispatcher->IsComposing() || >+ uint32_t(aStart) != composition->NativeOffsetOfStartComposition() || >+ uint32_t(aEnd) != composition->NativeOffsetOfStartComposition() + >+ composition->String().Length()) >+ { nit: put this to the end of previous line. nit: static_cast<uint32_t> (up to you)? >+ // Use text selection to set target position(s) for >+ // insert, or replace, of text. >+ WidgetSelectionEvent event(true, eSetSelection, widget); >+ event.mOffset = uint32_t(aStart); >+ event.mLength = uint32_t(aEnd - aStart); nit: static_cast<uint32_t> (up to you)? >+ event.mExpandToClusterBoundary = false; >+ event.mReason = nsISelectionListener::IME_REASON; >+ widget->DispatchEvent(&event, status); So, I still think that this should store selection at NOTIFY_IME_OF_SELECTION and queries selection only when the cache is dirty. But it's okay to put off this discussion to another bug. >+ if (!mIMEKeyEvents.IsEmpty()) { >+ for (uint32_t i = 0; i < mIMEKeyEvents.Length(); i++) { >+ const auto event = mIMEKeyEvents[i]->AsKeyboardEvent(); >+ // widget for duplicated events is initially nullptr. >+ event->mWidget = widget; >+ >+ if (event->mMessage == eKeyPress) { >+ mDispatcher->MaybeDispatchKeypressEvents(*event, status); >+ } else { >+ mDispatcher->DispatchKeyboardEvent( >+ event->mMessage, *event, status); nit: odd indent. >+ } I think that you should check if nsIWidget::Destroyed() is true here. If it's true, you can break this loop immediately. >+ } >+ mIMEKeyEvents.Clear(); >+ return; >+ } >+ >+ if (aStart != aEnd) { >+ // Perform a deletion first. >+ WidgetContentCommandEvent event( >+ true, eContentCommandDelete, widget); >+ event.mTime = PR_Now() / 1000; >+ widget->DispatchEvent(&event, status); You should check if nsIWidget::Destroyed() returns true here. (FYI: you don't need to mind this after WidgetQueryEvent dispatching.) >+ } >+ nit: odd empty line. >+ } else if (composition->String().Equals(string)) { >+ /* If the new text is the same as the existing composition text, >+ * the NS_COMPOSITION_CHANGE event does not generate a text >+ * change notification. However, the Java side still expects >+ * one, so we manually generate a notification. */ >+ IMETextChange dummyChange; >+ dummyChange.mStart = aStart; >+ dummyChange.mOldEnd = dummyChange.mNewEnd = aEnd; >+ AddIMETextChange(dummyChange); >+ } >+ >+ if (mInputContext.mMayBeIMEUnaware) { Perhaps, mInputContext.mMayBeIMEUnaware should be checked in TextEventDispatcher. I'll file it. Keep this for now. >+ SendIMEDummyKeyEvent(widget, eKeyDown); Check if nsIWidget::Destroyed() returns true here. >+ } >+ >+ if (composing) { >+ mDispatcher->SetPendingComposition(string, mIMERanges); >+ mDispatcher->FlushPendingComposition(status); >+ // Ensure IME ranges are empty. >+ mIMERanges->Clear(); >+ } else if (!string.IsEmpty() || mDispatcher->IsComposing()) { >+ mDispatcher->CommitComposition(status, &string); >+ } Check if nsIWidget::Destroyed() returns true here. >+ >+ if (mInputContext.mMayBeIMEUnaware) { >+ SendIMEDummyKeyEvent(widget, eKeyUp); Please add a comment here like: > SendIMEDummyKeyEvent(widget, eKeyUp); > // Be careful, the widget might have gone during dispatching an eKeyUp > // event. >+void >+GeckoEditableSupport::OnImeAddCompositionRange( >+ int32_t aStart, int32_t aEnd, int32_t aRangeType, int32_t aRangeStyle, >+ int32_t aRangeLineStyle, bool aRangeBoldLine, int32_t aRangeForeColor, >+ int32_t aRangeBackColor, int32_t aRangeLineColor) >+ range.mRangeStyle.mForegroundColor = >+ ConvertAndroidColor(uint32_t(aRangeForeColor)); >+ range.mRangeStyle.mBackgroundColor = >+ ConvertAndroidColor(uint32_t(aRangeBackColor)); >+ range.mRangeStyle.mUnderlineColor = >+ ConvertAndroidColor(uint32_t(aRangeLineColor)); nit: static_cast<uint32_t>? but up to you. >+ mIMERanges->AppendElement(range); I wonder, you could add the range to TextEventDispatcher with creating a method like |AppendClauseToPendingComposition(const TextRange& aRange)|. But for implementing it, we need some additional work because |TextEventDispatcher::PendingComposition::Set()| does a lot of things for safety. So, for now, keeping managing the clauses with mIMERanges is okay. >+void >+GeckoEditableSupport::OnImeUpdateComposition(int32_t aStart, int32_t aEnd) >+{ >+ // A composition with no ranges means we want to set the selection. >+ if (mIMERanges->IsEmpty()) { >+ MOZ_ASSERT(aStart >= 0 && aEnd >= 0); >+ RemoveComposition(); >+ >+ WidgetSelectionEvent selEvent(true, eSetSelection, widget); >+ selEvent.mOffset = std::min(aStart, aEnd); >+ selEvent.mLength = std::max(aStart, aEnd) - selEvent.mOffset; >+ selEvent.mReversed = aStart > aEnd; >+ selEvent.mExpandToClusterBoundary = false; >+ widget->DispatchEvent(&selEvent, status); Although, this is not a scope of this bug, eSetSelection causes "selectinchange" event now. Therefore, web page can change selection again. In such case, IME on Android may be confused. >+ /* >+ Update the composition from aStart to aEnd using >+ information from added ranges. This is only used for >+ visual indication and does not affect the text content. >+ Only the offsets are specified and not the text content >+ to eliminate the possibility of this event altering the >+ text content unintentionally. >+ */ Hmm, odd indent (and odd style for me) comment. >+ nsString string; nsAutoString. >+ RefPtr<TextComposition> composition(GetComposition()); >+ MOZ_ASSERT(!composition || !composition->IsEditorHandlingEvent()); >+ >+ if (!mDispatcher->IsComposing() || >+ uint32_t(aStart) != composition->NativeOffsetOfStartComposition() || >+ uint32_t(aEnd) != composition->NativeOffsetOfStartComposition() + >+ composition->String().Length()) >+ { Put this to the end of the previous line. >+ // Only start new composition if we don't have an existing one, >+ // or if the existing composition doesn't match the new one. >+ RemoveComposition(); >+ >+ { >+ WidgetSelectionEvent event(true, eSetSelection, widget); >+ event.mOffset = uint32_t(aStart); >+ event.mLength = uint32_t(aEnd - aStart); >+ event.mExpandToClusterBoundary = false; >+ event.mReason = nsISelectionListener::IME_REASON; >+ widget->DispatchEvent(&event, status); nsIWidget::Destroyed() should be checked here. >+nsresult >+GeckoEditableSupport::NotifyIME(TextEventDispatcher* aTextEventDispatcher, >+ const IMENotification& aNotification) >+ mEditable->NotifyIME( >+ GeckoEditableListener::NOTIFY_IME_OF_FOCUS); Looks odd indent to me. >+void >+GeckoEditableSupport::OnRemovedFrom(TextEventDispatcher* aTextEventDispatcher) >+{ >+} This may be called when the widget isn't available. So, in such case, you might need to discard composition in IME. >+void >+GeckoEditableSupport::WillDispatchKeyboardEvent( >+ TextEventDispatcher* aTextEventDispatcher, >+ WidgetKeyboardEvent& aKeyboardEvent, uint32_t aIndexOfKeypress, >+ void* aData) >+{ >+} Yes, perhaps, Android doesn't need to do anything with this method. >+InputContext >+GeckoEditableSupport::GetInputContext() >+{ >+ InputContext context = mInputContext; >+ context.mIMEState.mOpen = IMEState::OPEN_STATE_NOT_SUPPORTED; >+ return context; >+} >+ >+} >+} Please add comment what those |}| mean, e.g.,: >} // namespace widget >} // namespace mozilla >+ // RAII helper class that automatically sends an event reply through >+ // OnImeSynchronize, as required by events like OnImeReplaceText. >+ class AutoIMESynchronize { nit: Put the |{| to the next line under "c" of "class". And I think that this class name should be |AutoIMESynchronizer|. >+ struct IMETextChange final { nit: Put the |{| to the next line under "s" of "struct". >+ enum FlushChangesFlag { nit: Put the |{| to the ext line under "e" of "enum". And using enum class is better for type-safe but up to you. >+ // Not retrying. >+ FLUSH_FLAG_NONE, (If you use enum class, this can be just "eNone".) >+ enum RemoveCompositionFlag { Same, move |{| and possible to use enum class. >+ RefPtr<TextEventDispatcher> mDispatcher; Although, I don't check the lifetime of this class, please ensure that mDispatcher and its widget are NOT referenced by each other. Sorry for the delay to review and thank you for your great work!!

Bug 1137567 - 1. Allow dispatching key events during composition; r=esawin We potentially dispatch key events during composition to provide compatibility for pages that only listen to key events. Bug 1137567 - 2. Allow keyboard events in DispatchInputEvent when not on APZ thread; r=rbarker We use nsIWidget::DispatchInputEvent to dispatch our keyboard events on the Gecko thread, which on Android is not the APZ controller thread. We should allow these events to pass instead of crashing. Bug 1137567 - 3. Add GeckoEditableSupport class to support TextEventDispatcher; r=masayuki Add a separate GeckoEditableSupport class, which implements TextEventDispatcherListener and uses TextEventDispatcher for IME operations. The new class is entirely separate from nsWindow to allow it to be independently used in content processes as well. Most of the code is copied from nsWindow::GeckoViewSupport, and adapted to use TextEventDispatcher. Bug 1137567 - 4. Make nsWindow::WindowPtr available for outside classes; r=snorp Make nsWindow::WindowPtr available not just for classes inside nsWindow but for outside classes as well. Also, add support for RefPtr native objects to nsWindow::NativePtr. Bug 1137567 - 5. Use GeckoEditableSupport in nsWindow; r=esawin Use the new GeckoEditableSupport class in nsWindow to replace the previous code in nsWindow::GeckoViewSupport. GeckoEditable native methods now go to GeckoEditableSupport instead of GeckoViewSupport. Several native methods in GeckoEditable are changed from dispatchTo="proxy" to dispatchTo="gecko", because we no longer need the special nsWindow::WindowEvent wrapper for our native calls.

Pushed by nchen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/13bd2c36c194 Make nsWindow for Android use TextEventDispatcher; r=esawin r=rbarker r=masayuki r=snorp

I had to back this out for android failures like https://treeherder.mozilla.org/logviewer.html#?job_id=80075626&repo=mozilla-inbound&lineNumber=1843 https://hg.mozilla.org/integration/mozilla-inbound/rev/0ba0e2afff97

setAndObserveCompositionPref in test_assign_event_data.html does not invoke the callback if the pref is already set. This patch changes it to use SpecialPowers.pushPrefEnv so the callback is always invoked.

Comment on attachment 8841769 [details] [diff] [review] 6. Use pushPrefEnv in test_assign_event_data.html (v1) Could fix bug 1285414 too?

Pushed by nchen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4048d3a53107 Make nsWindow for Android use TextEventDispatcher; r=esawin r=rbarker r=masayuki r=snorp

Storing UniquePtrs to WidgetEvents in a member variable is a bit worrisome. What if the member variable is cleared while dispatching event?