591651 - CssHtmlTree should avoid hacking innerHTML

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Joe Walker [:jwalker]]

From Justin Dolske's review.
See https://bugzilla.mozilla.org/show_bug.cgi?id=582596#c78

>+StyleGroupView.prototype = {
>+  /**
>+   * The click event handler for the title of the style group view.
>+   */
>+  click: function StyleGroupView_click()
>+  {
...
>+    if (!this.closed) {
>+      this.element.style.display = "none";
>+      this.toggle.innerHTML = "▶";

Eww.

This should just be done with CSS. Set an attribute (|open="true"|, like XUL),
and have 1 structural rule that controls .display, and 1 theme rule that
controls the twisty widget appearance.

We've got native-looking twisties for Windows in
toolkit/themes/winstripe/global/tree/, not sure where the OS X ones are coming
from. There's also toolkit/themes/*/global/arrow/ which is uglier but on all
platforms. Maybe just use those for now, and file a theme bug to sort out where
to get nice arrows for any platform.

>+PropertyView.prototype = {
...
>+  click: function PropertyView_click(aEvent)
>+  {
...
>+    if (!this.closed) {
>+      this.element.style.display = "none";
>+      if (this.toggle) {
>+        this.toggle.innerHTML = "▶";
>+      }

Ditto for using CSS and chrome:// images here.

Comment 1

[Joe Walker [:jwalker]]

In reply to my cry for help, Rob Campbell added:

>> > >+      this.toggle.innerHTML = "▶";

> > This should just be done with CSS. Set an attribute (|open="true"|,
> > like XUL), and have 1 structural rule that controls .display, and 1 theme
> > rule that controls the twisty widget appearance.

He's suggesting you add an open attribute to your node to control when it opens or closes.

As in, <div id="twisty" open="false">

> > We've got native-looking twisties for Windows in
> > toolkit/themes/winstripe/global/tree/, not sure where the OS X ones are
> > coming from. There's also toolkit/themes/*/global/arrow/ which is uglier
> > but on all platforms. Maybe just use those for now, and file a theme bug
> > to sort out where to get nice arrows for any platform.

Here, he's suggesting using a CSS rule to control the look of the twisty in each state with something like:

div#twisty {
 -moz-appearance: treetwistyclosed;
}

div#twisty[open="true"] {
 -moz-appearance: treetwistyopen;
}

Those rules will work on mac or linux, but for winstripe, we need to use something different. Dolske suggests some graphics from toolkit/themes/winstripe/global/tree, but I'm having a rough time getting them to work in the tree on windows. Still trying to figure that out there.

Comment 2

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #1)
> In reply to my cry for help, Rob Campbell added:
> 
> >> > >+      this.toggle.innerHTML = "▶";
> 
> > > This should just be done with CSS. Set an attribute (|open="true"|,
> > > like XUL), and have 1 structural rule that controls .display, and 1 theme
> > > rule that controls the twisty widget appearance.
> 
> He's suggesting you add an open attribute to your node to control when it opens
> or closes.
> 
> As in, <div id="twisty" open="false">
> 
> > > We've got native-looking twisties for Windows in
> > > toolkit/themes/winstripe/global/tree/, not sure where the OS X ones are
> > > coming from. There's also toolkit/themes/*/global/arrow/ which is uglier
> > > but on all platforms. Maybe just use those for now, and file a theme bug
> > > to sort out where to get nice arrows for any platform.
> 
> Here, he's suggesting using a CSS rule to control the look of the twisty in
> each state with something like:
> 
> div#twisty {
>  -moz-appearance: treetwistyclosed;
> }

I was corrected (again by the inestimable Gavin Sharp):

s/treetwistyclosed/treetwisty/

Comment 3

[Mihai Sucan [:msucan]]

I have addressed the initial comments posted by Dolske without using XUL. I have implemented something that works without the toggle, without the innerHTML. The script just changes the open="true|false" attribute of the group/property view. From CSS the group is open/closed based on the attribute value.

Post-beta5 we can make further changes, if needed.