281635 - Links in pages with frames may behave incorrect/wrong. [unnecessary onchange event]

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Schauwi]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050203 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050203 Firefox/1.0+

I have a simple Homepage with two frames (devided horzontally).
In the left I have a drop-down-Link-Box (created with GoLive 6.0,
to select links from a drop-down-box) with links that should be 
loaded in the right of my two frames.
This works fine, but if I click any link on the newly displayed page 
in the right frame, suddenly the page is overwritten by the content 
of my left frame, so I see the drop-down-box in both frames.
This happens every-time I try and it began using the Nightly Built
later than this (2005-02-03)


Reproducible: Always

Steps to Reproduce:
1.Visit the link I attached with any Firefox 1.0+ Built later than 2005-02-03
2.Select any target from the dropdown box in the left frame
3.Click on any link on the page that was loaded in the right frame

Actual Results:  
The page loaded in the right frame dissappears and is overwritten
by the same page that is loaded in the left frame.

Expected Results:  
It should have opened the link I clicked on in the right frame,
'thats what's normal browsing is like'.

I don't know why Firefox messes up clicked links and since it works in IE and
Firefox 1.0 i addressed it as a major broken feature.

Comment 1

[OstGote!]

Works for me with Mozilla trunk build 2005021305 on WinNT4.

Can you reproduce it with a current FF trunk build? In the last time there were
some fixes in this area.

Comment 2

[OstGote!]

I revoke my comment 1. It does not work always. Strange. If it does not work,
and I select a new site on the left-frame dropdown menue, the new site will open
in a new window sometimes or I see your behavior.

Steps: Load your page, select on the left Google News and on the right side
click the 1st Google headline. Then I see your results. But not always.

Comment 3

[OstGote!]

Again... I see the following most of the time in the right frame if I open the
testcase:

a) original right frame -[select a site on the left]-> 
b) new site in right frame -[click a link on the site]-> 
c) left frame shows in right-side frame -> 
d) clicked link shows in right frame (as expected)

Most of the time I see c) only with back/forward, I guess it is too fast to see.
The transition from c-d seems sometimes "broken" so you see at the end only c)
like you (and me sometimes).

Boris could you please look at this? Thanks.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So... I cannot reproduce this with either 2005-02-03-05 or 2005-02-04-05 builds.

I _do_ see it with a 2005-02-05-06 build.  The most suspicious change in that
date range is for bug 279868.

Steps to reproduce reliably:

1)  Load the page
2)  Select "Google News" in the <select>
3)  Hit tab.

The problem is that the code in the page resets the selectedIndex after we make
the selection.  With the patch from bug 279868 that causes us to fire an
onchange event when the select is blurred, and that loads the currently selected
page in the right frame.  But the "currently selected page" has a URI of "#", so
we just load the left frame in the right frame.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimal-ish testcase

Comment 6

[Boris Zbarsky [:bzbarsky]]

So the problem is that the UpdateRecentIndex call causes us to fire the
onchange... We don't want that to be happening.  I think we should be passing
the _new_ index to UpdateRecentIndex().  That will ensure that an onchange does
not fire....

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

This is a pretty major regression; I think we should try to get this in for 1.8b1.

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 174384 [details] [diff] [review]
Proposed patch

This is not a complete fix. See upcoming testcase...

We are getting tripped by nsComboboxControlFrame::UpdateRecentIndex()
not updating the value unless it is -1:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/forms/nsComboboxCont
rolFrame.cpp&rev=1.305&root=/cvsroot&mark=2264-2270#2263

I tried UpdateRecentIndex(-1) instead and that seems to work.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm... why's onchange firing in that second testcase?  The recent selected index
is the same as the current selected index, no?

Comment 12

[Mats Palmgren (inactive)]

With current code, UpdateRecentIndex(aOldIndex):

nsListControlFrame::OnSetSelectedIndex - aOldIndex=-1 aNewIndex=0
nsComboboxControlFrame::UpdateRecentIndex(requested -1) current -1 => new -1
Document file:///home/mats/work/bug279868/onChange.html loaded successfully
nsListControlFrame::OnSetSelectedIndex - aOldIndex=0 aNewIndex=3
nsComboboxControlFrame::UpdateRecentIndex(requested 0) current -1 => new 0
nsComboboxControlFrame::UpdateRecentIndex(requested -1) current 0 => new -1
nsListControlFrame::FireOnChange - index=0 selectedIndex=3


with UpdateRecentIndex(aNewIndex):

nsListControlFrame::OnSetSelectedIndex - aOldIndex=-1 aNewIndex=0
nsComboboxControlFrame::UpdateRecentIndex(requested 0) current -1 => new 0
Document file:///home/mats/work/bug279868/onChange.html loaded successfully
nsListControlFrame::OnSetSelectedIndex - aOldIndex=0 aNewIndex=3
nsComboboxControlFrame::UpdateRecentIndex(requested 3) current 0 => new 0
nsComboboxControlFrame::UpdateRecentIndex(requested -1) current 0 => new -1
nsListControlFrame::FireOnChange - index=0 selectedIndex=3



with UpdateRecentIndex(-1):

nsListControlFrame::OnSetSelectedIndex - aOldIndex=-1 aNewIndex=0
nsComboboxControlFrame::UpdateRecentIndex(requested -1) current -1 => new -1
Document file:///home/mats/work/bug279868/onChange.html loaded successfully
nsListControlFrame::OnSetSelectedIndex - aOldIndex=0 aNewIndex=3
nsComboboxControlFrame::UpdateRecentIndex(requested -1) current -1 => new -1
nsComboboxControlFrame::UpdateRecentIndex(requested -1) current -1 => new -1
nsListControlFrame::FireOnChange - index=-1

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  The initial load also calls SetSelectedIndex, eh?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 15

[Mats Palmgren (inactive)]

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

When I think about it, removing the UpdateRecentIndex block
should also work.  Either way, r=mats

Comment 16

[Boris Zbarsky [:bzbarsky]]

No, I think if selectedIndex is set after the user has changed the value, we
don't want to fire an onchange when the control loses focus....

Comment 17

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Ah, I see what you mean now (BTW, this bug also occurred before bug 279868).
UpdateRecentIndex(-1) is what we want.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

preemptively approving for 1.8b

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 20

[Schauwi]

I'm Sorry to bother you again ... but something is still wrong:

Visit my test-page again and follow the instructions there 
(http://home.arcor.de/schlaubstar/firefox-bug/)

or:

1.) Create a two frame-Page (left and right frame)
2.) Create a drop-Down Box (like the ones you used here)
3.) Create a link to :   
    http://www.iww.uni-karlsruhe.de/lehre/veranstaltungen/WS0405/26272/
    that should open in the right frame
4.) Open your page, select the link above from the box on the left.
    In the Window on the right scroll down to the Section:
    "Übungen:"  In the last Sentence of this paragraph you can click on
    "Web-Server" ... do so.

I use the latest trunk (17-Feb.2005) and ... if I do so .., the right frame 
is always overwritten by ... ? I actually don't know ? Is it deleted ?

Comment 21

[Boris Zbarsky [:bzbarsky]]

When I follow the steps in comment 20, I get a file listing in the right frame
(as expected).

Re-resolving fixed.  If you see another problem (different from the original one
this bug was filed on), please file a _separate_ bug.  Cc me on it.  And give a
clear description of the problem.  Attach screenshots if needed.

Comment 22

[Mats Palmgren (inactive)]

(I have filed bug 282764 and I think it could be the same underlying problem as
 described in comment 20)