Closed Bug 1217700 Opened 4 years ago Closed 3 years ago

Create automated tests of NOTIFY_IME_OF_SELECTION_CHANGE and NOTIFY_IME_OF_TEXT_CHANGE

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox44 --- affected
firefox55 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod, Whiteboard: [qf-])

Attachments

(4 files)

No description provided.
For testing regression, I should fix this quickly for bug 1349138 but without complicated tests which I have planed.


I think that IMEContentObserver should always observe any changes in the editor. However, before computing each change, it should check if current input transaction is created for nsITextInputProcessor. When it's so, IMEContentObserver should send any notifications to widget.  Otherwise, respect the nsIMEUpdatePreference of the widget. (For fixing quickly, we don't need to fix this in e10s mode strictly because it's enough to test in non-e10s mode and PuppetWidget always require all notifications anyway.)

Then, TextInputProcessor should notify those notifications via nsITextInputProcessorCallback. Then, we can test these notifications from JS (mochitest).
Blocks: 1349138
Blocks: 1250823
No longer blocks: 1349138
Whiteboard: [qf]
Whiteboard: [qf] → [qf-]
I'm asking most reviews to makoto-san because looks like that smaug is too busy. However, only part 3 changes XPCOM API. Therefore, I'm requesting the review to smaug.
FYI: The part 4's designMode issue is a bug of nsFocusManager. nsFocusManager treats caret moving in chrome content as navigating documents always. I think that nsFocusManager should not do so if focused document is in designMode but anyway, this should be fixed in different bug.
Comment on attachment 8859865 [details]
Bug 1217700 part.1 nsIWidget should return reference to IMENotificationRequests

https://reviewboard.mozilla.org/r/131572/#review134742
Attachment #8859865 - Flags: review?(m_kato) → review+
Comment on attachment 8859867 [details]
Bug 1217700 part.3 Expose text change, selection change and position change notifications to nsITextInputProcessorCallback with nsITextInputProcessorNotification

https://reviewboard.mozilla.org/r/131576/#review134762

::: dom/interfaces/base/nsITextInputProcessorCallback.idl:74
(Diff revision 1)
>    readonly attribute ACString type;
> +
> +  /**
> +   * Be careful, line breakers in the editor are treated as native line
> +   * breakers.  I.e., on Windows, a line breaker is "\r\n" (CRLF).  On the
> +   * others, it is "\n" (LF).  If you want TextInputProcessor to tread line

treat?

::: dom/interfaces/base/nsITextInputProcessorCallback.idl:80
(Diff revision 1)
> +   * breakers on Windows as XP line breakers (LF), please file a bug with
> +   * the reason why you need the behavior.
> +   */
> +
> +  /**
> +   * This attribute is available when type is "notify-text-change" or

Perhaps "This attribute has a valid value when type is ..."
Same with other "is available..." attributes, since the attributes are there always, but just don't have meaning full value always.

::: dom/interfaces/base/nsITextInputProcessorCallback.idl:82
(Diff revision 1)
> +   */
> +
> +  /**
> +   * This attribute is available when type is "notify-text-change" or
> +   * "notify-selection-change".
> +   * This is offset of the selection start or the modified range.

offset of ... the modified range. What does that mean?
Attachment #8859867 - Flags: review?(bugs) → review+
Comment on attachment 8859867 [details]
Bug 1217700 part.3 Expose text change, selection change and position change notifications to nsITextInputProcessorCallback with nsITextInputProcessorNotification

https://reviewboard.mozilla.org/r/131576/#review134762

> Perhaps "This attribute has a valid value when type is ..."
> Same with other "is available..." attributes, since the attributes are there always, but just don't have meaning full value always.

Rewritten with "This attribute has a valid value". Although, these attributes raises exception when the type is not valid.

> offset of ... the modified range. What does that mean?

Rewritten with:

   * This is offset of the start of modified text range if type is
   * "notify-text-change".  Or offset of start of selection if type is
   * "notify-selection-change".
Comment on attachment 8859866 [details]
Bug 1217700 part.2 IMEContentObserver should observe all possible notifications and check if it should be notified when it occurs

https://reviewboard.mozilla.org/r/131574/#review135092
Attachment #8859866 - Flags: review?(m_kato) → review+
Comment on attachment 8859868 [details]
Bug 1217700 part.4 Add automated tests for IMEContentObserver

https://reviewboard.mozilla.org/r/131578/#review135118
Attachment #8859868 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/640ff6dddc6c
part.1 nsIWidget should return reference to IMENotificationRequests r=m_kato
https://hg.mozilla.org/integration/autoland/rev/0ea1fc4888f0
part.2 IMEContentObserver should observe all possible notifications and check if it should be notified when it occurs r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ab221fb1ba32
part.3 Expose text change, selection change and position change notifications to nsITextInputProcessorCallback with nsITextInputProcessorNotification r=smaug
https://hg.mozilla.org/integration/autoland/rev/ca065b2e52d9
part.4 Add automated tests for IMEContentObserver r=m_kato
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.