Closed Bug 1016063 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

28 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: neil, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

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.
Out of the bugs in that range, bug 922669 looks most suspicious.
The first bad revision is:
changeset:   157911:3315c6b05d23
user:        Cameron McCormack <cam@mcc.id.au>
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
Blocks: 922669
Component: XP Toolkit/Widgets: XUL → CSS Parsing and Computation
Neil, are there any CSS errors being reported in this case?
Flags: needinfo?(neil)
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.)
Flags: needinfo?(neil)
What do the selectors for the relevant rules look like?
Flags: needinfo?(neil)
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.
Flags: needinfo?(neil)
This patch tweaks about:config to use seltype="text" which will readily demonstrate the problem.
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.
David, since Cameron is out, do you have time to review this?
Attachment #8430537 - Flags: review?(dbaron)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8430537 - Attachment is obsolete: true
Attachment #8430537 - Flags: review?(dbaron)
Flags: needinfo?(cam)
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                                             ^^
Attachment #8430538 - Flags: feedback+
You're very welcome, and thank you for doing all the hard debugging bits!
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.
Attachment #8430538 - Flags: review?(dbaron) → review+
That patch didn't compile on Windows anyway, due to #ifdef in a macro argument, so let's try this
Attachment #8432936 - Flags: review?(cam)
Attachment #8430538 - Attachment is obsolete: true
Attachment #8432936 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/d11841a6d0ee
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Flags: in-testsuite?
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.
Attachment #8432936 - Flags: approval-mozilla-beta?
Attachment #8432936 - Flags: approval-mozilla-aurora?
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.
Attachment #8432936 - Flags: approval-mozilla-beta?
Attachment #8432936 - Flags: approval-mozilla-beta-
Attachment #8432936 - Flags: approval-mozilla-aurora?
Attachment #8432936 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.