83033 - wrong selection after back button on query.cgi

Status: VERIFIED WORKSFORME

(Bugzilla :: Query/Bug List, defect, P1)

Description

[Erich 'Ricky' Iseli]

If you select "Program=Browser" and "Component=Style System" on the query page,
make your query and go back to the query page using the back-button, the
Component selection no longer is "Style System" but "HTML Frames" instead.

Seen with build 2001-05-23. I'm adding this information because this could be a
HTML bug and not a bugzilla one.

Comment 1

[Dave Miller [:justdave]]

I am unable to duplicate this with Netscape 4.77 or MSIE 5.1.1 on Mac OS X.

-> Mozilla

This is going to be either a JavaScript issue or a DOM issue because those items 
(even after going to the page with the Back button) are selected by javascript 
code.  The javascript remembers which ones were selected as the page opens and 
re-selects them after the component and milestone lists are rebuilt.  The 
generated code is available here for examination:
<http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/query.cgi#325>

Comment 2

[Johnny Stenback (:jst)]

Over to pollmann.

Comment 3

[GavinS]

I see a similar thing, but in my case, no Components are selected on recent
Mozilla's - e,g, 20010709.

This shows the same symptoms as bug#72379, but I believe the actual problem may
be related to bug#61628 - please see my recent comments and test case on that bug

Comment 4

[Fabian Guisset]

This is strange indeed! Kiko, could you help us on that one? What does query.cgi
use to reselect the products/components? 
My experience after trying about 10 different products is that sometimes when
going back the correct product/component is selected, sometimes the wrong
component is selected, and sometimes nothing is selected at all (that might be
more of a cache issue: the page is no longer scrolled to the bottom etc..)
Any ideas?

Comment 5

[Christian Reis]

Fabian, we trigger the reselection when the program SELECT control emits
onChange (something it does more or less reliably: see bug 97004)

As you can see per the test page I've placed at the URL, we are indeed calling
onChange() when the page is rendered, after clicking on back,and therein lies
our problem: The JavaScript works fine, and because BioArt is selected, Version,
Component and Target Milestone are refilled. I don't know exactly how to avoid
this: how to I know if it was a back button page load or a real page load?

This is not an HTML control bug, and I'm not sure it is a bug at all. To acheive
the effect desired (that the same components, versions and milestones be
selected when back is clicked) I'd have to save these into globals, and hope
they are preserved when the page re-renders. Testing.

Comment 6

[Christian Reis]

Duh. This doesn't work unless I catch component, version and milestone
onChange() and have them register the selection. I've changed the test page to
catch this; I now see that JavaScript variables do not survive a submit-back
cycle, but oddly, the SELECT control selection does.

My question: should the browser re-trigger the JavaScript on back? And should
JavaScript variables survive back? And should SELECT control selection survive
too (yes!)

I should note that Netscape4 does the exact same thing on back, as you may
notice by loading the test URL and doing a simple cycle of select product,
component, submit and back.

I've had an idea based on these facts: I'm going to save selections when
onChange() is emitted for the first time. My rationale: normally, selections
will be null; however, when back (or reload, I release, which has the same
problem - see the test URL) is clicked, it very well might be we have components
selected. I can therefore catch this and reselect after filling out the select
boxes. Testing.

Comment 7

[Christian Reis]

Whee, it works. However, to fix properly, because of MULTIPLE, I'm going to have
to do a full scan of all indexes selected in the three dialogs, which isn't a
lot of fun. It won't slow down the first page load, however, because I can use
selectedIndex to check if anybody was selected at all and avoid that. The fact
that selectedIndex only saves the first selected index is, in my opinion, simply
evil.

This is tested as working on NS4 and Mozilla 0831 on the page:

http://bugs.async.com.br/query-saving.cgi

Comment 8

[Christian Reis]

Attachment: v0: Patch to query.cgi: adds saving last selection state of component boxes, incomplete.

Comment 9

[Christian Reis]

Guess what? I've fixed it 100%, and even fixed an extra bug:

- When reloading with multiple selections in the product select control,
components/vers/tms were not being refilled properly.

I invite people to test my patch which is currently only locally applied, and
see if the usability of the form has gone up. The current patch survives reloads
and forward/back navigation with no problems. I'm quite happy to say the browser
is bug-free in this behaviour - good work, DOM/HTML Controls people! It's my
code that, as always, is horked. :)

http://bugs.async.com.br/query-83033.cgi

This patch integrates the patch for bug 38694 - it was necessary, or I had to do
evil things to get the selecting to be uniformly done. If you think this is an
impediment to working with the query form, the fix is now in hand.

Comment 10

[Christian Reis]

Attachment: v1 (proper fix, works for multiple selections, a bit unpolished)

Comment 11

[Fabian Guisset]

Kiko: Er wow, I CC you for some input, and two hours later you have produced two
patches ;-)
Anyway, it's cool if it works... But isn't this a bug in the browser though?
Does your patch fix it also for other browsers?

Comment 12

[Christian Reis]

Well, Fabian, you must know how I feel wrt query.cgi. Did you want it
fixed or not? :) To start off with, the bug originally reported has been
fixed in the browser AFAICT (mismatched selection on back/forward), and
from the look of the comments up there. 

Let me report on my findings with the unpatched query.cgi for various
browsers. Notice that "resets Javascript" means basically that the
javascript is reparsed and reexecuted; when this happens, the content
for the comp/vers/tms select boxes is reloaded, and selections lost. My
patch fixes the JS for browers which behave in this way, by saving the
selection before refilling the boxes, and reapplying it. IMHO it's a
good idea, and the proper fix. On to the tests:

a) Opera5 Win32: 
    + Reload: clears all selections.
    + Back/Forward: keeps all selections, does NOT reset JS.

b) IE5 Win32: 
    + Back/Forward: keeps Program Selection, resets JS.
    + Reload: clears all selections.

c) Mozilla Win32 0831:
    + Reload: keeps Program selection, _DOES NOT RESET JS_, options
    seem to be filled in from HTML CODE (WRONG).
    + Shift-Reload: clears all selections.
    + Back/Forward: keeps Program selection, _DOES NOT RESET JS_, options
    seem to be filled in from HTML CODE (WRONG).

d) Mozilla Linux 0831 (current): 
    + Reload: keeps Program selection, resets Javascript.
    + Shift-Reload: clears all selections.
    + Back/Forward: keeps Program selection, resets Javascript.

My patch alters the behaviour for b) and d) - current moz linux and IE5
win32, which are important browsers for us (understatement here?) and
that deserve the fix. I believe linux works the way it should (I find
the reload behaviour nice, if unique), as does IE5 - when back/forward
navigating, cache contents, but reparse the JS. Opera's solution is
okay, but I believe it's a result of a very basic JS implementation -
since not even multiple selection onChange() emission is supported.

There indeed seems to be an inconsistency in the way Win32 handles
reloading and back/forward from Linux, but this is not the same problem
as this bug reported. I propose the following (since I've taken over
this one :):
    - Check in the fixes to query.cgi to support proper selection memory
      for Linux Mozilla and IE5
    - File a very specific bug, with testcase, for Win32-current, on
      the subject "Reload and Back/Forward does not cause Javascript to
      reparse and execute".

Acceptable?

Comment 13

[Fabian Guisset]

If this is a bugzilla-only issue, go ahead, discuss this with your fellow
bugzilla hackers and get it reviewed, you don't need any more input from I don't
think.

Comment 14

[Christian Reis]

Fabian, there remains the problem with inconsistency between Linux and
win32 events on back/forward and reload (as my comment before states).
What should I do, file a new bug?

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 47966 [details] [diff] [review]
v1 (proper fix, works for multiple selections, a bit unpolished)

This:
>+function getSelection(control, findall) {
>+    ret = Array();
>+    if ( ( ! findall ) && ( control.selectedIndex == -1 ) ) {
>+        return ret;
>+    }
>+    for ( i=0 ; i<control.length ; i++ ) {

should be:
>+function getSelection(control, findall) {
>+    var ret = Array();
>+    if ( ( ! findall ) && ( control.selectedIndex == -1 ) ) {
>+        return ret;
>+    }
>+    for ( var i=0 ; i<control.length ; i++ ) {

Other than that, r=caillon

Comment 16

[Jacob Steenhagen]

Comment on attachment 47966 [details] [diff] [review]
v1 (proper fix, works for multiple selections, a bit unpolished)

This doesn't apply cleanly to the current CVS...
Kiko, can you please update this patch and include callion's fixes?

Thanks.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Clearing patch and review keywords until new patch is attached.

Comment 18

[Christian Reis]

Attachment: Latest fix. Corrects JS string warnings, nice cleanups.

Comment 19

[Christian Reis]

Okay, I'm bothered enough to ask for an r=/sr= now please.

The latest patch can be tested at http://bugs.async.com.br/query-83033.cgi vs
http://bugs.async.com.br/query-orig.cgi and it _works_ for Linux and IE

Comment 20

[Christian Reis]

Attachment: v3: fix dumb perl warnings. want r=

Comment 21

[Christian Reis]

Attachment: v4: incorporates afranke's fixes to getSelection, an extra, useless loop in selectProduct, and typos.

Comment 22

[Christian Reis]

There is a change in the file that fixes another problem, which was when the
form was first loaded, last_sel would be empty and we would call fake_diff_array
on it. I've also fixed that in this line:

         // this is an optimization: if we've added components, no need
         // to remerge them; just merge the new ones with the existing
         // options.

-        if ( ( tmp ) && ( tmp.length < sel.length ) ) {
+        if ( ( tmp.length > 0 ) && ( tmp.length < sel.length ) ) {
             sel = fake_diff_array(sel, tmp);
             is_diff = 1;
         }
     }

This patch looks polished right now.

Comment 23

[Andreas Franke (gone)]

Comment on attachment 52340 [details] [diff] [review]
v4: incorporates afranke's fixes to getSelection, an extra, useless loop in selectProduct, and typos.

r=afranke

Comment 24

[Dave Miller [:justdave]]

Comment on attachment 52340 [details] [diff] [review]
v4: incorporates afranke's fixes to getSelection, an extra, useless loop in selectProduct, and typos.

>@@ -344,12 +352,12 @@
>-        if ( $data{$p} ) {
>-            $ret .= $arr."[".SqlQuote($p)."] = [";
>+        if ( $data{$prods[$p]} ) {
>+            $ret .= $arr."[$p] = [";

Was there a reason the SqlQuote() was being removed here?  I'm not the greatest
with Javascript, just want to make sure it was on purpose.

>@@ -541,7 +584,7 @@
>     // this is to avoid handling events that occur before the form
>-    // itself is ready, which happens in buggy browsers.
>+    // itself has been processed ready.

Why was this comment changed?  The new comment doesn't make any sense.

That's all my comments on this so far.  It obviously works, in testing it.  That
comment is my only concern with it.

Comment 25

[Christian Reis]

> >@@ -344,12 +352,12 @@
> >-        if ( $data{$p} ) {
> >-            $ret .= $arr."[".SqlQuote($p)."] = [";
> >+        if ( $data{$prods[$p]} ) {
> >+            $ret .= $arr."[$p] = [";
>
> Was there a reason the SqlQuote() was being removed here?  I'm not the greatest
> with Javascript, just want to make sure it was on purpose.

Well, $p is now a simple counter. If you look at the initialization, it has changed:

-    foreach my $p ( @prods ) {
+    for ( my $p = 0; $p <= $#prods; $p++ ) {

Turning the indexes into integers is actually one of the important changes this
bug introduces that intends to fix a whole lot of problems (and make page size
go down further).

About the comment: okay, you're right. How about:

--- query.cgi   Tue Oct 16 12:17:40 2001
+++ query-xxx.cgi       Tue Oct 16 12:17:47 2001
@@ -583,8 +583,8 @@
 
 function selectProduct( f ) {
 
-    // this is to avoid handling events that occur before the form
-    // itself has been processed ready.
+    // this is to avoid handling events that may occur before the form
+    // has been created - see the onLoad handler for BODY f.i.
 
     if ( ( !f ) || ( ! f.product ) ) {
         return;

I put this in because of a problem I had here. The onLoad handler was being
called but the wrong form was being passed in (because we used a numerical index
to document.forms) so there was no f.product. You might argue that this test is
now unnecessary because I've changed forms to use IDs, which was rolled in as
part of the fix for bug 58436.

Comment 26

[Jacob Steenhagen]

Looks like v4 isn't quite final... removing patch and review keywords until it is.

Comment 27

[Christian Reis]

Jake, all that changes is two lines of comments, for heaven's sake :-) Just
let's get a second review on it - nobody even reads the patch!

Comment 28

[Gervase Markham [:gerv]]

When the query.cgi templatisation lands, this will become a patch to query.atml
and you'll also have to change all the comments to [%# %] ones :-( Sorry about
that. So, it's probably worth waiting until that lands before you do any more.

Gerv

Comment 29

[Dave Miller [:justdave]]

Comment on attachment 52340 [details] [diff] [review]
v4: incorporates afranke's fixes to getSelection, an extra, useless loop in selectProduct, and typos.

Bug 98707 has landed.  As such, this patch is completely bitrotted.  But you're
welcome to begin working on it again.  In fact, please do, because this is a
release blocker. :)

Comment 30

[Gervase Markham [:gerv]]

Kiko: do you plan to work on this again?

Gerv

Comment 31

[Christian Reis]

Gerv: why, yes! I just had 10 days of fever that had the boojum beat out of me,
but I seem to be operational again. Sorry to take so long. I'll fix these fast now.

For me to fix this in a nice fashion, I filed a new bug (with patch) for some
minor but important improvements to the javascript used in query.atml. Please
expedite review on bug 122154.

Comment 32

[Dave Miller [:justdave]]

ok, bug 122154 is fixed now.  What's the status on this one?

Comment 33

[Christian Reis]

The status is the following: this works on ns4, and it _almost_ works on
mozilla, but for a form controls bug I've discussed with jkeiser on irc:

<kiko> jkeiser: so the issue is this: when we use the back button, the selected
items in the select control are restored
<kiko> jkeiser: but they are restored based on the _position_ of the originally
selected items, _not the contents_
<jkeiser> kiko: right
<jkeiser> that's true
<jkeiser> kiko: that is the problem, yeah ... if you want to fix code, you can
store / restore values instead of integers in the list (that might get a bit
hairy but it's doable)

So this is a Mozilla bug now. Let me file it; bug 125421

If anybody catches this breaking on other browsers, reopen and bug me.

Comment 34

[Dave Miller [:justdave]]

Verified worksforme.

This works fine in Netscape 4.7 Mac OS 9.2, and Internet Explorer 5.1 OS X. 
It's a Mozilla bug.

Comment 35

[Antonis Christofides]

On my NT4 SP6, IE 5.5 SP2 has the same behavior as Mozilla (although Netscape
4.78 and Opera 6.01 work correctly). Bug 95622 comment 34 says IE 6 also behaves
the same (bug 125421 is a dup of 95622).