Closed Bug 554823 Opened 15 years ago Closed 15 years ago

Hook up touch event listeners to document's and window's state

Categories

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

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

Attachments

(1 file, 2 obsolete files)

When a touch event listener is added or removed from any element, we need to inform this change to the document and the window, because we need to change some OS-level window modes if a page has requested them. This is similarly done in http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventListenerManager.cpp#459
Attached patch WIP v1 (obsolete) (deleted) — Splinter Review
First version. This goes on top of queue from bug 508906. Still some problems to solve: it doesn't check if the listener being removed actually exists, which is something may or may not be wanted. And other things to do: need to improve the dynamic of registering touch mode on the OS based on the active document, switching tabs, etc. This will be important after bug 130078 lands and need to reset state if the document changes
Now the next step is to change the native window settings whenever the displayed document changes. Basically, when a document is reloaded, a link is followed or the user change tabs, we need to check if the current document has touch listeners, and register for receiving raw touch input on the native window. I did some digging on the code today but couldn't find anything suited for this (detecting when the displayed document changes). I believe it's somewhere related in the DocumentViewerImpl/PresShell/PresContext, but I still didn't find the perfect function. The closest is the DocumentViewerImpl ctor/dtor, but they're obviously not called when changing tabs. Olli, any suggestions on where to look for this?
This is tricky. How to handle cases where a page is loaded in sidebar and it uses touch events, but then main content area has a page which uses gesture events.
(In reply to comment #3) > This is tricky. How to handle cases where a page is loaded in sidebar and it > uses > touch events, but then main content area has a page which uses gesture events. yeah, until we have a full engine this will be a problem.. For subdocuments we can just disallow them, but the sidebar case never occured to me, which makes this specially tricky. I think that for now this will be a thing that the pages will need to be aware of. Like how a webpage can resize the browser's window. (can a webpage from the sidebar do that too?) But I see that it's not just a conceptual problem, it's a technical one too.. what happens when the user switch tabs and the sidebar have touch listeners registered? hmmmm... it seems that the main content area will end up overriding it easily
I think a good feature analogy here is the back-forward toolbar buttons. They always reflect the history state for the main current page, so it is updated on tab switching or page navigation.
For now you could perhaps add some code to detect when content-primary docshell changes.
(In reply to comment #6) > For now you could perhaps add some code to detect when content-primary > docshell changes. Thanks for the pointer, I have a WIP patch that is triggered by ContentShellAdded and it works: it detects when the tab changes or a new one is created. (though it appears that it doesn't change when the doc changes, but I need to test it more) In the meantime I thought of a perhaps more friendly alternative taking into consideration the sidebar thing: the mode could be switched based on the top level document that have focus.. so if a page on the sidebar requests touch events it will work, and if the user wants to scroll the main page he just needs to tap it once to gain focus
Indeed, following focus would be better, I think. Just hook up something to focusmanager.
Okay, an update. I worked a bit on this today and it looks like using the FocusManager is the way to go. I spent some time coming up with the basic design on what goes into dom/content/widget, and it goes like this: 1. nsDocument tracks the counter of touch event listener on the document 2. whenever it changes from 0 -> 1 or 1 -> 0, it finds the nsPIDOMWindow related to the document and set its "touch status" 3. when setting the touch status, the window checks if it the focused window and in this case register with the OS to receive the raw input events 4. if it's not the focused window, it just marks a field as needs update, and when the FocusManager changes the focused window it checks if the window needs updating I have a semi-working patch for this but it needs more polish. The question I have right now is how bug 130078 would change things here. I don't know if the nsPIDOMWindow I'm getting are associated with a real widget (which won't be useful anymore) or with the fictional "window" object from a document. The data lies in nsIWidget if it matters. I'll investigate this more and probably will have to make changes. Also, I don't know yet how the design above works for the browser chrome or xul windows.
Attached patch WIP v2 (obsolete) (deleted) — Splinter Review
Hooking up the automatic changes to the Focus Manager. I was a bit confused on my last comment about nsPIDOMWindow and nsIWidget, but I've cleared the doubts now. This version is working all ok except for an issue on the first out-of-focus change, which I don't know yet why it's happening but I'll investigate soon (I believe it's some mismatch between focusedWindow/innerWindow/outerWindow and other similar windows that gives me trouble to pick the right one). The UpdateTouchState calls at the FocusManager seems a bit out-of-place there, so I'll try to change it to make everything on the nsPIDOMWindow (for example, I can change the call from nsFocusManager::WindowRaised to nsPIDOMWindow::ActivateOrDeactivate... I still need to check if I can make similar changes to the other call places)
Attachment #435299 - Attachment is obsolete: true
If you have questions about nsFocusManager, Neil is the right person.
Do you need this to be called just for top-level windows, or every child window? The change you made to nsFocusManager::SetFocusInner will be called even for inactive windows and for windows in background tabs.
(In reply to comment #12) > Do you need this to be called just for top-level windows, or every child > window? If I understand correctly what a top-level window means, it's just for top-level windows. Basically I want to make that call whenever the focus changes from a top-level document (say, by switching tabs, or by switching focus between the main content area and a page on the sidebar). At this point I'm unsure if I will want to ignore focus changes between chrome and content or not. If clicking on a tab changes focus to chrome, but then the process changes the focus back to the new content, that I'd be the ideal scenario for me.
(In reply to comment #13) > If I understand correctly what a top-level window means, it's just for > top-level windows. Basically I want to make that call whenever the focus > changes from a top-level document (say, by switching tabs, or by switching > focus between the main content area and a page on the sidebar). A toplevel window is the outer one with the titlebar. > At this point I'm unsure if I will want to ignore focus changes between chrome > and content or not. If clicking on a tab changes focus to chrome, but then the > process changes the focus back to the new content, that I'd be the ideal > scenario for me. I don't see why you would care whether chrome or content windows are being used.
Attached patch v3 (deleted) — Splinter Review
So I fiddled with this during this week, trying a number of different options, and got rid of the SetFocusInner changes. The approach I took is following the call of nsFocusManager::Focus when aIsNewDocument == true. It looks like this covers all the cases I needed. I'm pretty happy with the behavior, and it's also simpler to manage. The only thing it does not cover is seeing focus change between chrome and content, but I couldn't find a compelling reason to do that at this point (it might even be better to keep this way). I think the work intended for this bug is done now. I'll remerge this patch on the queue from bug 508906 and keep going from there. The only nit that I wanted to cover if possible would be to get rid of the line added to nsFocusManager and handle it on the window itself. It looks like this could be done following the Focus event sent here: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1624 , but I can't seem to find where this event is handled by the window.
Attachment #442228 - Attachment is obsolete: true
Marking as Resolved, per comment 15
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
How is this fixed? Or do you mean fixed in some your own branch?
(In reply to comment #17) > How is this fixed? Or do you mean fixed in some your own branch? Yeah, that. There's not an actual branch yet but it's part of the patch queue I'm posting on bug 508906. I used this bug to track the progress on this goal and to get feedback on the implementation. I'm thinking Bugzilla is not the ideal way to do that but the comments here were very helpful so it was good to have a bug for that.
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: