213103 - Events should go to capturing views even when clip rect is empty [Patch included]

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P1)

Description

[Stef Walter]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030716

Tree type splitters don't work inside of a scrolled view. After researching the
problem, I found the issues to be in nsViewManager where events aren't in some
cases delivered to views that have an empty clip rect, even when that view is
capturing.

Included is a patch. I need this fixed before I can complete bug #212789

Reproducible: Always

Steps to Reproduce:
1. Open XUL attachment
2 [review]. Note how tree splitters on top bar work fine, but splitters 
   inside the scrolled view cause a mess.


Actual Results:  
Splitters in top set of tree columns work, bottom set doesn't.

Expected Results:  
Behavior should be identical in the two cases, as all that's present is an
additional view (overflow: -moz-scrollbars-borizontal)

Comment 1

[Stef Walter]

Attachment: Test case

Comment 2

[Stef Walter]

Attachment: Patch to fix the issue

Comment 3

[Christopher Hoess (gone)]

Nate, you should request r= and sr= on the patch, maybe from roc and dbaron? 
(I'm not quite sure who the best candidates are, asking around #mozilla may 
help.)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you attach a diff -pu12 (unified diff, more context, show function name)?
 I can't tell what function you're modifying.

Comment 5

[Stef Walter]

Attachment: The same patch (with diff -pu12)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Perhaps it would be better to pass the |aEventProcessing| argument through to
AddToDisplayList and make this change conditional on that?

Really, though, it seems like the right way to fix this would really be to make
nsViewManager::HandleEvent only use BuildEventTargetList when it receives a
mouse event for which aCaptured is false.  But that would surely break something...

Comment 7

[Stef Walter]

Your call. I'm unfamiliar with the view manager code. What I don't understand is
why this only occurs when inside another view (the scrolling one). 

Comment 8

[Stef Walter]

Attachment: Patch incorporating dbaron's suggestion

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

A simpler patch would be to just remove the empty-clip-rect check:

-  if (empty) {
-    return PR_FALSE;

The check is not really needed since normally an empty cliprect will mean an
empty intersection is detected below.

Note that aAssumeIntersection should get set for the capturing view so your
events should be received. Try it and see.

Comment 10

[Stef Walter]

Yes this is what the original patch did. attachment #212789 [details] [diff] [review]

Comment 11

[Stef Walter]

Attachment: Simple patch as per roc's comment (#9)

Bad attachment id above. Here's the simple patch again to avoid confusion.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 128246 [details] [diff] [review]
Simple patch as per roc's comment (#9)

oh great! r+sr=roc+moz

but don't bother commenting all this out. Just remove the code. We don't need
gravestones for dead bugs.

Nice catch BTW. good work!

Comment 13

[Stef Walter]

Over to roc for check-in.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 128246 [details] [diff] [review]
Simple patch as per roc's comment (#9)

simple fix for a simple bug

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in. Thanks Nate!!!

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

actually marking FIXED