libservo: Merge input method activation into the EmbedderControl API#40014
Conversation
servo-highfive
added
the
S-awaiting-review
mukilan
left a comment
mukilan
left a comment
There was a problem hiding this comment.
I think ports/servoshell/egl/app_state.rs also needs to be updated to use the new signature for the show_ime method on the delegate.
| InputType::Url => Some(InputMethodType::Url), | ||
| InputType::Week => Some(InputMethodType::Week), | ||
| _ => None, | ||
| impl TryInto<InputMethodType> for InputType { |
There was a problem hiding this comment.
The std library recommends we implement TryFrom<InputType> for InputMethodType rather than TryInto.
| pending_webdriver_events: HashMap<InputEventId, Sender<()>>, | ||
|
|
||
| /// A list of showing [`InputMethod`] interfaces. | ||
| visible_input_method: Vec<EmbedderControlId>, |
There was a problem hiding this comment.
Should this use a plural name?
| visible_input_method: Vec<EmbedderControlId>, | |
| visible_input_methods: Vec<EmbedderControlId>, |
There was a problem hiding this comment.
Yep, I've modified the name of this member.
mrobinson
force-pushed
the
merge-input-method-into-embedder-control-api
branch
from
6dfda96 to
9b7a87b
Compare
mrobinson
force-pushed
the
merge-input-method-into-embedder-control-api
branch
from
9b7a87b to
a40f9c6
Compare
Thanks for the review. I've taken your suggestions and have also made a few more miscellaneous changes, fixes, and renames. PTAL. |
|
🔨 Triggering try run (#18764095783) for OpenHarmony |
|
|
How hard would it be to add an integration test with virtual keyboards for this? How about a test webpage with an input form, where we assert that e.g. "hello" is input. I would imagine the test script to be as follows:
Perhaps the test page could also have a touch handler for the surface we expect to be covered by the IME on the lower half of the screen, so we could assert if the IME doesn't come up (as an alternative / addition to validating text input. |
| pub input_method_type: InputMethodType, | ||
| pub text: String, | ||
| pub insertion_point: Option<u32>, | ||
| pub multiline: bool, |
There was a problem hiding this comment.
/// If the input is text, the second parameter defines the pre-existing string
/// text content and the zero-based index into the string locating the insertion point.
/// bool is true for multi-line and false otherwise.
Can we add the previous documentation for test and insertion point here?
There was a problem hiding this comment.
I've re-added this documentation.
mrobinson
force-pushed
the
merge-input-method-into-embedder-control-api
branch
2 times, most recently
from
f508bf9 to
3f32554
Compare
I've added a unit test for this functionality, which I think is a better match. This avoids the need for touch handling. |
mrobinson
force-pushed
the
merge-input-method-into-embedder-control-api
branch
from
8f85f89 to
bbc5a10
Compare
|
🔨 Triggering try run (#18772120901) for OpenHarmony |
Bencher Report
|
✨ Try run (#18772120901) succeeded. |
components/servo/tests/webview.rs
Outdated
| }; | ||
|
|
||
| ensure!(ime.input_method_type() == InputMethodType::Text); | ||
| ensure!(ime.text() == "boo"); |
There was a problem hiding this comment.
Where does the value "boo" come from?
There was a problem hiding this comment.
So it turns out that no tests are actually run at all when the libservo tests are run. I think this is a regression from #39812. I'll try to post a fix soon.
Input methods are very similar to other kinds of embedder controls such as file selection boxes, so merge them into the same libservo API. This simplfiies the API surface a bit. Signed-off-by: Martin Robinson <[email protected]>
Signed-off-by: Martin Robinson <[email protected]>
Signed-off-by: Martin Robinson <[email protected]>
mrobinson
force-pushed
the
merge-input-method-into-embedder-control-api
branch
from
bbc5a10 to
3782b0a
Compare
|
Okay. This has been rebased on top of #40131 and should be ready to review again. |
servo-highfive
removed
the
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
4 participants
Input methods are very similar to other kinds of embedder controls such
as file selection boxes, so merge them into the same libservo API. This
simplfiies the API surface a bit.
Testing: This change comes with a new unit test.