Bug 153146 - Web Inspector: cleanup TreeOutline class and separate styles from NavigationSidebarPanel
Summary: Web Inspector: cleanup TreeOutline class and separate styles from NavigationS...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks: 153034
  Show dependency treegraph
 
Reported: 2016-01-15 14:23 PST by Matt Baker
Modified: 2016-01-19 11:53 PST (History)
8 users (show)

See Also:


Attachments
[Patch] Proposed Fix (99.76 KB, patch)
2016-01-15 18:27 PST, Matt Baker
no flags Details | Formatted Diff | Diff
[Image] Broken tree element highlight (86.58 KB, image/png)
2016-01-17 14:44 PST, Matt Baker
no flags Details
[Patch] Proposed Fix (100.26 KB, patch)
2016-01-17 19:01 PST, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2016-01-15 14:23:25 PST
* 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.
Comment 1 Radar WebKit Bug Importer 2016-01-15 14:23:37 PST
<rdar://problem/24213071>
Comment 2 Matt Baker 2016-01-15 18:27:56 PST
Created attachment 269136 [details]
[Patch] Proposed Fix
Comment 3 Matt Baker 2016-01-15 18:31:59 PST
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.
Comment 4 Timothy Hatcher 2016-01-16 12:03:02 PST
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.
Comment 5 Matt Baker 2016-01-17 14:38:24 PST
(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;
}
Comment 6 Matt Baker 2016-01-17 14:44:13 PST
Created attachment 269200 [details]
[Image] Broken tree element highlight

Unfortunately, -webkit-padding-start produces the correct indentation but breaks the tree element highlight.
Comment 7 Matt Baker 2016-01-17 14:56:00 PST
(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?
Comment 8 Matt Baker 2016-01-17 15:11:22 PST
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.
Comment 9 Matt Baker 2016-01-17 19:01:39 PST
Created attachment 269209 [details]
[Patch] Proposed Fix
Comment 10 WebKit Commit Bot 2016-01-19 11:53:28 PST
Comment on attachment 269209 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 269209

Committed r195303: <http://trac.webkit.org/changeset/195303>
Comment 11 WebKit Commit Bot 2016-01-19 11:53:33 PST
All reviewed patches have been landed.  Closing bug.