53091 - Tree selection performance is horrible

Status: RESOLVED WORKSFORME

(Core :: XUL, defect, P2)

Description

[David Hyatt]

The following rule pattern in CSS:

tree:focus blah blah blah treerow {

}

is extremely inefficient and (in our implementation of CSS) results in a 
style-re-resolve on all descendants of the focused tree.  Given that this is not 
easy to fix for 6.0, I would argue that these rules should be backed out of all 
skins for now.

Note that the focus ring rule is ok, since it operates directly on the tree (and 
not on descendants of the tree), so it can stay.

We would lose:
  (1) The dotted border around the current item when focused
  (2) The color change on selected items when trees are focused

Comment 1

[David Hyatt]

Re-assigning to hangas and nominating for nsbeta3.

Comment 2

[Joe Hewitt (gone)]

Point well taken.  Just to note, there is only one descendant combinator in the 
tree focus selection rules, e.g.

tree:focus > treechildren treeitem[selected="true"] > treerow

... not three as in the example you gave.  By the way, I've changed the rules 
of focus rings quit a bit.  No descendant combinators, but plenty of long 
series of child combinators, e.g.

#folderTree > treehead > treerow > .treecell-header

... are these reasonably efficient?

Comment 3

[Phil Peterson]

cc'ing andreww

Comment 4

[David Hyatt]

Nope. Anything of the form

tree:focus with anything after it, be it a child or descendant

is going to be too slow.

Comment 5

[David Hyatt]

Actually, let me study the CSS implementation so that I can try to answer this 
extremely accurately.

For now the row rules should definitely go.

Comment 6

[David Hyatt]

Ok, hold up.  I think something is going wrong with focus here that's making 
this worse.  Stay tuned.

Comment 7

[David Hyatt]

Yay, I'm glad i kept looking at this!

The rules can stay.  This is actually a problem with :focus and :active in 
nsEventStateManager.cpp.  I have a fix.  Part of the slowdown was also a problem 
with :active being incorrectly applied (and slowing down the tree for :focus 
cases).  

A happy side-effect of this fix is that it also makes twisties and scrollbars
in tree views properly go into :active now.
  

Comment 8

[Joe Hewitt (gone)]

Dave, will this affect the :active state on treecells as well? Ben and I have 
attempted to make treecells appear pressed when clicked, but they have never 
matched any CSS rules like treecell:active.

Comment 9

[Peter Trudelle]

nsbeta3+, This is one of the smoking gun dependencies for tree performance,
which seems to be a dogfood issue for several engineers, and probably should be
nsbeta3+ in its own right.

Comment 10

[David Hyatt]

Oops. Blew away trudelle's nsbeta3+ of the bug.  Putting it back.

Comment 11

[Joe Hewitt (gone)]

So should I, or should I not, remove the accused selectors from the modern and 
classic themes? 

Comment 12

[David Hyatt]

They're ok.  Keep 'em.

Comment 13

[David Hyatt]

Keep 'em.  I have the fix in hand for the nsEventStateManager.cpp.

Comment 14

[David Hyatt]

fixed.

Comment 15

[andreww]

Great! tree:focus elements are very useful.

Comment 16

[John Morrison]

verified fixed win32 092005, linux 092013, mac 091920

Comment 17

[Judson Valeski]

reopening and changing OS to linux. linux mail/news is not usable because of 
this problem.

Comment 18

[David Hyatt]

jud, are you using a debug or an optimized build?

Comment 19

[David Hyatt]

I suspect joki's hover changes as being the culprit here.

Comment 20

[Peter Trudelle]

nominating for rtm, but need info on exactly how to reproduce.

Comment 21

[Peter Trudelle]

resolving as wfm