80837 - Twisties and thread lines should not be selected with rows

Status: RESOLVED FIXED

(Core :: XUL, enhancement, P3)

Description

[Blake Ross]

Jan, giving this to you because I don't think hyatt will be able to get to 
these for awhile.  If you don't want them, reassign to hyatt, or perhaps I can 
take them.

Twisties and thread lines should not be selected with the rest of the rows.  
Attaching screenshot to clarify.

Comment 1

[Blake Ross]

Attachment: screenshot

Comment 2

[[e-mail]]

Actually Windows supports two styles of lists:
1. Full row select (i.e. how Mozilla currently does it)
2. "Primary" text only select
My guess is that XUL authors would like to choose the highlight type.

Comment 3

[Jan Varga [:janv]]

Yeah, that sounds reasonable to ne

Comment 4

[Brant Gurganus]

[RFE] is deprecated in favor of severity: enhancement.  They have the same meaning.

Comment 5

[[e-mail]]

Latest ideas on IRC:
seltype="single" -> single selection
seltype="primary" -> single selection, but only highlighting primary cell text

Also, the selected and current properties will only reflect on the row or cell
text styles as appropriate for the seltype.

Comment 6

[[e-mail]]

Attachment: Implement seltype="primary"

Comment 7

[[e-mail]]

This doesn't work; the cell text needs to know when either the cell or row is
selected for the text colour, but only when the cell is selected for the border.

Unless there's a way to persuade :-moz-tree-cell-text to inherit colour...

Comment 8

[[e-mail]]

Wait, it's the border that threw me, but that's the "current" style. So, the
rule for "selected" should apply to the cell text for primary seltype, but to
both the row and the cell text for normal seltype, while the rule for "current"
is the either/or approach that I already have. I think that should work...

Comment 9

[[e-mail]]

Attachment: Fixed issue with previous patch

I also included the diff for folderPane.xul so you would have a tree to look
at.

Comment 10

[[e-mail]]

Attachment: Updated for bitrot

Comment 11

[[e-mail]]

Attachment: More bitrot

Comment 12

[Jan Varga [:janv]]

I see a few issues here:

1. We might want to support multiple selection in primary column, so we should
create a new attribute for this purpose. selstyle ?
I just checked IE's Organize Favorities and I saw it there.

2. seltype="primary" is cached, but this attribute may change
There are two possibilities, check the attribute directly each time or update
cached value when attribute changes

3. lineContext, if I understand it properly, primary reason for this is that we
have a CSS rule -moz-tree-line(selected,focus) to invert line color when the row
is selected. I don't think it solves the problem in general
We can have similar rule for images or twisties (objects inside the cell), what
will we do then ? add lineContext to PaintImage and PaintTwisty too ?
anyway, PaintImage would not work using lineContext, which is actually a context
for row not cell.

I still wonder about pure CSS implementation of this feature.
I know we would have to use "not" selectors, etc
What about a new class instead of attribute ?

We should probably measure perf impact of using "not" in selectors.

Comment 13

[Jan Varga [:janv]]

Attachment: example

Comment 14

[[e-mail]]

Jan, if you use attachment 115637 [details] [diff] [review] then twisties and images will see the selected
style when the text is selected. I don't know if that's an issue for twisties;
if it is then I would have to fetch the context early like I had to for lines.
If I do it any other way I will have to fetch two sets of cell properties, once
for the parts that are selected and once for the parts that are not selected.

Adding a new attribute is not a big issue apart from adding it to the atom list.

Dynamically watching the attribute is a problem because the tree has no frame.
Refetching the attribute at paint time would otherwise suffice.

Comment 15

[[e-mail]]

Attachment: My go at using CSS

These CSS rules make it so that <tree class="primary"> only displays the
selection on primary cell text.

Comment 16

[Jan Varga [:janv]]

This looks promising, I'll review it ASAP

Comment 17

[Jan Varga [:janv]]

The first thing I've noticed is that you forgot to update mac too.

Comment 18

[Jan Varga [:janv]]

I just checked with Joe, we tend to prefer selstyle="primary" as opposed to using
primary class.

Another issue, drop feedback does not highlight color of the underlying text.

And please update mac too, I'll try it on the mac then

Comment 19

[Jan Varga [:janv]]

Neil, actually I don't understand how your patch works w/o using not selector.
I hope it doesn't count with a bug in our style system.

>.primary > treechildren:-moz-tree-row,
>treechildren:-moz-tree-row {
>}
Do you know why treechildren:-moz-tree-row and other rules don't apply to
.primary trees ?
Am I missing something here ?

Thanks.

Comment 20

[[e-mail]]

.primary > increases the weight of the rule so that it overrules the
non-.primary rules.

Comment 21

[[e-mail]]

Attachment: Patch with drop feedback, mac theme and tree[selstyle="primary"]

Jan, thanks for catching that drop feedback bug.

Comment 22

[Jan Varga [:janv]]

Comment on attachment 116315 [details] [diff] [review]
Patch with drop feedback, mac theme and tree[selstyle="primary"]

Neil, this looks awesome!
r=varga

Comment 23

[Jan Varga [:janv]]

<jag> I would think that a class approach is cleaner...
<jag> If the purpose is purely for display style, class would be the thing to
use. If it's to convey information about the state of the object, then attribute
is more appropriate
<Jan> yeah

Jag is right, but I just found a reason why we need it as an attribute.
I think the tree click handler should be fixed to check for selstyle="primary"
and if that is true, change row selection only if the click targets at the text.
Otherwise it's a bit confusing from the usability point of view.

Neil, would you file a new bug for this issue ?
Thanks

Comment 24

[Jan Varga [:janv]]

I measured tree painting after applying this patch and I didn't see any
noticeable perf hit.

Comment 25

[jag (Peter Annema)]

Comment on attachment 116315 [details] [diff] [review]
Patch with drop feedback, mac theme and tree[selstyle="primary"]

sr=jag

Comment 26

[[e-mail]]

Fix checked in.

Comment 27

[Ere Maijala (slow)]

Did this break the highlighting of rows everywhere and folder icons in MailNews?
Bug 198642, bug 198656, bug 198653 etc.