Closed Bug 1524242 Opened 5 years ago Closed 5 years ago

Capture TabParent by LayersId when creating TextComposition

Categories

(Core :: DOM: UI Events & Focus Handling, enhancement, P2)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Project Flags:
Fission Milestone M2
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

(Whiteboard: [fission-event-m1])

Attachments

(1 file)

Bug 1524242 - Capture TabParent of out-of-process iframe when creating TextComposition.
47 bytes, text/x-phabricator-request
Details | Review

When creating a TextComposition, obtain the focused LayersId (from bug 1524236), use TabParent::GetTabParentFromLayersId() with that LayersId to find a TabParent, and capture a pointer to that TabParent in a field of TextComposition in place of the TabParent captured at present.

Priority: -- → P2
Whiteboard: [fission-event-m1]

I gather that FocusState can live in the GPU process. What's the best way to synchronously obtain the FocusState::mFocusLayersId from the main thread of the chrome process? Should there be sync IPC? Or should the main thread maintain its own copy of FocusState::mFocusLayersId that FocusState would push updates to via async IPC? Or should the mechanism that pushes focus updates to APZ also push updates to a chrome-process-main-thread copy?

Flags: needinfo?(kats)

In general we like to avoid sync IPC, and if this is something that can called a lot over a short period of time, or something that content can potentially trigger, then we definitely don't want to be using sync IPC for it.

The mechanism that pushes focus updates to APZ works by pushing a FocusTarget object as part of the paint transactions to the compositor. APZ then assembles the FocusTarget instances from the different processes in a tree structure and walks it to figure out which thing really has focus. So if had the content processes sending their FocusTarget objects to the UI process as well, we'd need to ensure that there's a IPC channel for that, and then duplicate/refactor the APZ code that builds the tree and figures out the final focus target. This also seems kind of unwieldy to me.

So by elimination I'm favoring the option where the main thread keeps a copy of the mFocusLayersId and APZ can send it an update whenever that changes.

In terms of implementation, I'd put something like the below snippet at the end of APZCTreeManager::UpdateFocusState:

GetContentController(aRootLayerTreeId)->NotifyFocusLayersIdChanged(mFocusState.GetFocusLayersId());

and then add the NotifyFocusLayersIdChanged function to PAPZ. Docs in that file explain somewhat how it's used, and you'd want to leave a no-op implementation in the ContentProcessController, with the "real" implementation in ChromeProcessController.

Flags: needinfo?(kats)

/cc also botond and rhunt in case they have any thoughts on the above

Fission Milestone: --- → M2

Masayuki, what's the relationship of TextComposition creation to keystrokes? Is it always created in response to a keyboard event? I see that it's in some cases created from a runnable, but I wonder if the machinery in the patch that makes APZ push focus changes to the chrome main thread is unnecessary and instead the chrome main thread should remember what the focused LayersId on the most recent keyboard event was.

Is TextComposition also used with touch screen input methods? If so, is in possible for the TextComposition to be associated content other than what the most recent touch was hit tested to hit?

That is, should I throw away the patch here and instead make the chrome main thread remember which TabParent most recently had a keystroke, click or tap delivered to it?

Flags: needinfo?(masayuki)

TextComposition represents a set of a compositionstart, zero or more compositionupdate events and a compositionend event.

Composition events represent a set of text input via IME (dead key may be implemented with IME in some platforms). For example, when user types a Japanese word with IME, typing first character of the word dispatches compositionstart and compositionupdate. Then, user keeps typing 2nd or later characters of the word and compositionupdate is fired for each typing. Then, user converts the word with proper Kanji characters and compositionupdate is also fired for every conversion. Finally, user commits the word with Enter key or something. At that time, a compositionend event is fired.

TextComposition instance is create at the compositionstart and destroyed at the compositionend. The typing word may include privacy of the user. Therefore, the composition shouldn't be leaked to different document. So, once TextComposition is created with a LaysersId, TextComposition shouldn't be re-associated with different LayerId. One of the purpose of TextComposition is that we need to protect the user's privacy. Currently, we focus is changed during composition, we (and other browser vendors) forcibly commit the composition in the editor which has the composing string.

I see that it's in some cases created from a runnable,

Really? I don't think so. compositionstart is created by native IME handler, and it's dispatched synchronously. Then, PresShell dispatches it into the DOM with using IMEStateManager. If focused content in the main process is a TabParent, TextComposition sends the composition event to the storing TabParent. Then, content process receives it in TabChild and dispatches it via PuppetWidget, PresShell and IMEStateManager. So, except IPC, composition events should be handled synchronously.

but I wonder if the machinery in the patch that makes APZ push focus changes to the chrome main thread is unnecessary and instead the chrome main thread should remember what the focused LayersId on the most recent keyboard event was

So, TextComposition should store the LayerId at creation.

Is TextComposition also used with touch screen input methods?

If it causes composition events, yes it is. You can check it with https://w3c.github.io/uievents/tools/key-event-viewer.html

If so, is in possible for the TextComposition to be associated content other than what the most recent touch was hit tested to hit?

No, should be locked with the LayerId at creation. Then, when focused LayerId is changed, existing TextComposition should commit the composition and should be destroyed.

That is, should I throw away the patch here and instead make the chrome main thread remember which TabParent most recently had a keystroke, click or tap delivered to it?

Important thing is "focus". When focus is changed, IMEStateManager updates input context of the widget. IME may change behavior for optimizing the input context. For example, if its type is url, IME may change their virtual keyboard to URL keyboard which has http:// button etc. So, when focused LayerId is changed, it should be stored in IMEStateManager like
sFocusedIMETabParent.

Flags: needinfo?(masayuki)

Thanks. It looks a lot like the approach taken by the current patch here is unsuitable and instead we should take the LayersId from whatever lower-level (keyboard, touch) event causes an IME composition to be created and pass it around or store it in a global on the chrome process main thread.

(In reply to Henri Sivonen (:hsivonen) from comment #7)

Thanks. It looks a lot like the approach taken by the current patch here is unsuitable and instead we should take the LayersId from whatever lower-level (keyboard, touch) event causes an IME composition to be created and pass it around or store it in a global on the chrome process main thread.

After looking at this in rr, maybe the current patch is the right approach after all. I misunderstood how keystrokes work when an IME is active. At least when using Mozc on Ubuntu, it looks like enough things happen within Mozc and Gtk that Gecko doesn't see the kind of key events I was thinking on in my previous comment.

Assignee: nobody → hsivonen
Status: NEW → ASSIGNED

Instead of maintaining a copy of APZ focus in the chrome process, maybe we could have APZ stamp composition events with LayersId the same way it stamps keyboard events but without taking any pan&zoom action ever for composition events.

Disregarding out-of-process iframes for the moment, when entering text into a text field residing in top-level Web content (i.e. content process) using the Mozc Japanese IME on Ubuntu, I see a new TextComposition object allocated both in the chrome process and in the content process. What do I need to know to understand this duality if composition event handling in the chrome process and the content process when the text is being entered into the content process?

Flags: needinfo?(masayuki)
Depends on: 1534961

when entering text into a text field residing in top-level Web content (i.e. content process) using the Mozc Japanese IME on Ubuntu, I see a new TextComposition object allocated both in the chrome process and in the content process.

Even when a content process has composition, chrome process needs to manage the composition. For example, if user clicks somewhere in chrome, chrome process needs to request IME to commit composition before moving focus. Thus, chrome process needs to know whether there is composition and which TabParent has composition. Therefore, chrome process has TextComposition instance.

What do I need to know to understand this duality if composition event handling in the chrome process and the content process when the text is being entered into the content process?

If you need to know whether TextComposition in the chrome process is for composition in the chrome process or a content process, you can check TextComposition::mTabParent (or TextComposition::GetTabParent() from outer).

Currently, I think, content process does not need to have TextComposition when its child iframe content process has composition because TextComposition in the chrome process manages the composition even if the parent content process steals focus, etc.

Flags: needinfo?(masayuki)

Thanks.

I think I now have a better patch (with APZ stamping composition events with the LayersId of the focused layer and with TextComposition capturing the TabParent only at creation), but when I try to use the Mozc IME with an out-of-process iframe, I get the kind of behavior I'd expect with a plain U.S. keyboard. That is, once the focus is in an out-of-process iframe, it looks like IME capture of keystrokes gets turned off.

What mechanism normally lets a content process to tell the chrome process to tell the IME to capture keystrokes?

I see a bunch of NotifyIME* messages in PBrowser, but I'd expect those to work, because those don't appear to be the kind of notifications that would need to bounce via PBrowserBridge.

Flags: needinfo?(masayuki)

I'm guessing the NotifyIME() code is the code I'm looking for but something is preventing it from working. Investigating sActiveTabParent next.

Component: Event Handling → User events and focus handling

(In reply to Henri Sivonen (:hsivonen) from comment #14)

Investigating sActiveTabParent next.

Indeed, it looks like IMEStateManager::OnChangeFocusInternal in the key method here. It needs change to take into account that the cross-process focus hierarchy is potentially deeper with out-of-process iframes. It's still a bit unclear to me what actions IMEStateManager::OnChangeFocusInternal should take in all processes and what actions should be reflected in the chrome process only.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #12)

Currently, I think, content process does not need to have TextComposition when its child iframe content process has composition because TextComposition in the chrome process manages the composition even if the parent content process steals focus, etc.

Do I understand correctly that it follows that content processes that contain iframe elements whose content in out-of-process don't need to maintain an analog of sActiveTabParent, either? And when such a process runs IMEStateManager::OnChangeFocusInternal with such an iframe as aContent, it should send a message to the chrome process to run IMEStateManager::OnChangeFocusInternal there with a by-LayersId TabParent?

In general, most of the interesting stuff in IMEStateManager::OnChangeFocusInternal seems to be meant for running in the chrome process only. What parts of it are meaningful to run in content processes? In particular, if a content process that contains the iframe element of an out-of-process iframe delegates to the chrome process, which bits (if any) of IMEStateManager::OnChangeFocusInternal should it run before returning?

AFAICT, after bug 1533716, the place in the content process that's most obviously aware of which TabParent has most recently obtained focus, including TabParents associated with out-of-process iframes, is TabParent::RecvRequestFocus.

How about splitting the logic of IMEStateManager::OnChangeFocusInternal such that chrome-process-only logic that reacts to changes in focused content process runs from TabParent::RecvRequestFocus and logic that's local to the current process runs by getting called from nsFocusManager as IMEStateManager::OnChangeFocusInternal is presently called?

(Still unclear to me what exactly the logic split should be.)

Also, if the chrome process ends up maintaining focused TabParent independently of APZ, should keyboard event dispatch use that TabParent instead of using APZ focus?

First of all, IMEStateManager::OnChangeFocus() should be called in out-of-process iframe when an element in it gets focus. Then, it's propagated to the chrome process's IMEStateManager::SetInputContextForChildProcess(). Finally, it sets InputContext of the widget to enable/disable IME and to set IME open mode.

(If the chrome process steals focus from a content process (including when another content process steals focus), the focus change in the chrome process is handled synchronously and IMEStateManager posts a message to previous focused content process to commit composition via requesting IME to commit existing composition. Then, IMEStateManager will receive some notifications from the old content process and ignore them. See bug 1377672.)

Do I understand correctly that it follows that content processes that contain iframe elements whose content in out-of-process don't need to maintain an analog of sActiveTabParent, either?

Yes, only the chrome process and currently focused content process need to manage IME.

And when such a process runs IMEStateManager::OnChangeFocusInternal with such an iframe as aContent, it should send a message to the chrome process to run IMEStateManager::OnChangeFocusInternal there with a by-LayersId TabParent?

I think that when focus is moved to a content which is associated with a TabParent, IMEStateManager::OnChangeFocusInternal() should be called as current implementation. Then, IMEStateManager::OnCHangeFocusInternal() in the out-of-process iframe's content process should be called and it should set InputContext of its PuppetWidget. Then, IMEStateManager::SetInputContextForChildProcess() in the chrome process should be called. I.e., probably, we don't need to change the path even though we may need to add LayerId information.

In general, most of the interesting stuff in IMEStateManager::OnChangeFocusInternal seems to be meant for running in the chrome process only. What parts of it are meaningful to run in content processes?

No, it in content process computes IME state in the process and manages focus moving in the content process. For example, when focus is moved from a <button> element to an <input> element in a content process, it sets InputContext of PuppetWidget. In this case, IMEStateManager::OnChangeFocusInternal() is called only in the content process.

BTW, you might be confused with 2 "focus" meanings in IMEStateManager.

IMEStateManager::OnChangeFocusInternal() and IMEStateManager::SetInputContextForChildProcess() handles focus move between DOM elements. Focused TabParent is managed with IMEStateManager::sActiveTabParent. On the other hand, there is "IME-focus" too. When editor gets focus and initializes its content, notifies IME of another "focus" via IMEStateManager::NotifyIME(). This is managed with IMEStateManager::sFocusedIMETabParent. The reason of there are 2 "focus" management here is, even after focused DOM element is changed, IMEStateManager needs to manage notifications from IMEContentObserver which observes editor's content/selection changes and composition events coming from IME.

Perhaps, you need to add sActiveLayerId for sTabParent and sFocusedIMELayerId for sFocusedIMETabParent.

Flags: needinfo?(masayuki)

(In reply to Henri Sivonen (:hsivonen) from comment #17)

How about splitting the logic of IMEStateManager::OnChangeFocusInternal such that chrome-process-only logic that reacts to changes in focused content process runs from TabParent::RecvRequestFocus and logic that's local to the current process runs by getting called from nsFocusManager as IMEStateManager::OnChangeFocusInternal is presently called?

I guess that current code working with TabParent is necessary in content process which has some out-of-process <iframe>. For example, focus is moved from an <input> element to such <iframe>, it needs to disable IME temporarily. If the process for <iframe> needs to enable IME, then, the process needs to do it with the chrome process.

(Of course, if you can make the method simpler, please do it, but I couldn't do it because of complicated code which shouldn't be duplicated.)

(In reply to Henri Sivonen (:hsivonen) from comment #18)

Also, if the chrome process ends up maintaining focused TabParent independently of APZ, should keyboard event dispatch use that TabParent instead of using APZ focus?

From the point of view of the privacy and DOM Events spec, I think so. Basically, keyboard events which changes composition should be fired in same process.

(Anyway, I'm also confused with the complicated things even though I designed.)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #20)

(In reply to Henri Sivonen (:hsivonen) from comment #17)

How about splitting the logic of IMEStateManager::OnChangeFocusInternal such that chrome-process-only logic that reacts to changes in focused content process runs from TabParent::RecvRequestFocus and logic that's local to the current process runs by getting called from nsFocusManager as IMEStateManager::OnChangeFocusInternal is presently called?

I guess that current code working with TabParent is necessary in content process which has some out-of-process <iframe>. For example, focus is moved from an <input> element to such <iframe>, it needs to disable IME temporarily. If the process for <iframe> needs to enable IME, then, the process needs to do it with the chrome process.

So when a content process containing an out-of-process iframe moves focus to such iframe, it should call DestroyIMEContentObserver(); within its own process, right? (I.e. sActiveIMEContentObserver lives in content processes, right?)

What's the code for temporarily disabling IME inside the iframe?

(In reply to Henri Sivonen (:hsivonen) from comment #18)

Also, if the chrome process ends up maintaining focused TabParent independently of APZ, should keyboard event dispatch use that TabParent instead of using APZ focus?

From the point of view of the privacy and DOM Events spec, I think so. Basically, keyboard events which changes composition should be fired in same process.

OK. I'll put bug 1534961 on hold at least for now and will try to make the chrome process maintain the focused TabParent independently of APZ so that changes in the focused tab parent don't change order relative to other messages when the GPU process is in use.

Thank you.

Flags: needinfo?(masayuki)

(In reply to Henri Sivonen (:hsivonen) from comment #21)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #20)

(In reply to Henri Sivonen (:hsivonen) from comment #17)

How about splitting the logic of IMEStateManager::OnChangeFocusInternal such that chrome-process-only logic that reacts to changes in focused content process runs from TabParent::RecvRequestFocus and logic that's local to the current process runs by getting called from nsFocusManager as IMEStateManager::OnChangeFocusInternal is presently called?

I guess that current code working with TabParent is necessary in content process which has some out-of-process <iframe>. For example, focus is moved from an <input> element to such <iframe>, it needs to disable IME temporarily. If the process for <iframe> needs to enable IME, then, the process needs to do it with the chrome process.

So when a content process containing an out-of-process iframe moves focus to such iframe, it should call DestroyIMEContentObserver(); within its own process, right? (I.e. sActiveIMEContentObserver lives in content processes, right?)

Right.

What's the code for temporarily disabling IME inside the iframe?

Currently, it's (I mean it's when new TabParent gets focus in the chrome process) done in IMEStateManager::OnChangeFocusInternal().

Flags: needinfo?(masayuki)

Well, InputContext::ORIGIN_CONTENT could be renamed to InputContext::ORIGIN_ANOTHER_PROCESS or something.

Depends on: 1535537
Attachment #9046284 - Attachment description: Bug 1524242 - Capture TabParent by LayersId when creating TextComposition. → Bug 1524242 - Capture TabParent of out-of-process iframe when creating TextComposition.
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2244c803a5d0
Capture TabParent of out-of-process iframe when creating TextComposition. r=masayuki

https://hg.mozilla.org/mozilla-central/rev/2244c803a5d0

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Regressions: 1549930
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: