Closed Bug 1083724 Opened 5 years ago Closed 5 years ago

menu's position is wrong

Categories

(Core :: XUL, defect)

36 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla36
Tracking Status
firefox35 --- unaffected
firefox36 + verified

People

(Reporter: c, Assigned: khuey)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

Attached image 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
Blocks: 979835
Do you have steps to reproduce? Does it appear only with a specific size of the browser window?
(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.
Version: Trunk → 36 Branch
[Tracking Requested - why for this release]: Clear UI bug
Flags: needinfo?(khuey)
Flags: needinfo?(bzbarsky)
OS: Windows NT → Windows 8.1
Duplicate of this bug: 1084146
I'll look at this tomorrow.
Duplicate of this bug: 1084219
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.
OS: Windows 8.1 → All
QA Contact: cornel.ionce
Hardware: x86_64 → All
Duplicate of this bug: 1083910
Duplicate of this bug: 1084332
Duplicate of this bug: 1084417
Duplicate of this bug: 1084900
Duplicate of this bug: 1084903
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?
Assignee: nobody → khuey
Flags: needinfo?(khuey)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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?
(which, in my totally amateur understanding, would be like the inverse of bug 936634 and would help with undefined becoming "undefined" instead of "")
(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.
> It looks like TreatUndefinedAs got removed from WebIDL?

Right, because it was equivalent to |optional DOMString arg = ""|.
Flags: needinfo?(bzbarsky)
(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.
> 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.
(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 :/
Yeah, it did.  Hopefully no more changes like that now that sequences have finally gotten repurposed as iterables... ;)
Attached patch PatchSplinter Review
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.
Attachment #8507584 - Flags: review?(bzbarsky)
Component: General → XUL
s/edit/audit/

Clearly not in the state of mind for it today :)
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
Attachment #8507584 - Flags: review?(bzbarsky) → review+
Duplicate of this bug: 1085675
Duplicate of this bug: 1086243
https://hg.mozilla.org/mozilla-central/rev/9601c62261c9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1087267
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.