251273 - Better attribute persistence

Status: NEW

(Core :: XUL, defect)

Description

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

In order to do a number of features and fix certain bugs (see bug 215489 for
example) we need some more control over how attributes are saved and restored
from localstore. Here's a table showing different events/methods and what we
think should happen:

Event              | Function               | DS Operation
-------------------+------------------------+-------------------
Document Load      | Attrs Restored FROM DS | READ
Insertion          | Attrs Restored FROM DS | READ
setAttr("persist") | Attrs Saved    TO   DS | WRITE
setAttr(attr)      | Attr  Saved    TO   DS | WRITE
.persist(id, attr) | Attr  Saved    TO   DS | WRITE
.restore(id, attr) | Attr  Restored FROM DS | READ

It was metioned persist/restore could take 'null' or empty string as the second
parameter and use that as an indication to save/restore all attributes. 

We may not need .restore, but its functionality is mentioned here, in case it is
ever needed.

Comment 1

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

congratulations on your purchase of bug 251273, when used correctly it will give
you many hours of enjoyment. 

Comment 2

[jag (Peter Annema)]

(In reply to comment #0)
> Event              | Function               | DS Operation
> -------------------+------------------------+-------------------
> Document Load      | Attrs Restored FROM DS | READ
> setAttr(attr)      | Attr  Saved    TO   DS | WRITE
> .persist(id, attr) | Attr  Saved    TO   DS | WRITE

The above three currently exist.

> Insertion          | Attrs Restored FROM DS | READ
> setAttr("persist") | Attrs Saved    TO   DS | WRITE

These two should be made to work.

> .restore(id, attr) | Attr  Restored FROM DS | READ

This may be nice to have, but I can't think of a use case.

Comment 3

[jag (Peter Annema)]

Btw, there's a bug in setAttr(attr) where the attribute name will match if it's
a substring of an ID in the persist attribute's value, e.g.

  <element id="foo" persist="abc" abc="10" b="0"/>

  getElementById("foo").setAttribute("b", "1");

This will cause attribute "b" to get persisted.

Comment 4

[jag (Peter Annema)]

Brendan said we need numbered issues. Let's try this:

Issue I: Persisted attributes aren't restored for dynamically inserted content.

Issue II: There are two bugs in setAttr(attr). a) Attributes whose names are
substrings of persisted attributes also get persisted. b) setting the "persist"
attribute should persist all attributes mentioned in case of addition.

Issue III: Another thing we were discussing was whether or not to filter when
restoring from localstore.

Say you have v1.0 of your doc and you persist e.g. the "hidden" attribute of
some element. Then in v1.5 you don't want to let that element be hidden anymore.
If it was hidden before the user upgraded, now it'll still be hidden, even
though "hidden" isn't listed in your "persist" attribute any longer. So you have
to add code to your onload method (probably from a timeout, even) to deal with
this upgrade scenario. And there's no way to remove it from the localstore.
Suckage. Wouldn't it be nice if you could specify which attributes you want
restored from localstore?

There are several things we could do to solve this problem.

1) Break backward compatibility and just filter on what's in the persist attribute.

   a) This means we now do have a need for .restore(id, attr) since we'd need a
way to restore things that were persisted with .persist() and aren't in
persist="..."

   b) Or we add a restore="..." attribute that lists the extra/dynamic
attributes we want restored.

2) Keep backward compatibility by only filtering when some other attribute exists.

   a) restorefilter="true". This additionally requires (1a) or (1b) to restore
extra/dynamic attributes.

   b) restore="attr1,attr2". We could make this the full list, or, to avoid
duplication, specify that it'll automatically include the attributes listed in
persist="...", so in most cases it'd end up restore="", and occassionally
restore="dynamicAttr".

Comment 5

[Nickolay_Ponomarev]

II.a from comment 4 is reported as bug 231333.

Issue III is reported in bug 231334.

Another (major imo) issue is bug 15232, which makes things like <button hidden="true" persist="hidden"/> not work.