Create automated tests of NOTIFY_IME_OF_SELECTION_CHANGE and NOTIFY_IME_OF_TEXT_CHANGE

RESOLVED FIXED in Firefox 55

Status

()

Core
Event Handling
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod})

Trunk
mozilla55
inputmethod
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox55 fixed)

Details

(Whiteboard: [qf-])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

Comment hidden (empty)
(Assignee)

Updated

11 months ago
Blocks: 1304624
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
(Assignee)

Updated

5 months ago
Blocks: 1250823
No longer blocks: 1349138
Whiteboard: [qf]
Whiteboard: [qf] → [qf-]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7cccb0e01cb19dcd53a724f97518f7b0649cf41
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e07bab91a1ff2736325bee8b725cfae79f1c1dac
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a52cf737fcdd911ae5b36c375263ef0aa8450461
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86b728aecb5e29229ea3902a6a18801e442bc15d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac37a7703515b12dea81a251374efacd276c7440
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93550f522b9ffcb4253644eef26615f949f969d3
https://treeherder.mozilla.org/#/jobs?repo=try&revision=101ce5f951f3d7b76262d98fa2d472f2bea1fc98
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ba85b2d36c871b3e04330d72bf8ff03f5326bbc
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9c76d59e49d3d7c5e83de90353bc13802780805
https://treeherder.mozilla.org/#/jobs?repo=try&revision=98194ad7dae54588e93c8f350cec78d419b61f6e
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6270abcbee217b426284fad7ec71a2f8176f8cd
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1df676c1ff7923e0e445e9bae0f1f70e7880ca8d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a17cc4cbda2c0ae987313ecc04fdffee6ccf0000
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c341254d8ad1dd48f1d4c87c3615952e76344606
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 22

4 months ago
mozreview-review
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 23

4 months ago
mozreview-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+
(Assignee)

Comment 24

4 months ago
mozreview-review-reply
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 25

4 months ago
mozreview-review
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 26

4 months ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 31

4 months ago
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

Comment 32

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/640ff6dddc6c
https://hg.mozilla.org/mozilla-central/rev/0ea1fc4888f0
https://hg.mozilla.org/mozilla-central/rev/ab221fb1ba32
https://hg.mozilla.org/mozilla-central/rev/ca065b2e52d9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.