Closed Bug 577579 Opened 14 years ago Closed 14 years ago

bug 130078 causes event breakage (fennec canvas event dispatch)

Categories

(Core :: Web Painting, defect)

Product:
Core
Component:
Web Painting
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
fennec 2.0b1+ ---

People

(Reporter: vlad, Assigned: tnikkel)

References

Details

Attachments

(5 files, 1 obsolete file)

xul testcase (but doesn't quite show the problem)
2.65 KB, application/vnd.mozilla.xul+xml
Details
Fennec fix?
982 bytes, patch
Details | Diff | Splinter Review
possible patch
4.32 KB, patch
Details | Diff | Splinter Review
add a new method
6.20 KB, patch
roc
: review+
Details | Diff | Splinter Review
make fennec use the new method
934 bytes, patch
mbrubeck
: review+
roc
: review+
Details | Diff | Splinter Review 
If I have bug 130078 applied, capturing chrome events seem to always have event.target as the toplevel DOMWindow, instead of the actual element underneath the mouse, as before.

This ends up breaking fennec, because for mouse events it depends on being able to walk upwards in the tree from the clicked event to find the right scrollbox to poke to get panning etc.  With 130078, the starting event.target is the toplevel window, and thus the upwards-walk never finds an element; the net result is that no scrolling/clicking/etc. is possbile.
I've seen it working couple of weeks ago, but now I see this problem too
I created this simple testcase in an attempt to show the problem, but this seems to work both with and without 130078, on windows at least -- clicking on any of the colored areas should show XULElement and the id of it, and not the window.  Will keep trying.
On Linux, with and without 130078, in Firefox, I get the same results as you.
So to reproduce this do I need to apply the 2 latest patches from  bug 130078?

(And would be great if someone had a testcase for this ;) )

Since Fennec uses DOMWindowUtils to send events to content, I wonder
if nsDOMWindowUtils::GetWidget needs some tweaking.
Hmm, or actually, how could that work.
Currently the not-visible xul:browser has a widget which nsDOMWindowUtils::SendMouseEvent uses. But if that widget is going away, 
and nsDOMWindowUtils does always use the toplevel widget, of course the
event is dispatched to whatever is shown on the screen - not to the
background webpage.
Ah, of course!

So we need to modify nsDOMWindowUtils::SendMouseEvent to somehow ensure that the event actually goes that window or some subwindow.
What patches exactly from 130078 are applied to show the problem? I ask because 130078 isn't very clear about what patches are needed and this is the behaviour I might expect if the "remove-widget" patch is applied but the "link-view-managers" patch was not applied.
I applied what I thought was the correct set of patches, but bug 130078 could probably use someone applying a new, complete, updated patch instead of the handful of piecemeal patches that are there.
Refreshed patches are available in my patch queue.
http://hg.mozilla.org/users/tnikkel_gmail.com/zoom-patches/
You want hookup-vms and remove-widget, ignore the rest.
Scrolling now works fine, but about:config click events does not work because 
this condition always FALSE:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/InputHandler.js#236
/* ignore all events that belong to other windows or documents (e.g. content events) */
if (aEvent.view != window)
   return;

with patches from bug 130078 aEvent.view == window always
So I'm not familiar how nsEvent handling works after bug 130078.
Who decides and where the Preshell (Or View/ViewManager?) to which the event is dispatched?
Events that come from the widget are dispatched via the view/viewmanager (and then usually the presshell) for that widget (both with and without 130078). When the content document loses its widget after 130078 that means it won't get events directly from widget code anymore. 130078 doesn't make any changes to the event handling code.
Bug 130078 does change event handling pretty radically, since the events for
content actually come from the same widget which is used for chrome.
(And that is what is bug is sort of about.)

I guess DOMWindowUtils needs to use the ViewManager/view or even PresShell
to dispatch events.
What I meant was that I haven't changed, and don't plan to change any of the code that deals with event handling. 130078 is certainly a big change.

Is there something that you think DOMWindowUtils will handle incorrectly when 130078 lands? I would hope not because that would mean that DOMWindowUtils is currently broken for any content iframe.
It is, yes.

Currently nsDOMWindowUtils::SendMouseEvent dispatches a synthetic mouse event through the nearest widget for the DOM window. So if the DOM window doesn't have a widget, the event goes to some ancestor window, and might not reach the DOM window SendMouseEvent was called on if there's content covering that window.

We need to change nsDOMWindowUtils::SendMouseEvent so that it dispatches events through the presshell for the window it was called on. The events can still be captured by popups and mouse capturing API, but I guess that's OK.
tracking-fennec: --- → 2.0b1+
I tried implementing comment 14 and it didn't fix the events.

Just commenting out the lines from comment 9 seems to fix it.
Why do we need the check in comment 9? It was added in a changeset with commit message "Content click bug fixed. Fennec content window now receives clicks!". Can we remove it? Will that break anything? Can we modify it?
In Fennec, somehow we need to check whether a mouse event's target was somewhere in content or not.  Currently that check is using aEvent.view.  If the event is a content-related event, we cancel the event and dispatch our own mousedowns and mouseups ourselves using windowutils.

Yes, we could probably replace this code with something else (and actually we will need to change this code soon, because we are seeing infinite clicks with visible same-process browser elements).  However, what is the purpose of aEvent.view if it always points to the root window?
We do need a way to tell the difference between Fennec's generated events and user input events, so I don't understand how just removing comment #9 could make Fennec work with bug 130078.
Currently in mobile-browser this is easy, because browser is hidden away so we don't see events firing infinitely.

This is a problem with using <browser> elements for rendering, and yes, I think we do need to be able to tell the difference.  Sounds like another bug to me.
OK, then it sounds like we should just take out the lines from comment #9 for now.
The lines from comment #9 will have to be replaced with different logic to figure out if the event is for content or not.

CC'ing mfinkle for his opinion.  If view does continue to be the root window, could we eliminate this property altogether?  What is its use after this bug lands?
The view property on events is part of the spec, so we can't remove it. See here http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/events/nsIDOMUIEvent.idl#42
The view property is a relic from the time when people thought we might be able to have multiple presentations for a single document (see defaultView property). Most DOM APIs (e.g. getBoundingClientRect, offset*) don't support that, and it's never going to happen.
It sounds like Fennec shouldn't be relying on this property anyway.
Now commenting out the lines from comment 9 doesn't work. I have no idea what is going on here. I can't seem to get consistent results. Does "make -f client.mk build" in the mozilla-central src dir not rebuild everything for fennec? Am I supposed to be using a mobile e10s repo? I seem to get getting remote browser stuff in my console anyways.
How does one debug fennec? I can attach and it takes a few seconds reading libxul.so etc, but when I try to place a breakpoint is can't find the symbol.
Attached patch Fennec fix? — — Splinter Review 
I don't have all the necessary patches applied, but does this do the trick?
Attachment #469063 - Flags: feedback?(tnikkel)
We need to fix DOMWindowUtils. But ofc if there is some temporary code which
makes Fennec to work reasonable well, that is ok.
(In reply to comment #28)
> We need to fix DOMWindowUtils. But ofc if there is some temporary code which
> makes Fennec to work reasonable well, that is ok.

Bug 130078 doesn't fix the problem where mouse events sent via DOMWindowUtils will not be received in the parent process, right?

Fennec e10s work right now because:
* for same process browsers, we can see if events are for that browser by checking if the target's ownerDocument is the root chrome document (and if it is for the browser, then we generated it and we ignore it to avoid infinite loops)
* for cross process browsers, once we sendCrossProcess*Events the parent process does not receive the events we sent over (thus avoiding the infinite loop)

But I thought this bug was about aEvent.view always being the root window, which is a separate issue.
Attached patch wip (obsolete) — — Splinter Review 
But there are plenty of other cases to fix in DOMWindowUtils.
Attached patch possible patch — — Splinter Review 
Timothy, does this work for you?

Key events are dispatched to the focused node, so they don't need similar
change.

I posted this to tryserver.
Attachment #469090 - Attachment is obsolete: true
Attachment #469112 - Flags: review?(tnikkel)
Comment on attachment 469112 [details] [diff] [review]
possible patch

I tried basically this same thing in comment 15 and it didn't work. Your patch didn't work either. But I think that it might be due to other breakage caused by 130078 in fennec: I get "ASSERTION: Our view doesn't have a widget. Totally stuffed!: 'Error', file ../../../../content/base/src/nsFrameLoader.cpp, line 772".
You were using E10s fennec?
I tested using Fennec with "browser.tabs.remote" false
(In reply to comment #33)
> You were using E10s fennec?
> I tested using Fennec with "browser.tabs.remote" false

I have no idea. I just more-or-less followed the instructions here: https://wiki.mozilla.org/Mobile/Build/Fennec. If it is not clear, I know nothing about Fennec.
Actually, I have now no idea what this bug is about.

(1) Is this about non-e10s-Fennec using DOMWindowUtils to send mouse events to
content or (2) is this about bug 130078 breaking e10s in some way? 

The patch is for the first one.

And btw, in E10s-Fennec .view will always be the chrome window when
the tab is remote.
Though, even in e10s-fennec there are non-remote tabs.
I think about:config is such.
(In reply to comment #0)
> If I have bug 130078 applied, capturing chrome events seem to always have
> event.target as the toplevel DOMWindow, instead of the actual element
> underneath the mouse, as before.

Based on this, the bug is about non-e10s-fennec, since
e10s-fennec can never give the actual element as the .target when the
tab is remote.
Comment on attachment 469063 [details] [diff] [review]
Fennec fix?

This doesn't seem to fix the issue.
Comment on attachment 469112 [details] [diff] [review]
possible patch

I think that we should not change nsIDOMWindowUtils::SendMouseEvent. It should continue to dispatch events via the widget. I don't think that not being able to click on things that aren't visible is a bug. I think it would be "magic" if it could do that. Since we use SendMouseEvent heavily in our tests I think that it should match the mouse events that a real user produces as closely as possible, and that means dispatching them on the root widget, even if that widget is the root chrome widget.

I think we should have a new function, SendMouseEventDirectly perhaps, that bypasses the widget and sends it directly to the presshell. This function can have the "magic" of being able to click on invisible elements. Fennec can use this new function.

I know that roc disagrees with me on this matter, so he should chime in at least, before you make changes.

Other than that I would r+ this patch.
(In reply to comment #39)
> I know that roc disagrees with me on this matter, so he should chime in at
> least, before you make changes.

I yield. Go ahead with SendMouseEventDirectly. Maybe call it SendMouseEventToWindow?
(In reply to comment #39)
> I think that we should not change nsIDOMWindowUtils::SendMouseEvent. It should
> continue to dispatch events via the widget. I don't think that not being able
> to click on things that aren't visible is a bug.
Well, Fennec's mouse event handling is based on that bug.
(Not that I've ever liked Fennec's event handling)


> I think it would be "magic" if
> it could do that. Since we use SendMouseEvent heavily in our tests I think that
> it should match the mouse events that a real user produces as closely as
> possible, and that means dispatching them on the root widget, even if that
> widget is the root chrome widget.
> 
> I think we should have a new function, SendMouseEventDirectly perhaps, that
> bypasses the widget and sends it directly to the presshell. This function can
> have the "magic" of being able to click on invisible elements. Fennec can use
> this new function.
Sounds ok to me.
We need methods also for other events, like mousescroll and gestures
But actually, how does SendMouseEvent work with 130078,
especially coordinates. Where is the event dispatched.
I think we have similar problem already with content iframes, since those don't
have their own widget atm, right?
(In reply to comment #42)
> But actually, how does SendMouseEvent work with 130078,
> especially coordinates. Where is the event dispatched.

It gets dispatched to the nearest widget, which could be for any ancestor document. Then it goes through the view subsystem and to the presshell for the root chrome document. PresShell::HandleEvent gets the frame for the coordinates of the event here http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6360 and then here http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6404 it lets the presshell that owns that frame handle the event. The same way that SendMouseEvent is handled currently in iframes.

> I think we have similar problem already with content iframes, since those don't
> have their own widget atm, right?

Correct. This is why I think that SendMouseEvent is not buggy: because the majority of our tests run in an iframe without a widget currently, and our tests use SendMouseEvent a lot, and it hasn't seemed to caused any problems yet. I think that the problem here is very-Fennec specific.
Attachment #469063 - Flags: feedback?(tnikkel)
Actually, only hidden, or "too small" iframes has the problem atm.
I changed my mind. IMO we should fix the current methods. Why should we add new ones?
If someone wants to send
events possibly to higher-up-in-the-DOMWindow-hierarchy, then they should use
DOMWindowUtils of that window.
But I need to re-post the patch to tryserver. The test results of the previous
time got lost.
Argh, tryserver is closed atm.
Comment on attachment 469112 [details] [diff] [review]
possible patch

Just applying this to trunk seems to break some tests.
Attachment #469112 - Flags: review?(tnikkel)
Of course, the patch would break menu handling.

Perhaps the other, rather ugly approach is needed after all.
Ok, let's go with what you call the ugly approach. :)
Actually, that isn't good either since then the new methods wouldn't 
work properly in all cases.
If both ways have flaws then the most logical thing to do is expose both and let the consumer use the one that fits their needs.
Attached patch add a new method — — Splinter Review 
smaug's not around and I need a patch here so I can land 130078 that will fix Fennec and go green on tinderbox.
Attachment #470042 - Flags: review?(roc)

        Attachment #470043 -
        Flags: review?(webapps)

    
    

    
  
Comment on attachment 470042 [details] [diff] [review]
add a new method

Rev nsIDOMWindowUtils IID

The methods probably need better clarifying comments. Make it clear that sendMouseEventToWindow will ensure the event is dispatched somewhere under this DOMWindow, whereas sendMouseEvent will dispatch via the toplevel window (and therefore the event could end up being handled not under this DOMWindow).

        Attachment #470042 -
        Flags: review?(roc) → review+

    
    

    
  
Comment on attachment 470043 [details] [diff] [review]
make fennec use the new method

r=me to land this

        Attachment #470043 -
        Flags: review+

        Attachment #470043 -
        Flags: review?(webapps) → review+

    
    

    
  
(In reply to comment #51)
> If both ways have flaws then the most logical thing to do is expose both and
> let the consumer use the one that fits their needs.

The most logical thing is to fix the only method to work properly.

    
    

    
  
Please file a followup to fix this all properly. Please.

    
    

    
  
Olli, so your position is that SendMouseEvent should ensure it dispatches events to content in or under the DOMWindow it's invoked on? And clients that want to dispatch events to whatever's topmost in the browser window should find the DOMWindowUtils for the toplevel window and use that?

I agree with you. However, Timothy argues that that's actually a behavior change for SendMouseEvent. Currently SendMouseEvent walks up to the nearest DOMWindow with a widget and fires the event there --- and for all mochitests running in the harness, the mochitest DOMWindow does *not* have a widget, so we'd be potentially changing the behaviour of those tests. I think that's a valid point, although I think that changing its behaviour to do what you and I want is the best thing to do anyway.

    
    

    
  
(In reply to comment #59)
> Olli, so your position is that SendMouseEvent should ensure it dispatches
> events to content in or under the DOMWindow it's invoked on? And clients that
> want to dispatch events to whatever's topmost in the browser window should find
> the DOMWindowUtils for the toplevel window and use that?
Right.
 
> I agree with you. However, Timothy argues that that's actually a behavior
> change for SendMouseEvent.9
I know. SendMouseEvent (and other similar methods) have been just a bit
broken since removing widgets from iframes.

And there is still the issue how to handle menus. They have still their own
native widget, right?

    
    

    
  
PresShell has code to find any active popups and dispatch mouse events to them if the popup is under the mouse position. This only happens for events dispatched to the root prescontext. This means that SendMouseEvent will work on popups if you're using the root DOMWindow.

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/84bb94187b86
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED

    
    

    
  
(In reply to comment #58)
> Please file a followup to fix this all properly. Please.

I don't know what you see as a proper fix for this, so could you file this followup?

    
    

    
  
(In reply to comment #59)
> I agree with you. However, Timothy argues that that's actually a behavior
> change for SendMouseEvent. Currently SendMouseEvent walks up to the nearest
> DOMWindow with a widget and fires the event there --- and for all mochitests
> running in the harness, the mochitest DOMWindow does *not* have a widget, so
> we'd be potentially changing the behaviour of those tests. I think that's a
> valid point, although I think that changing its behaviour to do what you and I
> want is the best thing to do anyway.

The change in behaviour of SendMouseEvent is a minor point. My biggest concern is that we use SendMouseEvent in our tests heavily to test mouse interactions from the users. It is basically the only way to do that. So I think it should match mouse events sent from a real users moving/clicking a mouse as closely as we can. If we instead direct mouse events to a certain presshell of our choosing then we bypass all the code which determines where the event should be handled that executes every time the user touches the mouse.

    
  
Assignee: nobody → tnikkel
Component: Layout: View Rendering → Layout: Web Painting

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: