1083724 - menu's position is wrong

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[[e-mail]]

Attachment: menu.jpg

User Agent: Mozilla/5.0 (Windows NT 6.4; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141016140518

Steps to reproduce:

It's caused by Bug 979835

Comment 1

[Loic]

Do you have steps to reproduce? Does it appear only with a specific size of the browser window?

Comment 2

[[e-mail]]

(In reply to Loic from comment #1)
> Do you have steps to reproduce? Does it appear only with a specific size of
> the browser window?

latest inbound builds and new profile. Just press the button.

Comment 3

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Clear UI bug

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'll look at this tomorrow.

Comment 7

[Cornel Ionce [:noni] [Hubs QA]]

Also reproduces on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 12.04 and for the following cases:
- on the newtab page for "What is this page" pop-up.
- for the hint pop-up when first entering Customize mode.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So this is pretty dumb.  The issue is that various things call http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/popup.xml#37 and omit arguments.  But while XPConnect converts undefined to "" WebIDL converts it to "undefined".  The popup C++ code (in nsMenuPopupFrame) then treats the presence of something other than "" as an indication to throw away the correct value of the position variable and replace it with the result of parsing undefined (which is nothing, of course).

It looks like TreatUndefinedAs got removed from WebIDL?  Should I just add

  if (aPosition == undefined) {
    aPosition = "";
  }

in the XBL?

Comment 15

[:Gijs (he/him)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #14)
> So this is pretty dumb.  The issue is that various things call
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/popup.
> xml#37 and omit arguments.  But while XPConnect converts undefined to ""
> WebIDL converts it to "undefined".  The popup C++ code (in nsMenuPopupFrame)
> then treats the presence of something other than "" as an indication to
> throw away the correct value of the position variable and replace it with
> the result of parsing undefined (which is nothing, of course).
> 
> It looks like TreatUndefinedAs got removed from WebIDL?  Should I just add
> 
>   if (aPosition == undefined) {
>     aPosition = "";
>   }
> 
> in the XBL?

Shouldn't the webidl definition here just define position (and later args) as optional?

Comment 16

[:Gijs (he/him)]

(which, in my totally amateur understanding, would be like the inverse of bug 936634 and would help with undefined becoming "undefined" instead of "")

Comment 17

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #16)
> (which, in my totally amateur understanding, would be like the inverse of
> bug 936634 and would help with undefined becoming "undefined" instead of "")

Sigh @ copy paste fail. I meant bug 999315. Apologies for the spam.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> It looks like TreatUndefinedAs got removed from WebIDL?

Right, because it was equivalent to |optional DOMString arg = ""|.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to :Gijs Kruitbosch from comment #15)
> Shouldn't the webidl definition here just define position (and later args)
> as optional?

That doesn't help because the "optionalness" is lost when calling the XBL method.  The <method> implementation does not know which arguments were explicitly passed in and which were omitted.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> That doesn't help because the "optionalness" is lost when calling the XBL method. 

Web IDL treats the value "undefined" as "not passed" and uses the default value if there is one.   It doesn't matter for purposes of default value handling whether you omitted the value or explicitly passed undefined; the effect is the same.

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Boris Zbarsky [:bz] from comment #20)
> > That doesn't help because the "optionalness" is lost when calling the XBL method. 
> 
> Web IDL treats the value "undefined" as "not passed" and uses the default
> value if there is one.   It doesn't matter for purposes of default value
> handling whether you omitted the value or explicitly passed undefined; the
> effect is the same.

You're right, of course.  This all changed pretty drastically since I stopped paying close attention :/

Comment 22

[Boris Zbarsky [:bzbarsky]]

Yeah, it did.  Hopefully no more changes like that now that sequences have finally gotten repurposed as iterables... ;)

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

I should probably edit everything else we touched in that bug for more instances of this issue.  A bit much for a Sunday night though.

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

s/edit/audit/

Clearly not in the state of mind for it today :)

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8507584 [details] [diff] [review]
Patch

You don't need the optional bits on the long and boolean: undefined will convert to 0 and false for those types anyway.

Similarly, you don't need optional bit on nullable interface types; undefined ends up converting to null.

You do need it on position here, but that's it.

Also, might as well change showPopup and moveToAnchor while you're here.

I looked through https://bug979835.bugzilla.mozilla.org/attachment.cgi?id=8504450 and there are no instances of "in DOMString" in there, the only instances of "in wstring" are arguments to showPopup, and the only instances of "in AString" are openPopup and moveToAnchor, both on PopupBoxObject.

r=me

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/fx-team/rev/9601c62261c9

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9601c62261c9

Comment 30

[Ekanan Ketunuti]

verified fixed in 36.0a1 20141022030202.

- menu anchoring to the button
- can close the menu by click twice on the menu 
- customization hint tooltip is no longer cut off.