Closed Bug 516615 Opened 15 years ago Closed 15 years ago

Zimbra emails don't open on nightly build

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jboriss, Assigned: enndeakin)

References

Details

(Keywords: dogfood, regression)

Attachments

(1 file, 2 obsolete files)

fix test by removing selection first
11.20 KB, patch
roc
: review+
smaug
: superreview+
Details | Diff | Splinter Review 
Zimbra loads in the nightly build, but clicking on an email to open it does not give a response other than the cursor changing from a hand to an arrow.
In Error Console: 

Warning: The 'charCode' property of a keyup event should not be used. The value is meaningless.
Source file: https://mail.mozilla.com
Build identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a1pre) Gecko/20090914 Minefield/3.7a1pre
Not a JS bug in any case.

/be
Assignee: general → nobody
Status: NEW → RESOLVED
Closed: 15 years ago
Component: JavaScript Engine → DOM: Events
QA Contact: general → events
Resolution: --- → DUPLICATE
Sorry, not a dup. What's going wrong?

/be
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment 1 shows just a valid warning. That shouldn't cause any problems.

What is the regression range for this?
2009-09-13 works, -14 doesn't
Somehow caused by bug 503943.
Blocks: 503943
Zimbra does use element.setCapture()/.releaseCapture() on body element.
Do we not dispatch click or what?
The click event is not firing because the mouseup is occurring on a different element (the body) instead of whatever was clicked on.

Now normally, even when mouse capturing is used, events should still reach descendants of the element doing the capturing. However, in this particular case, almost all content is absolutely positioned, so the frames are outside the body.

One possible solution is to just treat a capture on a <body> tag as a capture on the root element instead.
The question is: how does IE handle this case?
I can't really test it for a while, but presumably it doesn't consider its positioned elements to be outside of the body. Or, its different event model might cause it to fire on the right element anyway.
Attached patch testing this right now (obsolete) — — Splinter Review 
if we want to take this route, here is a patch that does it
Assignee: nobody → enndeakin
Status: REOPENED → ASSIGNED
So IE just treats:

<body>
  <p style="position:absolute">...</p>
</body>

as the <p> being inside the body. Whereas Mozilla's frame tree has <p> outside the body.

Maybe an alternative is some modification to GetFrameForPoint.
setCapture works on DOM, not on layout object tree, right?
Presshell could perhaps traverse DOM and not use frame tree - 
at least if that is what IE does.
From MSDN
"When the bContainerCapture parameter is set to true, a container object, such as a  div, captures mouse events for all objects in it. By passing the value false, objects in that container can fire events, and cancel event bubbling."
I interpret that so that setCapture works on DOM.

Needs testing, ofc.
I'm not clear what you're asking here. setCapture causes a pointer to an nsIContent to be stored indicating which element is capturing the mouse. When a mouse event occurs, the primary frame for it is retrieved and uses the normal mouse hit testing which uses frames and display lists to find a child within that frame to target at.
I mean something like...
If there is capturing content, presshell starts currently from the primary frame of the capturing content and then tries to find the target frame inside that.
But I think it should work other way when !gCaptureInfo.mRetargetToElement.
First use the normal hit testing, then if the target frame's content is a descendant of the capturing content, normal event handling can be used.
Does this make sense?

Btw, is it possible (and on purpose) that when !gCaptureInfo.mRetargetToElement,
targetFrame can be in some subshell and the event is dispatched there?
(In reply to comment #18)
> But I think it should work other way when !gCaptureInfo.mRetargetToElement.
> First use the normal hit testing, then if the target frame's content is a
> descendant of the capturing content, normal event handling can be used.
> Does this make sense?

That makes sense to me.
This really sucks for me. Can we get this fixed or backed out or something?
Keywords: dogfood
Attached patch implement as discussed (obsolete) — — Splinter Review 
The new test fails on Windows, but I can't test there currently.
Attachment #400775 - Attachment is obsolete: true

        Attachment #401566 -
        Attachment is obsolete: true

        Attachment #402119 -
        Flags: superreview?(Olli.Pettay)

        Attachment #402119 -
        Flags: review?(roc)

    
    

    
  
So (In reply to comment #22)
> Created an attachment (id=402119) [details]
So is this the previous patch + selection removal?

    
    

    
  
Sorry, the comment refers to adding a line to clear the selection in the test, a change from the previous patch.

    
    

    
  
Comment on attachment 402119 [details] [diff] [review]
fix test by removing selection first

>+    // if a node is capturing the mouse, check if the event needs to be
>+    // retargeted at the capturing content instead. This will be the case when
>+    // capture retargeting is being used, no frame was found or the frame's
>+    // content is not a descendant of the capturing content.
>+    if (capturingContent &&
>+        (gCaptureInfo.mRetargetToElement ||
>+         !targetFrame || !targetFrame->GetContent() ||
>+         !nsContentUtils::ContentIsCrossDocDescendantOf(targetFrame->GetContent(),
>+                                                       capturingContent))) {
Nit, add a space before capturingContent.

Does IE also allow sub documents to capture the event when not retargeting to the
element? If it does the same what this patch, then sr=me.

Or if it doesn't do that, then you'll probably need to use 
ContentIsDescendantOf(). if that works the same way as IE, sr=me

        Attachment #402119 -
        Flags: superreview?(Olli.Pettay) → superreview+

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/0580094398f3
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED

    
  
Flags: in-testsuite+

    
  
OS: Mac OS X → All
Hardware: x86 → All

    
  
Blocks: 519693
Depends on: 668640

    
  
Depends on: 623432

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: