Closed Bug 1692885 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: General, task, P2)

task

Tracking

()

RESOLVED FIXED

People

(Reporter: ntim, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 obsolete file)

No description provided.
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Attachment #9203256 - Attachment description: Bug 1692885 - Switch back ot setting hidden='false' on XUL treecols. r=Gijs → Bug 1692885 - Switch back to setting hidden='false' on XUL treecols. r=Gijs
Attachment #9203256 - Attachment is obsolete: true

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).

No longer regressed by: 1692113
Summary: Fix XUL hidden persistance for XUL <treecol> → Figure out a solution for XUL hidden persistence on XUL <treecol>
Assignee: ntim.bugs → nobody
Status: ASSIGNED → NEW

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.

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.

Severity: -- → N/A
Type: defect → task
Priority: -- → P2

(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.

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.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.