Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notify the embedder when it should display or hide an IME #20676

Merged
merged 1 commit into from Apr 23, 2018
Merged

Notify the embedder when it should display or hide an IME #20676

merged 1 commit into from Apr 23, 2018

Conversation

@fabricedesre
Copy link
Contributor

fabricedesre commented Apr 21, 2018 •
edited by SimonSapin

This adds a couple of embedder messages triggered when an editable element is focused or blured. The embedder also gets the type of data to edit so it can display a different keyboard type or a custom input method eg. for color choosing.

This is a partial fix for issue #12127

  • ./mach build -d does not report any errors
  • ./mach build-geckolib does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because there are no tests for the embedding api :(

This change is Reviewable
@highfive
Copy link

highfive commented Apr 21, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/document.rs, components/script/dom/element.rs, components/constellation/constellation.rs, components/script/dom/htmlinputelement.rs
  • @cbrewster: components/constellation/constellation.rs
  • @paulrouget: ports/servo/browser.rs, components/constellation/constellation.rs, components/compositing/compositor_thread.rs
  • @fitzgen: components/script/dom/document.rs, components/script/dom/element.rs, components/script_traits/script_msg.rs, components/script/dom/htmlinputelement.rs
  • @KiChjang: components/script/dom/document.rs, components/script/dom/element.rs, components/script_traits/script_msg.rs, components/script/dom/htmlinputelement.rs
@highfive
Copy link

highfive commented Apr 21, 2018

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@cbrewster
cbrewster reviewed Apr 21, 2018
View changes
components/constellation/constellation.rs
@@ -1247,6 +1247,14 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
FromScriptMsg::SetFullscreenState(state) => {
self.embedder_proxy.send(EmbedderMsg::SetFullscreenState(source_top_ctx_id, state));
}
FromScriptMsg::ShowIME(kind) => {
debug!("constellation got ShowIME message");
self.embedder_proxy.send(EmbedderMsg::ShowIME(source_top_ctx_id, kind));

This comment has been minimized.

Sign in to view
@cbrewster

cbrewster Apr 21, 2018

Member

I think I would rather have us talk to the embedder from the script thread instead of going through the constellation.

This comment has been minimized.

Sign in to view
@fabricedesre

fabricedesre Apr 21, 2018

Author Contributor

I didn't know this was a thing since we don't do it for other cases like TitleChange.

This comment has been minimized.

Sign in to view
@cbrewster

cbrewster Apr 21, 2018

Member

IRC Chat: https://mozilla.logbot.info/servo/20180421#c14649458-c14649484
TL;DR: We should avoid adding lots of cases where the constellation is only forwarding a message when the message could be sent directly. But, I would be okay if this were a followup that removed other instances of this.

Also in the case of TitleChange, the constellation is checking if the source pipeline was the top-level pipeline.
@cbrewster
cbrewster approved these changes Apr 21, 2018
View changes
components/script/dom/element.rs Outdated

if let Some(input) = self.downcast::<HTMLInputElement>() {
input.input_type().as_ime_type()
} else if let Some(_textarea) = self.downcast::<HTMLTextAreaElement>() {

This comment has been minimized.

Sign in to view
@cbrewster

cbrewster Apr 21, 2018

Member

nit: else if self.is::<HTMLTextAreaElement>() {
components/script_traits/script_msg.rs Outdated
@@ -163,6 +163,10 @@ pub enum ScriptMsg {
GetScreenSize(IpcSender<(DeviceUintSize)>),
/// Get the available screen size (pixel)
GetScreenAvailSize(IpcSender<(DeviceUintSize)>),
/// Request to present an IME to the user when an editable element is focused.
ShowIME(InputMethodType),
/// Request to hide the IME when the editable element is blured.

This comment has been minimized.

Sign in to view
@cbrewster

cbrewster Apr 21, 2018

Member

nit: s/blured/blurred/
@fabricedesre
Notify the embedder when it should display or hide an IME
Loading status checks…
4288661
@fabricedesre fabricedesre force-pushed the fabricedesre:ime-embedding branch from 2e1c978 to 4288661 Apr 22, 2018
@fabricedesre
Copy link
Contributor Author

fabricedesre commented Apr 23, 2018

@cbrewster Is there anything left for me to do here?
@cbrewster
Copy link
Member

cbrewster commented Apr 23, 2018

LGTM @bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Apr 23, 2018

📌 Commit 4288661 has been approved by cbrewster
@highfive highfive assigned cbrewster and unassigned fabricedesre Apr 23, 2018
@bors-servo
Copy link
Contributor

bors-servo commented Apr 23, 2018

⌛ Testing commit 4288661 with merge c5f7c9c...
bors-servo added a commit that referenced this pull request Apr 23, 2018
@bors-servo
Auto merge of #20676 - fabricedesre:ime-embedding, r=cbrewster
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
c5f7c9c 
Notify the embedder when it should display or hide an IME

<!-- Please describe your changes on the following line: -->
This adds a couple of embedder messages triggered when an editable element is focused or blured. The embedder also gets the type of data to edit so it can display a different keyboard type or a custom input method eg. for color choosing.

This is a partial fix for issue #12127

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach build-geckolib` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because there are no tests for the embedding api :(

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20676)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 23, 2018

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: cbrewster
Pushing c5f7c9c to master...
@bors-servo bors-servo merged commit 4288661 into servo:master Apr 23, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbrewster cbrewster

@paulrouget paulrouget

@asajeffrey asajeffrey

Assignees

@cbrewster cbrewster

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@fabricedesre @highfive @cbrewster @bors-servo @Manishearth
You can’t perform that action at this time.