1692885 - Figure out a solution for XUL hidden persistence on XUL <treecol>

Status: RESOLVED FIXED

(Toolkit :: General, task, P2)

Comment 1

[Tim Nguyen :ntim]

Changing:

              if (element.getAttribute("hidden") == "true") {
                element.setAttribute("hidden", "false");
              } else {
                element.setAttribute("hidden", "true");
              }

to

element.hidden = !element.hidden;

breaks XUL persistance for hidden="false". This may be problematic for columns that have been unhidden, but are hidden by default.

I would suggest switching to the collapsed attribute, since treecol[hidden="true"] has a rule for visibility: collapse anyway, and persistance would be kept since collapsed is XUL specific.

Comment 2

[Tim Nguyen :ntim]

Attachment: Bug 1692885 - Switch back to setting hidden='false' on XUL treecols. r=Gijs

Comment 3

[Tim Nguyen :ntim]

Switching to collapsed is probably feasible, but also lots of work, so just switching back this specific usage for now.

Comment 4

[Tim Nguyen :ntim]

This turns out not being an issue in XUL boolean attribute land (un-hidden is persisted as hidden="", which is un-hidden for XUL), so the current code is ok. Though that still needs to be fixed for HTML bool attributes.

Attribute removals should be persisted differently (not as empty-strings).

Comment 5

[Tim Nguyen :ntim]

TLDR, this is facing a problem where some places used to set hidden="false" and the false value would be persisted, but now the hidden attribute would just be removed (and hidden="" is persisted). This is problematic for treecols with hidden="true" by default, where un-hiding treecols would not be persisted after the underlying change (current state would restore hidden="" instead of hidden="false" which would both be considered as hidden in HTML land).

All usages of persistence with hidden are on <treecols>: https://searchfox.org/mozilla-central/search?q=persist%3D%22.*hidden&path=&case=false&regexp=true

Solutions include:

1. setting another attribute to force-hide and persist that too (kinda ugly imo)
2. switching to collapsed to hide columns (that would really just move the problem to the collapsed attribute though)
3. persisting some internal value like _moz_attribute_removed_ instead of persisting an empty string? and make the xul store code handle that keyword differently. There's the potential issue of .setAttribute("hidden", "_moz_attribute_removed_") fooling this, but I guess that's unlikely...
4. wait for <treecols> to stop being used and stop caring about this issue.

Solution 3. seems best, since it gives room for migrating other boolean style attributes as well.

Comment 6

[Magnus Melin [:mkmelin]]

Not sure what #5 was?

Maybe it's worth special casing hidden and the few other known boolean attributes during xul store persist/restore, and let those persist true/false? When storing, if set and not "false" store true, else store false. When restoring, toggleAttribute based on the true/false string from storage.

Comment 7

[Tim Nguyen :ntim]

(In reply to Magnus Melin [:mkmelin] from comment #6)

Not sure what #5 was?

Ah I started this line with 3., so bugzilla thought it was a list item.

Maybe it's worth special casing hidden and the few other known boolean attributes during xul store persist/restore, and let those persist true/false? When storing, if set and not "false" store true, else store false. When restoring, toggleAttribute based on the true/false string from storage.

Yeah, persisting the JS property instead would be a good idea here.

Comment 8

[Tim Nguyen :ntim]

Persisting the JS property doesn't look too nasty to implement, essentially it consists of:

• creating new methods called GetPersistedAttr() and SetPersistedAttr() which are essentially equivalents of Element::GetAttr()/SetAttr(), but special-casing attribute name hidden and using Hidden()/SetHidden() there instead. Potentially issues include:

  • XULPersist.cpp is mostly based on mozilla::dom::Element, whereas as we're dealing with XULElement, which suggests it might just be easier to do GetBoolAttr()/SetBoolAttr(), though at the same time as bug 1691321 (otherwise, GetBoolAttr()/SetBoolAttr() will return the wrong thing). Alternatively, it could be done independently, but then we'd need to duplicate the XUL versions of these methods.
  • Having to cast boolean to string in C++ and vice-versa, but that's probably not too hard

• Replacing the usages of GetAttr()/SetAttr() in dom/xul/XULPersist.cpp with the new methods.

• Handling backwards compat well (either through XULStore migration in BrowserGlue, either through XULPersist.cpp logic)

Overall, it doesn't look too hard.