Bug 153146 – Web Inspector: cleanup TreeOutline class and separate styles from NavigationSidebarPanel

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

* SUMMARY
Cleanup TreeOutline class and separate styles from NavigationSidebarPanel. TreeOutline styles are defined in NavigationSidebarPanel.css, and in the few places outside the sidebar where tree are used (call stack popovers, object trees, the DOM content tree), the needed styles are copied from NavigationSidebarPanel.css.

In addition to adding TreeOutline.css to encapsulate CSS, the following items should be cleaned up:

- Remove the "small" TreeOutline/TreeElement class. Small items are the rule, rather than the exception. Define a new "large" class name for the Timelines sidebar panel tree outline.
- No need to mix large/small tree elements within the same tree. Our code currently supports this (somewhat), but it's never used.
- Remove the "two-line" TreeOutline/TreeElement class. This isn't used, and can always be added back in if needed.
- TreeOutline's DOM node is usually an <ol> element. Make this the default, but allow it to be overridden in the constructor.
- Rewrite the disclosure and status button styles so the don't rely on assumptions about the tree element height.

[Radar WebKit Bug Importer]

<rdar://problem/24213071>

[Matt Baker]

Created attachment 269136 [details]
[Patch] Proposed Fix

[Matt Baker]

I checked all the tree outline instances I could find throughout the UI to make sure nothing was broken by this patch:

- Content tree
- DOM tree
- Timelines tree
- Call stack popover trees
- Object trees (Element tab -> Node)
- Object trees (console)

As far as I can tell everything renders identically, to the pixel. Please let me know if there is a use case that I missed.

[Timothy Hatcher]

Comment on attachment 269136 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=269136&action=review

Nice clean up!

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:595
> +        const maximumSidebarTreeDepth = 32;

Not sidebar specific now.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:596
> +        const baseLeftPadding = 5; // Matches the padding in NavigationSidebarPanel.css for the item class. Keep in sync.

TreeOutline.css, not NavigationSidebarPanel.css.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:604
> +            styleText += "." + WebInspector.NavigationSidebarPanel.ContentTreeOutlineElementStyleClassName + childrenSubstring + " > .item { ";

Should not use WebInspector.NavigationSidebarPanel.ContentTreeOutlineElementStyleClassName, it should just be .tree-outline now.

[Matt Baker]

(In reply to comment #4)
> Comment on attachment 269136 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269136&action=review
> 
> Nice clean up!
> 
> > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:595
> > +        const maximumSidebarTreeDepth = 32;
> 
> Not sidebar specific now.
> 
> > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:596
> > +        const baseLeftPadding = 5; // Matches the padding in NavigationSidebarPanel.css for the item class. Keep in sync.
> 
> TreeOutline.css, not NavigationSidebarPanel.css.
> 
> > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:604
> > +            styleText += "." + WebInspector.NavigationSidebarPanel.ContentTreeOutlineElementStyleClassName + childrenSubstring + " > .item { ";
> 
> Should not use
> WebInspector.NavigationSidebarPanel.ContentTreeOutlineElementStyleClassName,
> it should just be .tree-outline now.

Fixing this broke the indentation of object tree outlines, since the generated rules take precedence. Investigating further, the generated rules don't seem to be necessary. A simple change to TreeOutline.css accomplishes the same thing:

.tree-outline .children {
    -webkit-padding-start: 10px;
}

[Matt Baker]

Created attachment 269200 [details]
[Image] Broken tree element highlight

Unfortunately, -webkit-padding-start produces the correct indentation but breaks the tree element highlight.

[Matt Baker]

(In reply to comment #6)
> Created attachment 269200 [details]
> [Image] Broken tree element highlight
> 
> Unfortunately, -webkit-padding-start produces the correct indentation but
> breaks the tree element highlight.

The same problem occurs using padding-left instead of -webkit-padding-start. The generated styles applied padding to list items (.item), not the ordered list (.children). This works perfectly:

.tree-outline .children > .item {
    padding-left: 10px;
}

I'm not sure why we ever generated padding rules in the first place. I know we specified a maximum depth, beyond which children all had the same indentation, but it's unclear why we would want this (except to limit the amount of generated CSS). Maybe I'm missing something?

[Matt Baker]

The above solution doesn't work, I was using the wrong build. A possible solution is to add a tree outline class name for tree's that use the generated indentation rules, and provide a property (something like `customIndentation`.) to control the behavior.

[Matt Baker]

Created attachment 269209 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 269209 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 269209

Committed r195303: <http://trac.webkit.org/changeset/195303>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.