Closed Bug 185107 Opened 22 years ago Closed 22 years ago

[FIX]Popup menu after right-click inside an iFrame is in wrong place.

Categories

(Core :: XUL, defect, P1)

x86
All
defect

Tracking

()

VERIFIED FIXED
mozilla1.3beta

People

(Reporter: d_king, Assigned: bzbarsky)

References

()

Details

(Keywords: regression, Whiteboard: [adt3])

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20021212
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20021212

Normally when I click on the Intellicast Weather link using the right button on
the mouse, the popup menu appears right next to the cursor. This works fine for
the two Weather.com links.

Since a few days ago, this has stopped working properly.

Reproducible: Always

Steps to Reproduce:
1. Right-Click on Weather.com link and see it works.
2. Right-Click on Intellicast link (far right link) and see it doesn't work.
3.

Actual Results:  
Popup menu is way off to the left of the cursor.

Expected Results:  
Popup to appear in same way as Weather.com links, and as it used to.

I will add a simplified test case ASAP.
This sample has three links. The two on the left are links to Weather.com, and
right-clicking works fine. The third link is to Intellicast using an iFrame and
right-clicking doesn't work as it used to.
Keywords: regression
Changing component.
Assignee: asa → jaggernaut
Component: Browser-General → XP Toolkit/Widgets
QA Contact: asa → jrgm
This regressed between the daily build of 12/07 am, and 12/08 am. 
Reviewing the checkins for that period, this has to be bz's change to 
nsMenuPopupFrame.cpp for bug 180329.

(As prior history, see also bug 129782, which concerned <select>'s 
popups being mispositioned inside <iframe>'s).
Keywords: mozilla1.3, nsbeta1
Hmm..  Well, all that checkin did was make the fix for bug 172276 "correct" (as
in, working).  It's possible that that patch is somehow incorrect in other ways
(though it's not clear to me how, exactly).  For example, what's the window type
on iframe widgets?

Also, is this a problem on Windows only, or XP?
Well, I'm seeing this on WinXP. But I don't know if anyone has dupped the
problem with any other version of Windows, or any other OS.
Flags: blocking1.3b?
XP == "Cross platform" not "whatever the hell MS is calling it now"
I see this on Redhat 6.1 and Win2k.
jrgm, do the GetRootViewForPopup and GetWidgetForView calls return non-null in
the failing cases?
One sec. I have to pull your checkin into my debug build.
> bz: parentView, targetDocumentWidget are both non-null from those calls.
<bz> jrgm: erm.  that is Wrong
<bz> jrgm: but certainly explains the bug... ;(
> bz: that's in either the 'good' case [no iframe] and the 'bad' case [in 
iframe].
<bz> jrgm: non-null means that the target of the context menu is inside a 
menupopup
<bz> jrgm: which is most definitely not the case here
<bz> jrgm: it should be null in both cases
<bz> jrgm: yes
<bz> jrgm: both cases are wrong
> bz: okay, I'll put that in the bug.
> bz: although, my build is from a pull Saturday night, plus pulling your 
checkin to
>     nsMenuPopupFrame.cpp...
> bz: am I missing something else I need to make this all work right?
<bz> jrgm: doubtful....
So... the problem seems to be that GetRootViewForPopup (in nsMenuPopupFrame.cpp)
always returns a view.  If it hits a view with a widget whose window type is
eWindowType_popup it returns that view.  Otherwise it walks up till it hits a
view with no parent, then returns that.

Now I suspect that the root view of the <iframe>'s viewmanager _does_ have a
parent, since view managers form a tree now (roc, is that right?)  So we end up
getting a view (and widget) from the new codepath and not taking the old
codepath at all (the one that used the root view of the viewmanager and got the
closest widget that contained it).  And the widget/view we get now are those for
the big browser window, not the <iframe> like in the old codepath...

Oh, and the mixing of GetWidgetForView and GetOffsetFromWidget here to do the
same task bothers me... 
> Now I suspect that the root view of the <iframe>'s viewmanager _does_ have a
> parent, since view managers form a tree now (roc, is that right?)

Correct.

Perhaps this code should walk until it finds a view for a popup or it finds a
view which is the root view for its view manager. You can check the latter
condition using view->GetViewManager()->GetRootView() == view, or you can use
view->GetParent() == null || view->GetParent()->GetViewManager() !=
view->GetViewManager().
Not quite that easy... there are 3 callers of GetRootViewForPopup and the other
two depend on its current behavior (that of returning the view of the popup
window or failing that of the main root window).

I suppose we could pass in a boolean telling GetRootViewForPopup whether to keep
walking through the viewmanager root...

I know hyatt doesn't read his bugmail; ccing the reviewers off bug 172276 in
case they have any useful ideas.
*** Bug 185299 has been marked as a duplicate of this bug. ***
*** Bug 185244 has been marked as a duplicate of this bug. ***
This is a pretty high visibility regression. Adding to the blocking 1.3a list. 
Flags: blocking1.3b? → blocking1.3b+
*** Bug 185726 has been marked as a duplicate of this bug. ***
OS -> All according to bug 185726
OS: Windows XP → All
*** Bug 185730 has been marked as a duplicate of this bug. ***
*** Bug 185947 has been marked as a duplicate of this bug. ***
*** Bug 185956 has been marked as a duplicate of this bug. ***
*** Bug 186143 has been marked as a duplicate of this bug. ***
*** Bug 186475 has been marked as a duplicate of this bug. ***
FYI,

This is still happening on build 2002122604-TRUNK. So nothing that has been
checked in recently has "accidently" fixed this problem.

I have a debug buld of Mozilla on my machine, so if someone can point me in the
general direction of what code might be causing this, I'll take a quick look.
(I'm afraid that XP Toolkit/Widgets doesn't mean much to me).
See comment 11 through comment 13 and the patch in bug 180329; those make it 
very clear what code is responsible....
Nav triage team: nsbeta1+/adt3
Keywords: nsbeta1nsbeta1+
Whiteboard: [adt3]
Thanks for the pointers. From looking at the code, which is way beyond my
expertise, I'll go for testing any patch that is developed.

Blocks: 187026
Blocks: 187604
Attached patch hacky, hacky (obsolete) — Splinter Review
I suppose I could take this....
Assignee: jaggernaut → bzbarsky
Priority: -- → P1
Summary: Popup menu after right-click inside an iFrame is in wrong place. → [FIX]Popup menu after right-click inside an iFrame is in wrong place.
Target Milestone: --- → mozilla1.3beta
Comment on attachment 110720 [details] [diff] [review]
hacky, hacky

reviews?
Attachment #110720 - Flags: superreview?(jaggernaut)
Attachment #110720 - Flags: review?(hyatt)
Comment on attachment 110720 [details] [diff] [review]
hacky, hacky

Hrm. I'll have to study this code more before I'll give sr. I could give an rs
though, provided you fix "aStopAtView_m_anagerRoot" to be properly iCap'ed (see
surrounding code), and remove the fprintf.
Attached patch sure thingSplinter Review
Attachment #110720 - Attachment is obsolete: true
Attachment #110720 - Flags: superreview?(jaggernaut)
Attachment #110720 - Flags: review?(hyatt)
Attachment #110721 - Flags: superreview?(jaggernaut)
Attachment #110721 - Flags: review?(hyatt)
Comment on attachment 110721 [details] [diff] [review]
sure thing

r=hyatt
Attachment #110721 - Flags: review?(hyatt) → review+
Comment on attachment 110721 [details] [diff] [review]
sure thing

sr=jag
Attachment #110721 - Flags: superreview?(jaggernaut) → superreview+
fixed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Verified on WinXP with 20030109 build. Many thanks to those who worked to get
this fixed.
Status: RESOLVED → VERIFIED
*** Bug 188378 has been marked as a duplicate of this bug. ***
*** Bug 188892 has been marked as a duplicate of this bug. ***
*** Bug 191081 has been marked as a duplicate of this bug. ***
*** Bug 192303 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: