577574 - Fields using YUI autocomplete lose their additionally-entered data when pressing the Back button

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Frédéric Buclin]

test_edit_products_properties.t detected that if you create a new component and for some reason an error is thrown (e.g. because you forgot to enter a description), all fields for default roles are cleared when clicking the Back button of your browser. In Bugzilla 3.6 and older, there weren't. Bugzilla 4.0 should retain this information.

This is a regression due to bug 490923.

Comment 1

[Guy Pyrzak]

created bug http://yuilibrary.com/projects/yui2/ticket/2529038 in the YUI trac system

Comment 2

[Guy Pyrzak]

This is a bug in YUI but i have a fix, which will need to be applied to both the keyword AND the autocomplete field.

Comment 3

[Guy Pyrzak]

Attachment: V1

basically just turns on and off the autocomplete based on focus and blur

Comment 4

[Reed Loden [:reed]]

You have other stuff in your patch... probably want to remove that and submit another patch.

Comment 5

[Guy Pyrzak]

Attachment: v1.1

the patch had the validation stuff in it. sorry

Comment 6

[Max Kanat-Alexander]

Comment on attachment 456593 [details] [diff] [review]
v1.1

Cool idea. :-) Does onblur fire when the page is unloaded?

Comment 7

[Guy Pyrzak]

it should I've tested it firefox numerous times and IE. Blur should happen when unload occurs as well. We should test this incase but in my experience, it works fine

Comment 8

[Guy Pyrzak]

Ok, tested it. Looks like the blur doesn't happen if you hit enter on the field (don't click a button). If we add the unload event then it will break bf_cache on the edit bug page. We could add the unload only if they happen to focus on the field?

Comment 9

[Guy Pyrzak]

Attachment: V2

Comment 10

[Guy Pyrzak]

Attachment: V2.1

forgot to do the same thing for keywords.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 456613 [details] [diff] [review]
V2.1

>=== modified file 'js/field.js'
>+        try{

  Why is this in a try?

>+        var f = document.getElementById(field);
> [snip]

  This is enough code in both autocompletes that I'd like it abstracted out into a separate function that can be used in both.

>+        YAHOO.util.Event.addListener(field, "focus", function(ev){
>+          if( f ){
>+              f.setAttribute("autocomplete","off");
>+              YAHOO.util.Event.addListener(f, "focus", function(ev){ 
>+                  f.setAttribute("autocomplete","on");
>+              });

  You could add a comment above this to explain what it's doing and why it's necessary?

Comment 12

[Guy Pyrzak]

yeah i thought this too, especially since i kept pasting the same thing in 2 places.

Comment 14

[Frédéric Buclin]

(In reply to comment #1)
> created bug http://yuilibrary.com/projects/yui2/ticket/2529038 in the YUI trac
> system

Someone should reopen this bug. The YUI developer says it's related to using https://, but the same behavior is visible when using http://.

Comment 15

[Guy Pyrzak]

bug was reopened

Comment 16

[Guy Pyrzak]

Please note. Even if YUI fixes the bug in safari/chrome we will have to implement this patch to handle gecko based browsers since we purposefully remove the unload listener because we wanted to support the BF cache which breaks with the unload listener.

If i have to implement this fix for gecko based browsers anyway I don't see it as being as big of a deal if they do or don't fix this issue from within YUI.

Comment 17

[Guy Pyrzak]

So if i remove the lines that remove the unload window listener it works fine in firefox, so this bug is basically of our own making... except for safari, where the bug happens with or without our lines of code. I'm working the safari bug with the folks at YUI. I'm using landfill.bugizlla.org/adv_search_ui/ to test the fixes.

Comment 18

[Max Kanat-Alexander]

  Ah, okay. In that case, I think removing that code block is the way to go. I'd rather have a working back button than no b/f cache.

Comment 19

[Guy Pyrzak]

well the patch that I added keeps b/f cache most of the time AND fixes the bug, but it's up to you.

Comment 20

[Max Kanat-Alexander]

(In reply to comment #19)
> well the patch that I added keeps b/f cache most of the time AND fixes the bug,
> but it's up to you.

  Hmm. Any particular reason that the YUI folks don't want to solve the problem your way instead? If the developer that you're dealing with is causing trouble, I can get in touch with the YUI project manager about it.

Comment 21

[Boris Zbarsky [:bzbarsky]]

So why is an _unload_ listener needed to make the page not clobber the form value on navigation back, exactly?  At what point is the value overridden?

Comment 22

[Max Kanat-Alexander]

(In reply to comment #21)
> So why is an _unload_ listener needed to make the page not clobber the form
> value on navigation back, exactly?  At what point is the value overridden?

  Honestly, I have no idea why they need an unload listener, but I'm going to see if I can get in touch with any of the YUI folks to get the underlying bug resolved, and possibly put them in touch with you if they really need help on some tricky bit of Gecko event handling.

  For reference, the bug about the unload listener existing is:

  http://yuilibrary.com/projects/yui2/ticket/2091763

Comment 23

[Guy Pyrzak]

I think the _unload_ listener was used as a straight forward way to make sure autocomplete=off gets switched back to "on" even without a form on the page or if the user doesn't click back. It is necessary to set it back to "on" because that's what fills it back in when you click back, but it needs to be off when the user is focused to turn off the "autocomplete" that is built into most browsers.

Adding a listener to "blur" or "submit" would help a lot but wouldn't handle all the situations where "unload" fires but blur doesn't (such as people hitting enter on the text field). 

I kinda wish blur would always fire before unload but it doesn't (at least not on firefox).

Comment 24

[Max Kanat-Alexander]

pyrzak: Awesome, thank you for that explanation. :-)

bz: Is there any better solution than using onblur and onsubmit? We need to be sure that "autocomplete='on'" gets turned back on after the YUI autocomplete is no longer needed.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  The silly overloading of autocomplete="off" is killing us here, huh?

Does using a pagehide listener work?  And then onfocus to set autocomplete off?

Comment 26

[Max Kanat-Alexander]

(In reply to comment #25)
> Ah, I see.  The silly overloading of autocomplete="off" is killing us here,
> huh?

  Yeah.

> Does using a pagehide listener work?  And then onfocus to set autocomplete off?

  I suspect that would work, yeah--pyrzak, can we move the unload listener into being a pagehide listener? I bet upstream should be doing that too.

Comment 27

[Max Kanat-Alexander]

bz: We want to switch the unload listener to a pagehide listener, but we ran into a problem with feature detection. if ("onpagehide" in window) is true on WebKit, but false on Gecko. We'd like to create a generic test for support of the pagehide event--is there some better way to do it?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Event attributes are generally lazily resolved in Gecko, so that will test false for them...

In Gecko's case, there's no way to object-detect those; you have to UA-sniff.  :(

Comment 29

[Guy Pyrzak]

Ok, then I guess we should just UA-sniff for this patch, bummer.

Comment 30

[Max Kanat-Alexander]

Well, we should object-detect and then UA-sniff.

Comment 31

[Guy Pyrzak]

Attachment: v3

Comment 32

[Max Kanat-Alexander]

Comment on attachment 465840 [details] [diff] [review]
v3

This patch also modifies .htaccess, for some reason.

Comment 33

[Guy Pyrzak]

Attachment: v3.1

sorry i have to mod the .htaccess for my instance

Comment 34

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified template/en/default/global/header.html.tmpl
Committed revision 7452.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/                         
modified template/en/default/global/header.html.tmpl
Committed revision 7388.