15232 - persist should be able to remove an attribute from an element

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[hangas]

persist should be able to remove an attribute from an element.  Currently persist
saves the value of an attribute to the localstore.rdf file only if there is an
attribute.  We need a way to save the fact that there is no attribute so that
when the document is built the next time it can remove an attribute that is set
by default in the XUL.  We currently use checked="true" on menu items that must
persist, the problem comes in when we remove this attribute to remove the
checkmark, nothing is saved to the localstore.rdf file, so the next time this
window is displayed it does not get the checked="true" removed from the element.

We need to fix globalOverlay.js when this is fixed, to remove the hack of setting
checked="false".

Comment 1

[Chris Waterson]

I suck.

Comment 2

[Rann Bar-On]

any chance of this being done in the near future?  it's really annoying when
trying to create checkbox menuitems that use persist to save their values.  The
only workaround is to use the prefs file, which is really cumbersome.  This will
prevent companies attempting to add menus for their employees from using such
menus, which could be a major problem with acceptence.  Recommend fixing before PR3.

Comment 3

[Rann Bar-On]

nominating nsbeta3, because of above reasons.

Comment 4

[Chris Waterson]

This would be a great feature to have, but I'm not going to be able to get to it
before nsbeta3.

Comment 5

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

I no longer see a checked="false" hack in any of the globalOverlay.js files in the repository, but setting autocheck="false" on the menuitem, then manually setting the attribute to "false" when the menuitem is unchecked (i.e. doing menuitem.setAttribute("checked", "false") instead of menuitem.removeAttribute("checked")), correctly hides the checkmark and enables the persistence of that state.

I have added a note about this workaround to the menuitem entry in the XUL reference <http://developer.mozilla.org/en/docs/XUL:menuitem>.

Comment 6

[Decathlon]

> I have added a note about this workaround to the menuitem entry in the XUL
> reference <http://developer.mozilla.org/en/docs/XUL:menuitem>.

https://developer.mozilla.org/en/XUL/menuitem

Comment 7

[Ian Nartowicz]

This works now?  I have a situation where I removed a persisted attribute and it is not present when I start a new session.  The attribute is on a box in my own dialog window and set attributes persist.

Comment 8

[u462496]

(In reply to Ian Nartowicz from comment #7)
> This works now?  I have a situation where I removed a persisted attribute
> and it is not present when I start a new session.

Any update on this?  I am observing the same thing (persisting of no checked attribute), but I don't want to rely on it until I have verification from the devs.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 15232 - Allow persist to remove attributes. r=smaug

In a somewhat hacky way (by storing an special token). This should
probably be good enough for chrome code.

This is the root cause of bug 1792870 (before the regression we were
setting hidden="true/false" and thus storing the hidden attribute). Now
we're not because the attribute is removed, and since there are columns
that are hidden by default we'll never un-hide them.

We don't seem to have many tests for XULPersist... :(

I can try to write some.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 15232 - Allow persist to remove attributes. r=smaug

In a somewhat hacky way (by storing an special token). This should
probably be good enough for our UI.

This is the root cause of bug 1792870 (before the regression we were
setting hidden="true/false" and thus storing the hidden attribute). Now
we're not.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a915fb2a70a9
Allow persist to remove attributes. r=smaug

Comment 13

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/a915fb2a70a9