1016063 - Trees display row selection even with seltype="cell" or seltype="text"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[[e-mail]]

SeaMonkey's UI has a tree with seltype="text". As of SeaMonkey 2.25 (Gecko 28), this tree displays a full row selection. SeaMonkey 2.24 correctly displays the text selection.

Note that the alternating background used by DOM Inspector does display correctly.

Comment 1

[[e-mail]]

Working: mozilla-central changeset 5eb1c89fc2bc
Broken: mozilla-central changeset 7f14d602e0c5

Comment 2

[[e-mail]]

Out of the bugs in that range, bug 922669 looks most suspicious.

Comment 3

[[e-mail]]

The first bad revision is:
changeset:   157911:3315c6b05d23
user:        Cameron McCormack <[e-mail]>
date:        Thu Nov 28 17:46:38 2013 +1100
summary:     Bug 922669 - Part 1: Parse selectors with user action pseudo-classes after pseudo-elements. r=bz

Comment 4

[Boris Zbarsky [:bzbarsky]]

Neil, are there any CSS errors being reported in this case?

Comment 5

[[e-mail]]

No, there are no CSS errors, in fact I think the CSS rules are being applied, but in the wrong order, so that the rules for tree[seltype="cell"] > treechildren aren't overriding the base treechildren rules. (This is hard to see with the background rules, but if I'm careful I can see that both row and text borders are being applied.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

What do the selectors for the relevant rules look like?

Comment 7

[[e-mail]]

Well, I think the relevent part of the windows theme looks like this:

treechildren::-moz-tree-row {
  border: 1px solid transparent;
  min-height: 18px;
  height: 1.3em;
}

treechildren::-moz-tree-row(selected) {
  background-color: -moz-cellhighlight;
}

treechildren::-moz-tree-row(selected, focus) {
  background-color: Highlight;
}

treechildren::-moz-tree-row(current, focus) {
  border: 1px dotted Highlight;
}

treechildren::-moz-tree-row(selected, current, focus) {
  border: 1px dotted #F3D982;
}

tree[seltype="cell"] > treechildren::-moz-tree-row,
tree[seltype="text"] > treechildren::-moz-tree-row {
  border: none;
  background-color: transparent;
}

So what seems to be happening is that those last rules aren't overriding the earlier rules as they should.

Comment 8

[[e-mail]]

Attachment: Patch to demonstrate the problem

This patch tweaks about:config to use seltype="text" which will readily demonstrate the problem.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, thank you.

So consider these three selectors:

  treechildren::-moz-tree-row

  treechildren::-moz-tree-row(selected, current, focus)

  tree[seltype="cell"] > treechildren::-moz-tree-row

On trunk, those are assigned weights of 1, 769, 258 respectively.  The reason for that is that we're now including the weight of pseudo-element selectors, since those can now have different weights (e.g. depending on whether they have :hover or not).  But tree pseudo-elements abuse the selector storage by storing the stuff in parens in the class list.  So we think that selector has three extra classes in it, effectively.  We need to fix CalcWeight to ignore those pseudo-classes for XUL tree pseudo-element selectors.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't include the fake classnames XUL tree pseudo-elements have when calculating style rule specificity

David, since Cameron is out, do you have time to review this?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't include the fake classnames XUL tree pseudo-elements have when calculating style rule specificity

Er, let's try to use PseudoType() consistently...

Comment 12

[[e-mail]]

Comment on attachment 8430538 [details] [diff] [review]
Don't include the fake classnames XUL tree pseudo-elements have when calculating style rule specificity

I can verify that this patch fixes the bug, thanks!

>+             "If pseudo-elements can have id or attribute  selectors "
Whitespace nit                                             ^^

Comment 13

[Boris Zbarsky [:bzbarsky]]

You're very welcome, and thank you for doing all the hard debugging bits!

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8430538 [details] [diff] [review]
Don't include the fake classnames XUL tree pseudo-elements have when calculating style rule specificity

Review of attachment 8430538 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing now I'm back.  Looks good, but I wonder if the assertions might be more easily understood if you inverted them:

  MOZ_ASSERT(!(IsPseudoElement() &&
#ifdef MOZ_XUL
               PseudoType() != nsCSSPseudoElements::ePseudo_XULTree &&
#endif
               mClassList),
             ...);

and similar for the other assertion.  It's easier to get my head around it this way, but maybe it's just me.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't include the fake classnames XUL tree pseudo-elements have when calculating style rule specificity

That patch didn't compile on Windows anyway, due to #ifdef in a macro argument, so let's try this

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d11841a6d0ee

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8432936 [details] [diff] [review]
Don't include the fake classnames XUL tree pseudo-elements have when calculating style rule specificity

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 922669
User impact if declined: Some XUL style rules for trees don't work right
Testing completed (on m-c, etc.): Passes tests
Risk to taking this patch (and alternatives if risky): I think this is low risk.
String or IDL/UUID changes made by this patch: None.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8432936 [details] [diff] [review]
Don't include the fake classnames XUL tree pseudo-elements have when calculating style rule specificity

Too late for FF30 here, we've gone to build on our final candidate and this is not serious enough a regression (also has already shipped in 28/29) so we can let it ride up from FF31.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a132629caa5f