437212 - Refreshing a page does not update fields

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Filipe Brandenburger]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008060309 Firefox/3.0
Build Identifier: version 3.0.4

I had a bug displayed in Firefox and got a notification that the severity had
changed. I did a refresh but the field did not change. Opening the page in a
new Window or tab did show the updated severity.

I checked that if you reload with Ctrl-F5 instead of just F5 it works.


Reproducible: Always

Steps to Reproduce:
1. Open a bug on one computer/one browser/one tab.
2. Open the same bug on another computer/another browser/another tab.
3. Change the severity on the second computer/browser/tab.
4. Commit the change.
5. Go to the first computer/browser/tab which has that same bug open and press "F5" or click the "Refresh" button.
Actual Results:  
The severity shown will be the old one.

Expected Results:  
The severity shown should be the new one.

A workaround is:
6. Go to the first computer/browser/tab which has that same bug open and press "Ctrl-F5" button.

This was tested with Firefox 1.5 (CentOS4) and Firefox 3.0 (Ubuntu) on an installation of bugzilla 3.0.4.

Comment 1

[Frédéric Buclin]

This is not a bug in Bugzilla but the way Firefox works. F5 doesn't override values currently displayed in single-select fields. If you don't like the way Firefox works, file a bug in Firefox.

Comment 2

[Guy Pyrzak]

I have experienced this "bug" as well, the main problem is that the Bugzilla tells you to click on a link, however, most people click "back" and then refresh (not meta-refresh), which is what you did. The problem isn't JUST firefox but the "autocomplete" feature of many browsers. They assume that the value that the user has "entered" is correct over what the server sent. Perhaps an option is to use the same method we use for the "title".

So there is a user experience issue here, and there are ways it could be resolved (see the title in 3.2 as an example). But even with the title example, it just lets you know that you've got a value different than the server. There isn't any clear "heads-up" that the field changed from. But I'm also not sure we want to do about that. There is an annoying way to store which fields a user has "touched" between page loads to mitigate the autocomplete bug. But I'm not sure if this is a direction Bugzilla as a whole wants to do. It's a LOT more complexity for something that happens rarely.

But thanks for filing the bug. 

Comment 3

[cps]

Surely there's something that could be done to prevent this kind of thing though:-

1) User A opens bug report.
2) User B sets milestone/priority etc.
3) User A refreshes.
4) User A enters a comment and commits.

Whoops, user A accidentally removed the milestone and priority.  The didn't do anything 'bad' like not click on the link and never at any point modifed the milestone/priority in the form.  I'm forever doing this and  yes I'm also using firefox.

Surely there's a way to distinguish between something the user set on the form vs something that was defaulted and hence should be subject to 'refresh'?

Comment 5

[Kent Rogers [:mrbball]]

I opened bug 688163 and I somewhat disagree that it is a duplicate of this bug.

I was not able to find this bug, due to the generic nature of the summary, and
although it describes similar symptoms, bug 688163 is against a specific Bugzilla
functionality that does not work (mid-air collision detection).

Whether it is addressed here or in 688163, I think this is a Bugzilla bug.
Regardless of how Firefox works (or how some think it should work), the problem
I reported in 688163 is that Bugzilla does not detect mid-air collisions in some
situations, possibly leading to very bad results.

I don't understand what all happens in an HTML form on a browser refresh, but in
Firefox, some some fields apparently update while others don't.  If that's "the
way Firefox works", Bugzilla should figure out a way to deal with it so that
Bugzilla works the way it should work and detect mid-air collisions.

I did a little testing and found that regardless of whether delta_ts is a hidden
field or a single select field, it is always updated in the form on a refresh, so
there's obviously more to it than that.  Can anyone explain why delta_ts gets
updated and other bug fields don't?  I'm guessing it has to do with values that
are selected (or not) in the single select fields, but really have no idea.

If there were a way to make delta_ts NOT update on a refresh, I think that would
resolve the mid-air detection problem, but I couldn't figure out how to do that.

Comment 6

[Guy Pyrzak]

A possible fix for the single select is to have another div that contains the same value as the field and on refresh compare the value in the div to the value of the field and let the user know that they don't match, but this doesn't guarantee that the user will do anything about it, but it lets them know that something has changed.

The problem from the Firefox point of view is that the browser thinks that You changed the drop down field (which you didn't), and it keeps your value instead of updating it to what the browser has specified. So the browser keeps the old value instead of updating it to the new value from the server. 

The reason why delta_ts isn't impacted is because 1, it's not a drop down, 2, it's hidden.

So yes, the 2 bugs are the same and right now we're of the opinion that the complexity needed to fix this for one particular browser is too high. But we do know of ways to fix it. Hope that helps clarify things.

Comment 7

[Kent Rogers [:mrbball]]

> The problem from the Firefox point of view is that the browser thinks that
> You changed the drop down field (which you didn't), and it keeps your value
> instead of updating it to what the browser has specified. So the browser
> keeps the old value instead of updating it to the new value from the server. 
> 
> The reason why delta_ts isn't impacted is because 1, it's not a drop down,
> 2, it's hidden.
> 
It's more than that.  In my testing, I changed delta_ts from a hidden to a drop
down and it had no effect.  I'm pretty sure it has to do with the fact that both
the old and new values are options for the drop-down and one of them (the old
value) was marked as selected.

Unfortunately, I couldn't figure out a way to simulate that for delta_ts to see
if that caused it to "not update" and thus trigger a mid-air.  There would have
to be a way to save the old value of delta_ts and have that be one of the options
in the drop-down.  If that could be done, I believe it would "stay selected"
because as you say, Firefox would think I selected it.

I'm guessing there is a way to do this, but I'm not enough of a html/javascript
guru to know what it is.

Comment 8

[Kent Rogers [:mrbball]]

Any chance of getting this bug or bug 688163 re-opened?

I think Firefox is working as designed, and this is a pretty severe problem that
can blow mid-air collisions out of the water under certain conditions, causing data
loss.  If nothing else, it should stay open to make it clear that this is still and
problem and in case somebody comes up with a workaround/trick to get delta_ts to
not update on a refresh.

Thanks.

Comment 10

[hwine]

A lot has changed since the original decision not to address this in 2008. Is this still the correct UX decision? 

Given continued issues it causes, I think the decision should be revisited. (Obviously, I think it should be changed and the bug addressed - but that's for the team to decide.)

Comment 12

[Frédéric Buclin]

(In reply to Hal Wine [:hwine] from comment #10)
> A lot has changed since the original decision not to address this in 2008.

What has changed?

Comment 13

[hwine]

I believe several things have changed enough since 2008 to warrant a revisit:
 * the tools available to address this issue (html5, javascript libraries, etc)
 * user expectations of web applications have increased 
 * client side capabilities (cpu, bandwidth, browser versions) have improved

IMO, the two choices are: (a) have open as bug, and (b) close "won't fix". "invalid" doesn't seem right if the problem continues to affect 1/4 or more of the user base. (I would suspect more given who some of Bugzilla's institutional users are.)

Comment 14

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

I've hit this problem several times in recent memory, but this is the first time since finding this bug.

1) I open a window to look at a bug in bugzilla.
2) I wait. Others make changes to the bug.
3) I refresh the window and see all the bugzilla comment updates. 
4) I do textedit to add a comment to the bug and hit save. 
5) I discover that the refresh I did in (3) did update to display new comments, but did *not* update pulldown menus, so the "Status" was unintentionally overwritten without warning. This meant that I reopened a bug by accident.
6) re-close the bug quickly before IT are paged for open blocker bugs.


If it helps, see https://bugzilla.mozilla.org/show_bug.cgi?id=742129

Comment 15

[Frédéric Buclin]

joduinn, the symptom you describe is well known and that's what this bug is about. As said in comment 1 and in many other places, that's how your browser works. If would make much more sense to blame Firefox to not reload all fields when refreshing your page.

Comment 16

[hwine]

Attachment: Demo of autocomplete attribute

Frédéric, I did some googling and found what appears to be a simple fix for bugzilla to implement. If an application (the one cited is banking login) does not want the browser to prefill information in a field, the application can specify that by setting the "autocomplete" attribute to "off" for that field. As you would expect, firefox honors that setting, and the behavior is as desired (based on the numerous comments on this bug).

I've attached a very simple HTML page which can be used to demonstrate the effect.

Comment 17

[Kent Rogers [:mrbball]]

Setting autocomplete="off" appears to be a very functional fix for this problem, at
least for single select fields.  It might not be the most desirable thing for some
of the text boxes (where it's nice to see what you've entered in the past), but
setting autocomplete="off" for all fields (except Additional Comments) is the only
way to be sure of completely eradicating this problem and allowing mid-air collisions
to work correctly again.

Is there any reason to NOT implement this solution?  Are there any fields other than
Additional Comments where it doesn't make sense to turn off autocomplete?  It does
not seem to have any adverse affect on the fields that are already being
auto-completed via Javascript (e.g. keywords).

Comment 18

[Frédéric Buclin]

(In reply to Kent Rogers [mrbball] from comment #17)
> Is there any reason to NOT implement this solution?  Are there any fields
> other than
> Additional Comments where it doesn't make sense to turn off autocomplete? 

For select fields, this solution seems fine. For text fields, I wouldn't want my comment to be deleted when refreshing the page, so I agree we shouldn't put autocomplete=off for the additional comment field. The user may have edited the other text fields, such as the bug summary, the keyword field, or dependencies, but I guess this is a good compromise.

Note that fields must not refresh themselves when clicking the Back button of the browser. They must refresh themselves only when reloading the page. I would accept a patch which does that.

Comment 19

[Kent Rogers [:mrbball]]

(In reply to Frédéric Buclin from comment #18)
> Note that fields must not refresh themselves when clicking the Back button
> of the browser. They must refresh themselves only when reloading the page.
>
Unfortunately, with autocomplete="off", fields refresh when clicking the Back
button of the browser. :-(  I agree that this is unacceptable.

Comment 20

[hwine]

I think we could get a good debate going on whether or not behavior when using back button is more significant than the existing problem. 

Sounds like we're now we're down to a difference of opinion on how the UX should work. Does that mean it's time for a site/user configurable preference?

Comment 21

[:glob ✱]

(In reply to Hal Wine [:hwine] from comment #20)
> I think we could get a good debate going on whether or not behavior when
> using back button is more significant than the existing problem. 

unfortunately bugzilla's error management relies on working back functionality.

if there's an error during processing an update, bugzilla's displays the error on a standalone page, and invites the user to "Press back and try again".  setting autocomplete=off would revert and changes made to those fields when you navigate back, so this is undesirable.

Comment 22

[:glob ✱]

i've made a rough proof of concept for a fix for this:

http://drop.glob.com.au/437212/

this tracks if a user has changed the value of the select.  if they haven't changed the select's value, the select's value is updated to match the server's pre-selected value.  once the select's value is changed, this value is retained when the page is refreshed.

when the poc is loaded, "one" or "two" is randomly chosen as the default value for the select.  refresh the page a few times and the first select's value should change to reflect the server's pre-selected value, while the second one (without the fix) retains the same value.  change the first select's value to "three" and refresh the page; "three" should stay selected.

Comment 23

[:glob ✱]

Attachment: patch v1

this patch forces the browser to use the option the server has tagged as "selected" when a page is refreshed, but only if the user hasn't changed the select.

Comment 24

[:glob ✱]

Comment on attachment 637784 [details] [diff] [review]
patch v1

this doesn't work with IE8, will provide an updated patch.

Comment 25

[:glob ✱]

Attachment: patch v2

Comment 27

[Frédéric Buclin]

Comment on attachment 653686 [details] [diff] [review]
patch v2

Sorry for taking so long to review your patch.

I just tested with Firefox 15.0.1, and your patch seems to have no effect at all. Fields which have been edited by another user are not updated, despite the source code shows that the selected attribute is now on the new items.

Comment 28

[:glob ✱]

(In reply to Frédéric Buclin from comment #27)
> I just tested with Firefox 15.0.1, and your patch seems to have no effect at
> all. Fields which have been edited by another user are not updated, despite
> the source code shows that the selected attribute is now on the new items.

it works for me on firefox 15.
please describe the steps you took to test.

here's what i'm doing:
1. open a bug in firefox 15 as user A (don't touch any fields)
2. open the same bug in another browser (eg. chrome), as user B
3. in chrome, change the priority field, and save changes
4. in firefox, refresh the bug

without the patch, the priority will remain unchanged in firefox after the refresh.
with the patch, the priority will be updated to show the new value as set by user B.

if you touch the priority field as user A, even without saving the change, refreshing won't pick up the new value set by user B.  this is by design, as it closely matches firefox's current behavour.

Comment 29

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #28)
> here's what i'm doing:
> 1. open a bug in firefox 15 as user A (don't touch any fields)
> 2. open the same bug in another browser (eg. chrome), as user B
> 3. in chrome, change the priority field, and save changes
> 4. in firefox, refresh the bug

That's *exactly* what I did too. But fields are not refreshed in Fx 15. They are correctly refreshed in Opera, though.

Comment 30

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #29)
> That's *exactly* what I did too. But fields are not refreshed in Fx 15. They
> are correctly refreshed in Opera, though.

Actually, Opera refreshes fields correctly without your patch too.

Comment 31

[Frédéric Buclin]

Comment on attachment 653686 [details] [diff] [review]
patch v2

>=== added file 'js/show_bug.js'

>+                var preSelectedIndex = getPreSelectedIndex(el);
>+                if (preSelectedIndex != -1)
>+                    el.selectedIndex = preselectedIndex;

I found why your patch didn't work (after wasting one hour on it)! On the last line above, you wrote preselectedIndex instead of preSelectedIndex (note the "s" vs "S" in "Selected"). If I fix this typo, then your patch works fine in Firefox for all single-select fields. Unfortunately, it regresses custom multi-select fields. If you select several values in a multi-select field and submit your changes, all values are automatically unselected except the first selected value when the page is displayed again. Your patch doesn't handle multi-select fields at all.

Comment 32

[Frédéric Buclin]

One more thing: when updating your patch, please merge show_bug.js with field.js. There is no reason to have a separate JS file for this, and I see other places where this code could be used, e.g. for attachment attributes.

Comment 33

[:glob ✱]

Attachment: patch v3

fixes the typo (sorry!), moves the code into fields.js and util.js, fixes multiselect fields.

Comment 34

[Frédéric Buclin]

Comment on attachment 662043 [details] [diff] [review]
patch v3

>=== modified file 'js/field.js'

>+function initDirtyFieldTracking() {

>+        if (el_dirty) {

Instead of this huge if() block, it would be much cleaner to call "continue" if el_dirty doesn't exist (this would also decrease the indentation).


>+                if (!el.multiple) {
>+                    isDirty = preSelected.index == currentSelected.index;

The logic is reversed. isDirty must be true only if the indexes are different.


>+                        for (var i = 0, l = preSelected.length; i < l; i++) {
>+                            var found = false;
>+                            for (var j = 0, m = currentSelected.length; j < m; j++) {
>+                                if (currentSelected[j].index == preSelected[i].index) {

That's overly complex. As both arrays are ordered by their index, you simply have to loop only once:

    for (var i = 0, l = preSelected.length; i < l; i++) {
        if (currentSelected[i].index != preSelected[i].index) {
            isDirty = true;
            break;
        }
    }

That's all you need.


Otherwise looks good.

Comment 35

[:glob ✱]

Attachment: patch v4

Comment 36

[Frédéric Buclin]

Comment on attachment 662852 [details] [diff] [review]
patch v4

>=== modified file 'js/field.js'

>+function initDirtyFieldTracking() {

>+                for (var j = 0, m = preSelected.length; j < m; j++) {
>+                    preSelected[j].selected = true;
>+                }

I just realized that your patch still doesn't work correctly for multi-select fields: this code only selects newly-selected options, but doesn't unselect the other ones. So if a multi-select field had options 1 and 3 selected, and then someone else changed it to options 1 and 2, then if you reload the page, you see that option 2 is now selected (as desired), but option 3 hasn't been unselected (oops). On submit, this means that option 3 would be re-selected again.

AFAICT, this is the last thing to fix.

Comment 37

[:glob ✱]

Attachment: patch v5

nice catch -- this revision clears the selected options.

it pans out that IE <= 8 doesn't expose the 'selected' attribute on multiselect options, making this fix impossible.  however as IE <= 8 isn't affected by the refreshing issue, i simply don't enable it for those browsers.

Comment 38

[Frédéric Buclin]

Comment on attachment 663293 [details] [diff] [review]
patch v5

Your patch seems to be useful on Firefox only. With Opera, IE9 and Chrome, the full page is refreshed, discarding all your pending changes. I didn't check if that was the case without your patch. So r=LpSolit as it fixes the problem on Firefox.

Comment 39

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified js/field.js
modified js/util.js
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/field.html.tmpl
modified template/en/default/bug/show-header.html.tmpl
modified template/en/default/flag/list.html.tmpl
Committed revision 8401.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified js/field.js
modified js/util.js
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/field.html.tmpl
modified template/en/default/bug/show-header.html.tmpl
modified template/en/default/flag/list.html.tmpl
Committed revision 8397.