108198 - prefs.js gets bogus changes made; Textfields in pref panels not displayed;

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect, P2)

Description

[Tracy Walker [:tracy]]

seen on commercial builds:

linux 2001-11-02-06-trunk
mac 2001-11-02-04-trunk

-Open the Preferences dialogue from Edit | Preferences
-Notice the Homepage is blank.  Set it to your homepage.
-close the dialogue and renter...the page is gone again.

clicking on Home from the browser goes to the page that was entered.

note:  upon installation of this build, for existing profiles, the previously 
set homepage was reset to the default (home.netscape.com for commercial builds) 

also note: subsequent relaunches of the build continue to remember what was 
selescted as the homepage (even though it doesn't appear in the Preferences 
dialogue).

Comment 1

[sairuh (rarely reading bugmail)]

kw-o-rama

Comment 2

[sairuh (rarely reading bugmail)]

samir/ben/hyatt, any ideas what caused this regression?

Comment 3

[Tracy Walker [:tracy]]

more clarification on reproducing this.  It DOES remember the homepage at first, 
but as soon as you go into the edit preferences dialogue and click okay.  the 
homepage pref gets set to default.

Comment 4

[Tracy Walker [:tracy]]

this is affecting mfcembed on windows in such that whatever profile is used to 
launch with the default homepage is used.  switching to another profile and the 
homepage is correct.  switching back to the original and the desired homepage is 
back.   seems to only affect the startup profile and only during that session.  
Perhaps this is another bug for mfcembed only? 

Comment 5

[Samir Gehani]

Tracy,
Are there any errors or warnings reported in the JavaScript console?  If yes,
could you attach them here?  Thanks.

Comment 6

[John Morrison]

There are no JS errors thrown by this. 

I think that this must be related to the forms checkin from last night (since 
these xul textfield widgets are html form controls under the hood). 

I have updated layout to pick up jst's latest change to 
nsGfxTextControlFrame2.(cpp|h), and I still get a blank textfield for the 
home page. I'm going to fully update to the tip right now to see if this 
is otherwise fixed.

However, this is much more serious than the loss of your homepage setting. 
Any pref panel that is viewed (not altered) and contains a textfield will 
be set to "" or '0' if you click OK to close the pref panel. You do not 
need to alter any fields, just _view_ the panel, then click OK, to have 
these changes happen.

Among other serious changes you can unintentially make to your profile are,
set the memory and history cache size to zero, set the HTTP protocol to '1.0',
cause history to stop recording visited sites, never persist cookies (I think),
and change the accept-encoding of the http headers.

This is bad. This is really a blocker.

(p.s., don't open the 'Navigator->Languages' panel. That is a crash.)

Comment 7

[John Morrison]

cc: darin, gordon, pavlov, since I noticed this when I inadvertently set my 
cache size to zero (and some of the other possible changes can really affect
networking characteristics).

Comment 8

[John Morrison]

I just finished updating, and this is still not showing content for textfields
in XUL dialogs, and overwriting prefs.js with blank or zero values, disabling
things like disk and memory cache, etc., etc. 

-> jst

Comment 9

[John Keiser (jkeiser)]

The problem here is a change we made when fixing html:input.  Specifically
setAttribute("value") no longer sets .value -- it only sets the value attribute,
as per spec and IE behavior.  The value attribute is *not* equivalent to .value,
as evidenced by this snippet:

inputElem.setAttribute("value", "x");
inputElem.value = "y";
alert(inputElem.getAttribute(inputElem.getAttribute("value"));

This gives you "y".

People should not rely on this dubious side effect of setAttribute("value")
anymore.  In the interests of getting the builds usable again I'll attach a
patch to textbox to set .value when setAttribute("value") is called.  This fixes
the problem, but we should not resolve this bug until all clients of
setAttribute() are fixed.

Comment 10

[John Keiser (jkeiser)]

Attachment: New textbox.xml (for blake)

Comment 11

[John Keiser (jkeiser)]

Attachment: Full textbox.xml (bad transfer)

Comment 12

[Blake Ross]

Attachment: patch

Comment 13

[Blake Ross]

Comment on attachment 56463 [details] [diff] [review]
patch

sr=blake

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 56463 [details] [diff] [review]
patch

gah. ok. r=[e-mail]

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 56463 [details] [diff] [review]
patch

er. hold on. back up. retracting r=.

Comment 16

[John Morrison]

Attachment: what do you mean IE doesn't reflect attribute 'value' into property 'value' (and vice versa)

Comment 17

[[not reading bugmail]]

this is a problem with lots of other prefs. not being saved and used in recent
builds.. especially the mail/news accounts in 11-02.  

Example: What I am talking about is the appearance->startup in ns6.2 is not
using the correct setting if I wanna check mail to startup in from a profile
made with 6.1.  So besides the issue with 11-02.. 

The thing I see most often: is that many prefs in general are not being used by
just copying the prefs.js file into a new profile directory.  Question: does
this require more than the prefs.js file being copied to work?  I have to hit
'OK' button, even filters I had to go into ns6.2 and hit enter before they
worked again on my Imap account.

Will this patch fix any of these issues I'm refering to?

-dman84

Comment 18

[Blake Ross]

This patch had to be backed out for now. It looks as if not all of the 
necessary parts of xbl have been implemented already to support this (although 
the patch was semantically wrong anyways according to current proposal).

Comment 19

[John Morrison]

I am not an expert in this (an understatement), but I don't read the following
as eliminating attribute-property reflection for HTML DOM 2.

http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-642250288
  1.6.1. Property Attributes
    HTML attributes are exposed as properties on the element object. ...
    The attributes are exposed as properties for compatibility with DOM
    Level 0. This usage is deprecated ...

So, that's deprecated, not unsupported. Per spec. Given also that the 
attached testcase shows that IE5.5 supports this reflection, I don't see
why we would remove this for HTML DOM (which are the objects that provide 
the base features for the XUL widgets).

Comment 20

[David Hyatt]

Also adding my 2 cents.  Even if you did accept that the value attribute only 
applies to the initial state of the control (a claim I think you could dispute, 
but let's accept it for the sake of argument), there is still a problem here.

Even when the form is still in the "reset" or "initial" state, we don't honor 
changes to the value attribute.  Until .value has actually changed, through 
user input, or through script, it should be tied to the value attribute.  In 
other words, when the input element is still in its initial state, changes to 
the value attribute should be reflected.

Right now, we essentially do this, except we treat .value as having changed the 
moment the frame is constructed.  It can be seen how bogus this is by examining 
these two pieces of DOM code.

var v = document.createElement('input');
v.defaultValue = 'goo';
v.defaultValue = 'moo';
eltInDoc.appendChild(v);

At this point v will show 'moo'.  But now look at this sample.

var v = document.createElement('input');
eltInDoc.appendChild(v);
v.defaultValue = 'goo';
v.defaultValue = 'moo';

In this case, the input field stays empty.  This is obviously buggy behavior, 
and should be fixed either by reverting to our previous behavior (which also 
matches IE's behavior) or by truly caching a notion (e.g., with a bit in the 
content node) of whether or not the input field has left its initial state.  

e.g., for GetValue:
  if (bit is set)
    return the default value;
  else
    if (frame) return frame's value; else return stashed content node .value;

and for SetValue
  if (!bit is set)
    set the bit;
  if (frame)
    set the frame's val;
  else
    stash the val;

and in the input frame code:
  if (contents of frame ever change)
    set the bit on our content node;

This would cover 90% of the cases in XUL anyway and help retain an intuitive 
model, but honestly I don't see what the harm is in reflecting the value 
attribute changes into the form control like we used to.
 

Comment 21

[David Hyatt]

BTW, just because it needs to be said, I won't sr wallpapering bugs that 
attempt to patch the XBL to cover up what is obviously a bug in the HTML input 
field's implementation.  

Comment 22

[David Hyatt]

An addendum to my pseudocode.  If the form element is ever reset, then the bit 
should also be cleared.

Comment 23

[[not reading bugmail]]

Hyatt, the problem I'm seeing in ns6.2 is that prefs.js file is not being write
to disk on using the application 'x' close button..[which I use all the time to
close mozilla] but will write to disk on File->Exit.  And I'm assuming that
hitting 'ok' is part of the XBL to write perfs to memory and not to disk till
File->Exit is issued?  Should I'll file another bug?

Comment 24

[[not reading bugmail]]

is it possible writing to disk is also is causing issues in the currently nightly?

Comment 25

[John Keiser (jkeiser)]

This is *not* clearly a bug.  Different people apparently have different
opinions on what it should do.  In the end, .setAttribute("value") should only
set *one*

The spec seems to say setting "value" should reset the control.  JST says this
is a bug in the spec that will be corrected in the next version (and he ought to
know, his name is at the top of this document).  As sicking has pointed out, one
strange effect about this is that input.defaultValue = input.defaultValue is
*not* a noop--it has side effects.  It is counterintuitive.

From the DOM spec, emphasis mine:
http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-26091157
defaultValue of type DOMString
When the type attribute of the element has the value "text", "file" or
"password", this represents the HTML value attribute of the element. The value
of this attribute does not change if the contents of the corresponding form
control, in an interactive user agent, changes. See the value attribute
definition in HTML 4.0.

*Even if we do this* XBL should be changed.  By doing setAttribute("value")
instead of .value, it is setting .defaultValue as well as .value, which in most
cases is not the effect they are trying to achieve.  It is bad form in any
universe except one where setAttribute("value") == .value.

I would rather implement hyatt's suggestion though.  It keeps us somewhat intuitive.

Comment 26

[Blake Ross]

hyatt: as john said, that was a temporary workaround to help us find call sites 
needing to be fixed to open the tree.

Comment 27

[David Hyatt]

Do you mean "XUL should be changed"?  XBL isn't setting the value attribute. 
XUL is.

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch from jkeiser that implements hyatts proposal

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think i'm leaning towards liking hyatts proposal best (but i'm not sure if we 
might break any existing content).

But in any case, the current XUL is broken (XUL, not XBL), it should not 
do .setAttribute("value", foo), but rather .value=foo, so that is defenetly a 
bug. But IMHO we should add something (hack or not) in asap that unbreaks the 
current XUL, and give people time to fix it.

Also, as usual, we should not mimic IE's behaviour since it's very 
inconsistent. IE resets .value on .setAttribute("value") but not 
on .defaultValue=...

Comment 30

[John Keiser (jkeiser)]

Attachment: Input Element Test

Comment 31

[David Hyatt]

To make SetValueChanged a little faster, you could even cache whether or not 
the value changed in the frame too, so you don't have to repeatedly QI the 
content node and make the virtual function call to tell the content node 
something it already knows.  I doubt that will be too big a deal though.

r/sr=hyatt

Comment 32

[Johnny Stenback (:jst)]

r/sr=jst

Comment 33

[John Keiser (jkeiser)]

JST has checked in this patch.  It fixes home page for me, and other prefs
panels I am able to get to.  Many prefs panels are hindered by lingering
(unrelated) crashers.

There are *probably* prefs panels that don't work properly because they are
using setAttribute("value").  They will all, however, initialize properly.  I
have filed bug 108482 to fix these and we'll get to them shortly, but I don't
think they are going to be of blocker severity.

Comment 34

[Tracy Walker [:tracy]]

verified on linux commercial 2001-11-06-10-trunk & mac commercial 
2001-11-06-04-trunk

Comment 35

[Axel Hecht]

so as blake backed out his textbox.xml change, and I can't use 
aTextbox.setAttribute('value','foo') like .value on a xul:textbox, is the 
changed behaviour intentional?
And if so, would someone mind posting to a few newsgroups announcing this?

Comment 36

[hanasaki]

Hanges when entering EMail.
Please reopen.

Comment 37

[Tracy Walker [:tracy]]

the hang entering email today may be due to a new bug with PSM.  I haven't 
logged the bug yet.  but anything that needs access to PSM is hanging

Comment 38

[Tracy Walker [:tracy]]

see bug 108661 for hangs this morning

Comment 39

[[not reading bugmail]]

ok, I understand now about XBL vs XUL.. it appears XUL checkboxes such as ns6.2
is not writting general.* prefs to disk for the checkbox I select to use to the
pref in the file on the disk.

Comment 40

[[not reading bugmail]]

ok, found the problem.. Mozilla and ns6.2 will read the prefs file if you do a
manual selection from the desktop shortcut just fine, settings will stick.  The
problem lies in the commandline option for -P <profilename> to bypass the
profile picker.  This will not read the settings from the file.. and -P
<profilename> was broken last week and someone checked in a change for it.  So
prefs stick upon manual profile selection, not -P option.  :)

Comment 41

[[not reading bugmail]]

filed bug 109015 for -P <profilename> is not reading pref settings on load..

Comment 42

[[not reading bugmail]]

also filed bug 109018 for clicking on the application 'X' close button after all
open windows are shutdown should write prefs.js file to disk.