Closed
Bug 1137572
Opened 9 years ago
Closed 8 years ago
Add some useful methods to TextEventDispatcher for native key and IME handlers
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
(Keywords: inputmethod)
Attachments
(9 files, 6 obsolete files)
7.47 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
11.97 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
5.03 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
1.56 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
15.42 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
part.7 Add TextEventDispatcherListener::WillDispatchKeyboardEvent() for easier to maintain (r=smaug)
13.52 KB,
patch
|
Details | Diff | Splinter Review | |
15.35 KB,
patch
|
Details | Diff | Splinter Review | |
2.12 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
21.42 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
Currently, TextEventDispatcher supports only TextInputProcessor. However, native key event handlers need to include additional information to keyboard events. E.g., alternative charCode values, shortcut key information defined by platform and some data for plugins.
Assignee | ||
Updated•9 years ago
|
Whiteboard: [leave open]
Assignee | ||
Comment 1•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a3332326a73
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3823f6a42b74
Assignee | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae7ebb773b64
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=67387988ebf5
Assignee | ||
Comment 5•8 years ago
|
||
For making TextEventDispatcher work with native IME/keyboard event handlers, it should be able to create input transaction for that. I.e., there should be 3 types of input transaction: 1. for native event handler 2. for TIP with IME-application on Firefox OS 3. for TIP with automated tests
Attachment #8720190 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•8 years ago
|
||
Currently, nsIWidget::NotifyIME() tries to notify TextEventDispatcher (only when there is an instance for the widget), after that, nsBaseWidget::NotifyIMEInternal(). If native IME event handler uses TextEventDispatcher, TextEventDispatcherListener::NotifyIME() should handle the notification even if there is no instance of the TextEventDispatcher because NOTIFY_IME_OF_FOCUS may be necessary for native IME handlers (e.g., TSF needs to create a context for focused editor *before* creating TextEventDispatcher). Therefore, we need to change nsBaseWidget, it should create TextEventDispatcher instance if it receives notification to IME. And TextEventDispatcher should always notify TextEventDispatcherListener even when there is no input transaction or an input transaction for TIP. For solving this issue, it's the best to create and implement nsIWidget::GetNativeTextEventDispatcherListener() which returns TextEventDispatcherListener instance for native IME/keyboard event handlers. Therefore, it does make sense to retrieve the listener per widget since TextEventDispatcher is created per widget. Note that GetNativeTextEventDispatcherListener() may return nullptr if: 1. the platform widget doesn't use TextEventDispatcher to handle native IME/keyboard events 2. the widget won't handle input events, e.g., nsCocoaWindow.
Attachment #8720198 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8720198 -
Flags: superreview?(bugs)
Assignee | ||
Comment 7•8 years ago
|
||
When TIP generates composition, TextEventDispatcher should set different input context from native input context since it make XP part can distinguish if a composition event is for active composition. With this patch, even if native IME/keyboard event handler uses TextEventDispatcher, each widget can return proper IME context for native composition.
Attachment #8720203 -
Flags: review?(bugs)
Assignee | ||
Comment 8•8 years ago
|
||
Because of input transaction type, TextEventDispatcher can decide if composition/keyboard events are dispatched via nsIWidget::DispatchInputEvent() for APZ. So, we can make TIP and TextEventDispatcher simpler.
Attachment #8720204 -
Flags: review?(bugs)
Assignee | ||
Comment 9•8 years ago
|
||
TextEventDispatcher::AppendClauseToPendingComposition() assumes that the user can add clauses from the first clause to the last clause without any overlap. However, native IME API may not guarantee that. So, we should use TextRangeArray which is created by current native IME handler since the code is tested by a lot of IMEs. SetPendingComposition() allows to set TextRangeArray directly.
Attachment #8720205 -
Flags: review?(bugs)
Assignee | ||
Comment 10•8 years ago
|
||
TextEventDispatcher doesn't support to copy some members of WidgetKeyboardEvent which are not necessary for TIP. We should copy them when the input transaction for native input.
Attachment #8720206 -
Flags: review?(bugs)
Assignee | ||
Comment 11•8 years ago
|
||
Currently, TextEventDispatcher corrects some members (e.g., keyCode, charCode) automatically. But it's not enough for native keyboard event handling. 1. If some modifier is pressed, charCode cannot be computed from mKeyValue. E.g., When Ctrl key is pressed. In such case, platform specific code needs to decide better charCode. 2. If two or more keypress events are fired for a keydown event, the platform specific code needs to proper modifier state and alternative char codes. Therefore, TextEventDispatcher should call a callback after initializes WidgetKeyboardEvent and platform specific code corrects some members for each platform's specific rules. This is currently enough when one of Ctrl, Alt, Meta or OS is active and the event is keypress. We need to call it at keydown and keyup in follow up bug, though. I think that the callback's name is WillDispatchKeyboardEvent() is the best because it's not used in any platforms and not similar to other methods like Init*().
Attachment #8720208 -
Flags: review?(bugs)
Assignee | ||
Comment 12•8 years ago
|
||
On Windows and Linux, native IME/keyboard event handler sets time and timeStamp of WidgetEvent to the time of native event. So, TextEventDispatcher needs to have optional argument for that in some methods. But it's too ugly we set two optional arguments. Therefore, I'd like to create a struct which has time and timeStamp. For making the relation between WidgetEvent and the new struct simpler, it should be base class of WidgetEvent. I name it WidgetEventTime. (EventTime is conflict with OS X specific type...)
Attachment #8720210 -
Flags: review?(bugs)
Assignee | ||
Comment 13•8 years ago
|
||
During a call of WillDispatchKeyboardEvent(), WidgetKeyboardEvent's keyCode and charCode become different. So, after that, TextEventDispatcher should correct them again for conforming common rules. # I'll post patches for Windows, Linux and Mac later.
Attachment #8720212 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Whiteboard: [leave open]
Assignee | ||
Comment 14•8 years ago
|
||
I posted patches for Windows specific code to bug 1137561. Please check it for checking why each patch of this bug is necessary.
Updated•8 years ago
|
Attachment #8720198 -
Flags: superreview?(bugs) → superreview+
Assignee | ||
Comment 15•8 years ago
|
||
FYI: I posted patches for Mac into bug 1137563 and for Linux into bug 1137565. The patches for Linux are the simplest and easiest to read.
Assignee | ||
Comment 16•8 years ago
|
||
I wonder, currently, I'm moving some platform specific code to XP level (i.e., into TextEventDispatcher). This will guarantee consistent behavior of whether a keyboard event should be dispatched or not between platforms. Next, I guess that some members of native IME handler can be replaced with referring TextEventDispatcher's members. I'll try to do that Q2 or later. Finally, I wonder, if we add some callback methods to TextEventDispatcherListener, we can get better consistency. E.g., when native key event is received by widget, it should be notified to TextEventDispatcher and store the information. Then, when it needs to dispatch a keyboard event for the native event, TextEventDispatcher can call another callback for initializing WidgetKeyboardEvent. The ideal goal is, only changing TextEventDispatchr (and small part of each platform widget) can fix bugs related to UI Events. But I don't want to make the performance worse, of course. So, I won't add a lot of callback methods.
Updated•8 years ago
|
Attachment #8720190 -
Flags: review?(bugs) → review+
Comment 17•8 years ago
|
||
Comment on attachment 8720198 [details] [diff] [review] part.2 Add nsIWidget::GetNativeTextEventDispatcherListener() for TextEventDispatcher::NotifyIME() > TextEventDispatcher::NotifyIME(const IMENotification& aIMENotification) > { >+ nsresult rv = NS_ERROR_NOT_IMPLEMENTED; >+ >+ // First, send the notification to current input transaction's listener. > nsCOMPtr<TextEventDispatcherListener> listener = do_QueryReferent(mListener); >- if (!listener) { >- return NS_ERROR_NOT_IMPLEMENTED; >+ if (listener) { >+ rv = listener->NotifyIME(this, aIMENotification); >+ // If the listener isn't available, it means that it cannot handle the >+ // notification or request for now. I don't understand this. We clearly have a listener. So it is available. >+ // NS_ERROR_NOT_IMPLEMENTED because it's not implemented at such moment. >+ if (rv == NS_ERROR_NOT_AVAILABLE && >+ mInputTransactionType != eNativeInputTransaction) { >+ rv = NS_ERROR_NOT_IMPLEMENTED; >+ } and don't then understand all this either. Why special case eNativeInputTransaction and all that. > } >- nsresult rv = listener->NotifyIME(this, aIMENotification); >- // If the listener isn't available, it means that it cannot handle the >- // notification or request for now. In this case, we should return >- // NS_ERROR_NOT_IMPLEMENTED because it's not implemented at such moment. >- if (rv == NS_ERROR_NOT_AVAILABLE) { >- return NS_ERROR_NOT_IMPLEMENTED; >+ >+ if (mInputTransactionType == eNativeInputTransaction || !mWidget) { >+ return rv; > } >- return rv; >+ >+ // If current input transaction isn't for native event handler, we may need >+ // to send the notification to the native text event dispatcher listener. >+ nsCOMPtr<TextEventDispatcherListener> nativeListener = >+ mWidget->GetNativeTextEventDispatcherListener(); I must be missing something here. I would imagine nativeListener to be used only when mInputTransactionType == eNativeInputTransaction but apparently that isn't the case. So I'm lost with the setup here. >+ if (!nativeListener) { >+ return rv; >+ } >+ switch (aIMENotification.mMessage) { >+ case REQUEST_TO_COMMIT_COMPOSITION: >+ case REQUEST_TO_CANCEL_COMPOSITION: >+ // It's not necessary to notify native IME of requests. >+ return rv; >+ default: { >+ // Even if current input transaction's listener returns NS_OK or >+ // something, we need to notify native IME of notifications because >+ // when user typing after TIP does something, the changed information >+ // is necessary for them. >+ nsresult rv2 = >+ nativeListener->NotifyIME(this, aIMENotification); >+ // But return the result from current listener except when the >+ // notification isn't handled. >+ return rv == NS_ERROR_NOT_IMPLEMENTED ? rv2 : rv; >+ } >+ } How are these changes related to adding GetNativeTextEventDispatcherListener to nsIWidget? >+NS_IMETHODIMP_(TextEventDispatcherListener*) >+nsBaseWidget::GetNativeTextEventDispatcherListener() >+{ >+ // TODO: If all platforms support to use TextEventDispatcher for handling "if all the platforms supported use of TextEventDispatcher ..." >+ // native IME and keyboard events, this method should be removed since >+ // in such case, this is overridden by all subclasses. all the subclasses I'm rather lost with some of the changes here.
Attachment #8720198 -
Flags: review?(bugs) → review-
Updated•8 years ago
|
Attachment #8720203 -
Flags: review?(bugs) → review+
Comment 18•8 years ago
|
||
Comment on attachment 8720204 [details] [diff] [review] part.4 TextEventDispatcher::DispatchInputEvent() should decide if dispatches events with nsIWidget::DispatchInputEvent() with input transaction type > if (aForTests) { >- rv = dispatcher->BeginTestInputTransaction(this); >+ bool isAPZAware = gfxPrefs::TestEventsAsyncEnabled(); So this is in practice always false. I know I reviewed the patch which made us use this method, but is it actually right? only window_wheeltransaction.xul ever sets the pref. > TextEventDispatcher::DispatchInputEvent(nsIWidget* aWidget, > WidgetInputEvent& aEvent, >- nsEventStatus& aStatus, >- DispatchTo aDispatchTo) >+ nsEventStatus& aStatus) > { > RefPtr<TextEventDispatcher> kungFuDeathGrip(this); > nsCOMPtr<nsIWidget> widget(aWidget); > mDispatchingEvent++; > > // If the event is dispatched via nsIWidget::DispatchInputEvent(), it > // sends the event to the parent process first since APZ needs to handle it > // first. However, some callers (e.g., keyboard apps on B2G and tests > // expecting synchronous dispatch) don't want this to do that. > nsresult rv = NS_OK; >- if (aDispatchTo == eDispatchToParentProcess) { >+ if (mInputTransactionType == eNativeInputTransaction || >+ mInputTransactionType == eAsyncTestInputTransaction) { > aStatus = widget->DispatchInputEvent(&aEvent); > } else { > rv = widget->DispatchEvent(&aEvent, aStatus); > } This code becomes close to impossible to follow. Why eNativeInputTransaction and eAsyncTestInputTransaction call DispatchInputEvent, but all the other cases (whatever they are), uses DispatchEvent. switch-case would make this a tad easier to read, but even then this would be harder to follow. The current comment + aDispatchTo == eDispatchToParentProcess check is very clear. > enum InputTransactionType : uint8_t > { > // No input transaction has been started. > eNoInputTransaction, >- // Input transaction for native IME or keyboard event handler. >+ // Input transaction for native IME or keyboard event handler. Note that >+ // keyboard events may be dispatched via parent process if there is. > eNativeInputTransaction, >- // Input transaction for automated tests. >- eTestInputTransaction, >- // Input transaction for Others (must be IME on B2G). >+ // Input transaction for automated tests which are APZ-aware. Note that >+ // keyboard events may be dispatched via parent process if there is. >+ eAsyncTestInputTransaction, >+ // Input transaction for automated tests which assume events are fired >+ // synchronously. I.e., keyboard events are always dispatched in the >+ // current process. >+ eSyncTestInputTransaction, Could we perhaps call this eSameProcessSyncTestInputTransaction or something like that. I know it is long, but the 'sync' part is kind of important here. >+ // Input transaction for Others (must be IME on B2G). Events are fired >+ // synchronously because TextInputProcessor which is the only user of >+ // this input transaction type supports only keyboard apps on B2G. >+ // Keyboard apps on B2G doesn't want to dispatch keyboard events to >+ // chrome process. Therefore, this should dispatch key events only in >+ // the current process. > eOtherInputTransaction and this could be eSameProcessSyncInputTransaction perhaps?
Attachment #8720204 -
Flags: review?(bugs) → review-
Comment 19•8 years ago
|
||
Comment on attachment 8720205 [details] [diff] [review] part.5 Implement TextEventDispatcher::SetPendingComposition() for some platforms whose clause information may overlap each other or the order may not be from start to the end >+TextEventDispatcher::PendingComposition::Set(const nsAString& aString, >+ const TextRangeArray* aRanges) took a bit time to understand this part. but fine.
Attachment #8720205 -
Flags: review?(bugs) → review+
Updated•8 years ago
|
Attachment #8720206 -
Flags: review?(bugs) → review+
Comment 20•8 years ago
|
||
Comment on attachment 8720208 [details] [diff] [review] part.7 Add TextEventDispatcherListener::WillDispatchKeyboardEvent() for easier to maintain >+ // Request the alternative char codes for the key event. >+ // XXX Currently, they are necessary only when the event is eKeyPress. >+ keyEvent.alternativeCharCodes.Clear(); >+ if (aMessage == eKeyPress && >+ (keyEvent.IsControl() || keyEvent.IsAlt() || >+ keyEvent.IsMeta() || keyEvent.IsOS())) { >+ nsCOMPtr<TextEventDispatcherListener> listener = >+ do_QueryReferent(mListener); >+ if (NS_WARN_IF(!listener)) { >+ return false; >+ } >+ listener->WillDispatchKeyboardEvent(this, keyEvent, aIndexOfKeypress, >+ aData); >+ } Nit, I would probably write this if (listener) { listener->WillDispatchKeyboardEvent(this, keyEvent, aIndexOfKeypress, aData); and remove the early return. Just a bit easier to read, IMO, and it isn't really an essential part of the method whether or not there is listener. >+ * @param aData The pointer which was specified at calling >+ * the method of TextEventDispatcher. So we need some example here explaining what aData might be.
Attachment #8720208 -
Flags: review?(bugs) → review+
Comment 21•8 years ago
|
||
Comment on attachment 8720212 [details] [diff] [review] part.9 TextEventDispatcher::DispatchKeyboardEventInternal() shouldn't clear keyCode of keypress event if the key is a printable key I'm lost with this. Isn't the whole idea of WillDispatchKeyboardEvent that it can update keyCode or charCode values, or in fact any properties in the event. This looks like a hack. WillDispatchKeyboardEvent implementations shouldn't do unexpected changes. Could we just assert here that the implementations don't do anything unexpected. Feel free to explain why we want this and ask review again, but as of now this really looks like a hack around some WillDispatchKeyboardEvent implementation.
Attachment #8720212 -
Flags: review?(bugs) → review-
Comment 22•8 years ago
|
||
Comment on attachment 8720210 [details] [diff] [review] part.8 Callers of methods to dispatch composition events of TextEventDispatcher should be able to specify specific time/timeStamp >+ WidgetEventTime(uint64_t aTime, >+ TimeStamp aTimeStamp) nit, align the latter param under the first one
Attachment #8720210 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 23•8 years ago
|
||
(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #17) > > TextEventDispatcher::NotifyIME(const IMENotification& aIMENotification) > > { > >+ nsresult rv = NS_ERROR_NOT_IMPLEMENTED; > >+ > >+ // First, send the notification to current input transaction's listener. > > nsCOMPtr<TextEventDispatcherListener> listener = do_QueryReferent(mListener); > >- if (!listener) { > >- return NS_ERROR_NOT_IMPLEMENTED; > >+ if (listener) { > >+ rv = listener->NotifyIME(this, aIMENotification); > >+ // If the listener isn't available, it means that it cannot handle the > >+ // notification or request for now. > I don't understand this. We clearly have a listener. So it is available. > > >+ // NS_ERROR_NOT_IMPLEMENTED because it's not implemented at such moment. > >+ if (rv == NS_ERROR_NOT_AVAILABLE && > >+ mInputTransactionType != eNativeInputTransaction) { > >+ rv = NS_ERROR_NOT_IMPLEMENTED; > >+ } > and don't then understand all this either. Why special case > eNativeInputTransaction and all that. Okay, I removed this hack. Instead of that, I changed the result of TextInputProcessor::NotifyIME(). The hack was necessary only for TIP. This must be easier to read for you. > >+ // If current input transaction isn't for native event handler, we may need > >+ // to send the notification to the native text event dispatcher listener. > >+ nsCOMPtr<TextEventDispatcherListener> nativeListener = > >+ mWidget->GetNativeTextEventDispatcherListener(); > > I must be missing something here. I would imagine nativeListener to be used > only when > mInputTransactionType == eNativeInputTransaction > but apparently that isn't the case. So I'm lost with the setup here. I had believed so, but I realized that when I tested on Windows. E.g., on Windows, NOTIFY_IME_OF_FOCUS is necessary to notify TSF at new editor getting focus. However, the may be no transaction or other type transaction because input transaction is created immediately before dispatching keyboard event or composition event. Therefore, even if native input transaction isn't there, native IME handler needs to be notified notifications from content. I explained this issue in this new patch. > >+ if (!nativeListener) { > >+ return rv; > >+ } > >+ switch (aIMENotification.mMessage) { > >+ case REQUEST_TO_COMMIT_COMPOSITION: > >+ case REQUEST_TO_CANCEL_COMPOSITION: > >+ // It's not necessary to notify native IME of requests. > >+ return rv; > >+ default: { > >+ // Even if current input transaction's listener returns NS_OK or > >+ // something, we need to notify native IME of notifications because > >+ // when user typing after TIP does something, the changed information > >+ // is necessary for them. > >+ nsresult rv2 = > >+ nativeListener->NotifyIME(this, aIMENotification); > >+ // But return the result from current listener except when the > >+ // notification isn't handled. > >+ return rv == NS_ERROR_NOT_IMPLEMENTED ? rv2 : rv; > >+ } > >+ } > How are these changes related to adding GetNativeTextEventDispatcherListener > to nsIWidget? So, nsBaseWidget::NotifyIMEInternal() shouldn't be used anymore after widget starts to use TextEventDispatcher because the notification should be received with TextEventDispatcherListener. This design becomes possible with nsIWidget::GetNativeTextEventDispatcherListener(). So, yes, it's related, but I could've separated the patch, sorry. FYI: NotifyIME() is now: > nsIWidget::NotifyIME() -> > TextEventDispatcher::NotifyIME() -> > TextEventDispatcherListener::Notify() -> > (back to nsIWidget::NotifyIME()) -> > nsBaseWidget::NotifyIMEInternal() This is changed by this patch: > nsIWidget::NotifyIME() -> > TextEventDispatcher::NotifyIME() -> > TextEventDispatcherListener::Notify() -> >+ (if the active listener isn't for native input transaction, >+ TextEventDispatcherListener::Notify() ->) > (back to nsIWidget::NotifyIME()) -> > nsBaseWidget::NotifyIMEInternal() In the future: > nsIWidget::NotifyIME() -> > TextEventDispatcher::NotifyIME() -> > TextEventDispatcherListener::Notify() -> > (if the active listener isn't for native input transaction, > TextEventDispatcherListener::Notify() ->) >- (back to nsIWidget::NotifyIME()) -> >- nsBaseWidget::NotifyIMEInternal() I.e., NotifyIMEInternal() won't be implemented on the widgets which use TextEventDispatcher. Then, they won't receive notifications to IME without TextEventDispatcherListener for native event handlers.
Attachment #8720198 -
Attachment is obsolete: true
Attachment #8723419 -
Flags: review?(bugs)
Assignee | ||
Comment 24•8 years ago
|
||
(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #18) > > if (aForTests) { > >- rv = dispatcher->BeginTestInputTransaction(this); > >+ bool isAPZAware = gfxPrefs::TestEventsAsyncEnabled(); > So this is in practice always false. I know I reviewed the patch which made > us use this method, but > is it actually right? only window_wheeltransaction.xul ever sets the pref. Honestly, I don't know. It was implemented by bug 1146243. http://hg.mozilla.org/mozilla-central/rev/884d44a41ac7 Anyway, this shouldn't be in the scope of this bug. > > TextEventDispatcher::DispatchInputEvent(nsIWidget* aWidget, > > WidgetInputEvent& aEvent, > >- nsEventStatus& aStatus, > >- DispatchTo aDispatchTo) > >+ nsEventStatus& aStatus) > > { > > RefPtr<TextEventDispatcher> kungFuDeathGrip(this); > > nsCOMPtr<nsIWidget> widget(aWidget); > > mDispatchingEvent++; > > > > // If the event is dispatched via nsIWidget::DispatchInputEvent(), it > > // sends the event to the parent process first since APZ needs to handle it > > // first. However, some callers (e.g., keyboard apps on B2G and tests > > // expecting synchronous dispatch) don't want this to do that. > > nsresult rv = NS_OK; > >- if (aDispatchTo == eDispatchToParentProcess) { > >+ if (mInputTransactionType == eNativeInputTransaction || > >+ mInputTransactionType == eAsyncTestInputTransaction) { > > aStatus = widget->DispatchInputEvent(&aEvent); > > } else { > > rv = widget->DispatchEvent(&aEvent, aStatus); > > } > This code becomes close to impossible to follow. Why eNativeInputTransaction > and eAsyncTestInputTransaction call > DispatchInputEvent, but all the other cases (whatever they are), uses > DispatchEvent. > switch-case would make this a tad easier to read, but even then this would > be harder to follow. The current > comment + aDispatchTo == eDispatchToParentProcess check is very clear. Okay, I created ShouldSendInputEventToAPZ() method. I believe that this makes the code easier to read and safer for maintaining.
Attachment #8720204 -
Attachment is obsolete: true
Attachment #8723432 -
Flags: review?(bugs)
Assignee | ||
Comment 25•8 years ago
|
||
Attachment #8720208 -
Attachment is obsolete: true
Assignee | ||
Comment 26•8 years ago
|
||
Attachment #8720210 -
Attachment is obsolete: true
Assignee | ||
Comment 27•8 years ago
|
||
Sounds reasonable.
Attachment #8720212 -
Attachment is obsolete: true
Attachment #8723500 -
Flags: review?(bugs)
Updated•8 years ago
|
Attachment #8723500 -
Flags: review?(bugs) → review+
Comment 28•8 years ago
|
||
Comment on attachment 8723419 [details] [diff] [review] part.2 Add nsIWidget::GetNativeTextEventDispatcherListener() for TextEventDispatcher::NotifyIME() this is getting complicated. It is not quite clear to me who notifies and what and when. Some times TextEventDispatcher calls NotifyIME on the native listener and in some cases widget notifies the TextEventDispatcher. I guess we don't enter to a loop because... TextEventDispatcher doesn't notify if the listener is native. And mTextEventDispatcher->NotifyIME(aIMENotification); call in nsBaseWidget::NotifyIME doesn't do anything if TextEventDispatcher doesn't have listener or something like that.
Attachment #8723419 -
Flags: review?(bugs) → review+
Comment 29•8 years ago
|
||
Comment on attachment 8723432 [details] [diff] [review] part.4 TextEventDispatcher::DispatchInputEvent() should decide if dispatches events with nsIWidget::DispatchInputEvent() with input transaction type > // If the event is dispatched via nsIWidget::DispatchInputEvent(), it > // sends the event to the parent process first since APZ needs to handle it > // first. However, some callers (e.g., keyboard apps on B2G and tests > // expecting synchronous dispatch) don't want this to do that. > nsresult rv = NS_OK; >- if (aDispatchTo == eDispatchToParentProcess) { >+ if (mInputTransactionType == eNativeInputTransaction || >+ mInputTransactionType == eAsyncTestInputTransaction) { > aStatus = widget->DispatchInputEvent(&aEvent); > } else { > rv = widget->DispatchEvent(&aEvent, aStatus); > } So this is still just as hard to read as in the previous patch. One option I guess would be to have a helper method, maybe called 'bool IsParentProcessDependentTransaction()' which then returns true if mInputTransactionType is eNativeInputTransaction or eAsyncTestInputTransaction and use that here. That would kind of self document this code.
Attachment #8723432 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 30•8 years ago
|
||
Oops! I'm very sorry, I attached old patch (perhaps, forgot to do qrefresh?). This is what I'd like you to review. Like your idea, this has |ShouldSendInputEventToAPZ()|.
Attachment #8723432 -
Attachment is obsolete: true
Attachment #8723863 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8723863 -
Attachment description: bug1137572-4.diff → part.4 TextEventDispatcher::DispatchInputEvent() should decide if dispatches events with nsIWidget::DispatchInputEvent() with input transaction type
Comment 31•8 years ago
|
||
Comment on attachment 8723863 [details] [diff] [review] part.4 TextEventDispatcher::DispatchInputEvent() should decide if dispatches events with nsIWidget::DispatchInputEvent() with input transaction type Thanks, this is easier to follow.
Attachment #8723863 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 32•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9f1b8cc65f68a638e15ab489280288bcf90b985 Bug 1137572 part.1 TextEventDispatcher should manage its input transaction type r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/b8af9e4c043927f01a858538b0d7e46b6e5c25b0 Bug 1137572 part.2 Add nsIWidget::GetNativeTextEventDispatcherListener() for TextEventDispatcher::NotifyIME() r=smaug, sr=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/a8dbb4e58e546843c0b0710f8aa2b453f5cfcadc Bug 1137572 part.3 Use pseudo IME context when TextEventDispatcher has input transaction which is not for native event handler r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/06d5532f051f5a0c6796443d0c1794700c6667b6 Bug 1137572 part.4 TextEventDispatcher::DispatchInputEvent() should decide if dispatches events with nsIWidget::DispatchInputEvent() with input transaction type r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/f31b1e7194143f36ab1e6f0355b9b1b1a11cec0a Bug 1137572 part.5 Implement TextEventDispatcher::SetPendingComposition() for some platforms whose clause information may overlap each other or the order may not be from start to the end r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/5482d679c1a8b7c55657d50086f8eb6a59ef9ad9 Bug 1137572 part.6 TextEventDispatcher::DispatchKeyboardEventInternal() should copy mNativeKeyEvent when it's in native text input transaction r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/2982777d071d36ffa9761d4ec2347ff36640627c Bug 1137572 part.7 Add TextEventDispatcherListener::WillDispatchKeyboardEvent() for easier to maintain r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/782cc1725295b36e34a854a4d64159da3b16f4e9 Bug 1137572 part.8 Callers of methods to dispatch composition events of TextEventDispatcher should be able to specify specific time/timeStamp r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/fc409d9244ce5b34fd484f52b2d81ceb300cbd9c Bug 1137572 part.9 TextEventDispatcher should not allow WillDispatchKeyboardEvent() modifies unexpected members of WidgetKeyboardEvent r=smaug
Comment 33•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a9f1b8cc65f6 https://hg.mozilla.org/mozilla-central/rev/b8af9e4c0439 https://hg.mozilla.org/mozilla-central/rev/a8dbb4e58e54 https://hg.mozilla.org/mozilla-central/rev/06d5532f051f https://hg.mozilla.org/mozilla-central/rev/f31b1e719414 https://hg.mozilla.org/mozilla-central/rev/5482d679c1a8 https://hg.mozilla.org/mozilla-central/rev/2982777d071d https://hg.mozilla.org/mozilla-central/rev/782cc1725295 https://hg.mozilla.org/mozilla-central/rev/fc409d9244ce
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•