Closed Bug 562554 Opened 14 years ago Closed 14 years ago

Toolbar buttons with drop-down menus are still busted in SeaMonkey Modern Theme

Categories

(Core :: XUL, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.3a5

People

(Reporter: philip.chee, Assigned: Felipe)

References

Details

(Keywords: regression)

Attachments

(1 file)

In a way this is the opposite of Bug 557987 in that only the main button mouse handler is called although the mouse is clicked on the drop-down marker. Perhaps you went too far in the other direction?

Note that this only occurs in SeaMonkey Modern. Can someone test this in Firefox using a theme that doesn't use native widgets?
Testing in Seamonkey, the difference seems to rely that on the default theme, <toolbarbutton type="menu-button"> is implemented as

<content>
  <toolbarbutton />
  <dropmarker />
</content>


and on the Modern Theme, it is:

<content>
  <stack>
    <toolbarbutton />
    <dropmarker />
  </stack>
</content>


it appears that on the second case, the dropmarker never gets the hit.  I'm building Seamonkey now to investigate why is that happening
http://mxr.mozilla.org/comm-central/source/suite/themes/modern/global/globalBindings.xml#11 is where the <stack> gets added, FWIW.

Still, this shouldn't destroy behavior that way, I think...
Ok, I found the problem, and it's due to a subtle difference on the behavior of nsDisplayXULEventRedirector (which redirects clicks from the children of a nsMenuFrame to the frame itself), combined with the use of stack here.

( http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsBoxFrame.cpp?mark=2111-2116,2147#2111 )

Before the HitTest changes for nodesFromRect, the XULEventRedirector would check the topmost frame on point X/Y and its parents for allowevents=true. If none had it, the event is redirected at the menu frame.

Now, it does the same check not only for the topmost, but for all the frames on the point X/Y.  Given this:

-- nsMenuFrame --
  <stack>
    <toolbarbutton allowevents="true" />
    <dropmarker />
  </stack>
-- --

since they are stacked together, a click on the dropmarker will eventually see allowevents=true from the toolbarbutton, and won't redirect the event to the menu frame.



I don't know what would be the best way to fix this. One possible solution would be to do this:  if the first frame and none of its ancestors have allowevents=true, add the mTargetFrame as first element to the list of hit frames.

It looks to me that this would make it go back to the same behavior as before, while allowing the list to contain all hittable frames as is the purpose of the new hittest.


Roc, what do you think of this?
Attached patch Proposed fixSplinter Review
This implements the solution above. I'm not sure that the current behavior is in fact _incorrect_, but it looks like the XULEventRedirector is already a kind of a hack for menuframes, so keeping it on the old behavior is probably the best thing to do.
Attachment #442937 - Flags: review?(roc)
Pushed as: http://hg.mozilla.org/mozilla-central/rev/3a26cbe20cd2

(note I am also the guinea pig for the tree opening up [metered] so there is a [small] chance this won't stick)
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
(In reply to comment #7)

> Pushed as: http://hg.mozilla.org/mozilla-central/rev/3a26cbe20cd2

Looks like the patch works for me on my SM-Trunk 20100510132520

Thank you.
Status: RESOLVED → VERIFIED
Can someone explain the test that was added here. The test contains the following markup:

<toolbarbutton type="menu" id="toolbarmenu" height="200px">
  <menupopup id="menupopup" onpopupshowing="eventReceived('popupshowing'); return false;"/>
  <stack>
    <button width="100px" left="0px" height="30px" id="button1"
            allowevents="true"
            onclick="eventReceived('clickbutton1'); return false;"/>
    <button width="100px" left="70px" id="button2"
            onclick="eventReceived('clickbutton2'); return false;"/>
  </stack>
</toolbarbutton>


However, the binding which is attached to the <toolbarbutton> has the following anonymous content:

<content>
  <children includes="observes|template|menupopup|panel|tooltip"/>
  <xul:image class="toolbarbutton-icon"
             xbl:inherits="validate,src=image,label,type"/>
  <xul:label class="toolbarbutton-text" crop="right" flex="1"
             xbl:inherits="value=label,accesskey,crop,dragover-top"/>
  <xul:dropmarker type="menu" class="toolbarbutton-menu-dropmarker"
                  xbl:inherits="disabled,label"/>
</content>

As you can see there is no insertion point for the <stack> element. The current XBL code kind of freaks out in this situation and doesn't create any anonymous content.

I'm currently in the process of changing the XBL code to be more sensible and actually honor what is being written. That means that we won't create any rendering for nodes which doesn't have insertion points.

However this breaks the current test as it means that the <stack> isn't rendered and thus doesn't receive any clicks. So far this test is the only confirmed instance of where we rely in the current XBL behavior of essentially ignoring the binding.

How can I fix this test without breaking what it's intending to test?
it's testing an odd interaction between elements stacked together in a menu frame and the allowevents attr:

<toolbarbutton type="menu">
  <stack>
    <button id="b1" allowevents="true">
    <button id="b2" />
  </stack>
</toolbar>


Button 2 should be on top of button 1. Notice that button 2 does not have allowevents="true" (and the stack doesn't have it either). The test is testing that a click on button 2 is redirected to the parent element (the toolbarbutton) and not to the element underneath (button 1).

( See this structure in use here: http://mxr.mozilla.org/comm-central/source/suite/themes/modern/global/globalBindings.xml#11 )

You could get rid of the stack by positioning the elements through CSS, and it'd still be testing the same thing. But the toolbarbutton binding won't accept <button>s as children either :/.  So perhaps include an extending binding in the test file itself that does accept a stack?


But the parent needs to be an element that generates a nsDisplayXULEventRedirector. http://mxr.mozilla.org/mozilla-central/ident?i=WrapListsInRedirector


(hope this explanation is not too confusing.. stop by my desk if you need more info)
Sicking, note that Bug 508710 will move this test from mochi-plain to mochi-chrome
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.