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)
Tracking
()
VERIFIED
FIXED
mozilla1.3beta
People
(Reporter: d_king, Assigned: bzbarsky)
References
()
Details
(Keywords: regression, Whiteboard: [adt3])
Attachments
(2 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
hyatt
:
review+
jag+mozilla
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•22 years ago
|
||
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.
Reporter | ||
Updated•22 years ago
|
Keywords: regression
Comment 2•22 years ago
|
||
Changing component.
Assignee: asa → jaggernaut
Component: Browser-General → XP Toolkit/Widgets
QA Contact: asa → jrgm
Comment 3•22 years ago
|
||
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
Assignee | ||
Comment 4•22 years ago
|
||
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?
Reporter | ||
Comment 5•22 years ago
|
||
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?
Assignee | ||
Comment 6•22 years ago
|
||
XP == "Cross platform" not "whatever the hell MS is calling it now"
Comment 7•22 years ago
|
||
I see this on Redhat 6.1 and Win2k.
Assignee | ||
Comment 8•22 years ago
|
||
jrgm, do the GetRootViewForPopup and GetWidgetForView calls return non-null in
the failing cases?
Comment 9•22 years ago
|
||
One sec. I have to pull your checkin into my debug build.
Comment 10•22 years ago
|
||
> 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....
Assignee | ||
Comment 11•22 years ago
|
||
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().
Assignee | ||
Comment 13•22 years ago
|
||
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.
Comment 14•22 years ago
|
||
*** Bug 185299 has been marked as a duplicate of this bug. ***
Comment 15•22 years ago
|
||
*** Bug 185244 has been marked as a duplicate of this bug. ***
Comment 16•22 years ago
|
||
This is a pretty high visibility regression. Adding to the blocking 1.3a list.
Flags: blocking1.3b? → blocking1.3b+
Comment 17•22 years ago
|
||
*** Bug 185726 has been marked as a duplicate of this bug. ***
Comment 19•22 years ago
|
||
*** Bug 185730 has been marked as a duplicate of this bug. ***
Comment 20•22 years ago
|
||
*** Bug 185947 has been marked as a duplicate of this bug. ***
Comment 21•22 years ago
|
||
*** Bug 185956 has been marked as a duplicate of this bug. ***
Comment 22•22 years ago
|
||
*** Bug 186143 has been marked as a duplicate of this bug. ***
Comment 23•22 years ago
|
||
*** Bug 186475 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 24•22 years ago
|
||
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).
Assignee | ||
Comment 25•22 years ago
|
||
See comment 11 through comment 13 and the patch in bug 180329; those make it
very clear what code is responsible....
Comment 26•22 years ago
|
||
Nav triage team: nsbeta1+/adt3
Reporter | ||
Comment 27•22 years ago
|
||
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.
Assignee | ||
Comment 28•22 years ago
|
||
Assignee | ||
Comment 29•22 years ago
|
||
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
Assignee | ||
Comment 30•22 years ago
|
||
Comment on attachment 110720 [details] [diff] [review]
hacky, hacky
reviews?
Attachment #110720 -
Flags: superreview?(jaggernaut)
Attachment #110720 -
Flags: review?(hyatt)
Comment 31•22 years ago
|
||
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.
Assignee | ||
Comment 32•22 years ago
|
||
Attachment #110720 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #110720 -
Flags: superreview?(jaggernaut)
Attachment #110720 -
Flags: review?(hyatt)
Assignee | ||
Updated•22 years ago
|
Attachment #110721 -
Flags: superreview?(jaggernaut)
Attachment #110721 -
Flags: review?(hyatt)
Comment 33•22 years ago
|
||
Comment on attachment 110721 [details] [diff] [review]
sure thing
r=hyatt
Attachment #110721 -
Flags: review?(hyatt) → review+
Comment 34•22 years ago
|
||
Comment on attachment 110721 [details] [diff] [review]
sure thing
sr=jag
Attachment #110721 -
Flags: superreview?(jaggernaut) → superreview+
Assignee | ||
Comment 35•22 years ago
|
||
fixed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 36•22 years ago
|
||
Verified on WinXP with 20030109 build. Many thanks to those who worked to get
this fixed.
Status: RESOLVED → VERIFIED
Comment 37•22 years ago
|
||
*** Bug 188378 has been marked as a duplicate of this bug. ***
Comment 38•22 years ago
|
||
*** Bug 188892 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 39•22 years ago
|
||
*** Bug 191081 has been marked as a duplicate of this bug. ***
Comment 40•22 years ago
|
||
*** 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.
Description
•