566178 - search bar suggestions have alternating background (autocomplete tree shouldn't be considered a multicol tree)

Status: VERIFIED FIXED

(Toolkit :: Themes, defect)

Description

[Dão Gottwald [:dao]]

Attachment: screenshot

Comment 1

[Steffen Wilberg]

Attachment: undo odd row highlighting in autocomplete.css

I think we want to undo odd row highlighting in all autocomplete result popups.

Tested on Linux, needs testing on Mac.

Comment 2

[Neil Deakin]

OK, what is the way one is supposed to use to specify that this kind of highlighting is to be used or not?

What is the relationship with the alternatingbackground attribute?

Comment 3

[Nomis101]

(In reply to comment #1)
> Tested on Linux, needs testing on Mac.

On Mac it looks like this (latest trunk, Mac OS X 10.6):
http://img32.imageshack.us/img32/7702/bildschirmfotof.jpg

Comment 4

[Steffen Wilberg]

Neil, the odd-row-highlighting is specified by tree.css for all trees with more than one column:
  treechildren::-moz-tree-row(multicol, odd) {
    background-color: -moz-oddtreerow;
  }

In bug 282127, my first approach was to sprinkle 'alternatingbackground="tree"' to every tree with more than one column. Then I found the general solution above by implementing "multicol".

The alternatingbackground="true" attribute is obsolete, not unused anywhere except DOM Insepctor, and not even styled by Gnomestripe. I propose to remove it altogether in bug 565460.

So the proposed way to suppress this highlighting is by using css like in the patch above. Since odd-row-highlighting is a platform convention on Mac and Linux, the need to suppress it should be limited to special cases like this.
This is just the second case after Gnomestripe's aboutsessionrestore.css, which wants a special "alternate" highlight instead.

Nomis101, thanks for the screenshot, looks good!

Comment 5

[Dão Gottwald [:dao]]

Can we make it so that this tree isn't considered "multicol" in the first place?

Comment 6

[Steffen Wilberg]

The code to implement multicol (bug 282127) is just this, in nsTreeBodyFrame::PrefillPropertyArray:
    if (mColumns->GetColumnAt(1))
      mScratchArray->AppendElement(nsGkAtoms::multicol);

The autocomplete popup uses a tree, and an (optional) comments column, which is used to show the "Suggestions" label in this case.
I don't think we want to change any of that...

I could add something like treechildren:not([alternatingbackground="false"]) to tree.css and alternatingbackground="false" to search.xml or autocomplete.xml.

Or I could exclude autocomplete-trees from the odd-row-styling in tree.css:
-treechildren::-moz-tree-row(multicol, odd) {
+tree:not(.autocomplete-tree) treechildren::-moz-tree-row(multicol, odd) {

Comment 7

[Steffen Wilberg]

Attachment: alternate approach: tree:not(.autocomplete-tree)

So here's the approach from the last sentence in comment 6.
Sadly I had to add tree:not(.autocomplete-tree) to the "selected" and "selected, focus" cases as well to make them still apply.
Tested on Linux only.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 446071 [details] [diff] [review]
alternate approach: tree:not(.autocomplete-tree)

>-treechildren::-moz-tree-row(multicol, odd) {
>+tree:not(.autocomplete-tree) treechildren::-moz-tree-row(multicol, odd) {

treechildren:not(.autocomplete-treebody)?

>  background-color: -moz-oddtreerow;

Please indent 1px further.

Comment 9

[Steffen Wilberg]

Attachment: alternate approach, v2: treechildren:not(.autocomplete-treebody)

Right, that works as well, and should be faster.
I also fixed the indent.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 447376 [details] [diff] [review]
alternate approach, v2: treechildren:not(.autocomplete-treebody)

>--- a/toolkit/themes/gnomestripe/global/tree.css
>+++ b/toolkit/themes/gnomestripe/global/tree.css

>-treechildren::-moz-tree-row(multicol, odd) {
>- background-color: -moz-oddtreerow;
>+treechildren:not(.autocomplete-treebody)::-moz-tree-row(multicol, odd) {
>+  background-color: -moz-oddtreerow;
> }
> 
>-treechildren::-moz-tree-row(selected) {
>+treechildren::-moz-tree-row(selected),
>+treechildren:not(.autocomplete-treebody)::-moz-tree-row(multicol, odd, selected) {
>   background-color: -moz-cellhighlight;
> }

Do you actually care about 'multicol' and 'odd' here? Does it affect the selector weighting?

> 
> treechildren::-moz-tree-row(selected, focus),
>-treechildren::-moz-tree-row(multicol, odd, selected, focus) {
>+treechildren:not(.autocomplete-treebody)::-moz-tree-row(multicol, odd, selected, focus) {
>   background-color: Highlight;
> }

Same question.

Comment 11

[Steffen Wilberg]

Yes, see comment 7.
"selected" and "selected, focus" don't apply to "multicol, odd" rows.
So I do need "multicol, odd, selected" and "multicol, odd, selected, focus".

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> "selected" and "selected, focus" don't apply to "multicol, odd" rows.

Do they actually not apply or are they overridden?

Comment 13

[Steffen Wilberg]

They're overridden. Adding !important does the trick:

treechildren:not(.autocomplete-treebody)::-moz-tree-row(multicol, odd) {
  background-color: -moz-oddtreerow;
}

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

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

Comment 14

[Steffen Wilberg]

Dão: please decide which patch (or modification of the last patch by comment 13) you like...

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 447376 [details] [diff] [review]
alternate approach, v2: treechildren:not(.autocomplete-treebody)

I removed 'multicol' and 'odd' from said selectors and couldn't see a difference...

Comment 16

[Jesse Ruderman]

Attachment: screenshot (looks terrible on ubuntu)

Comment 17

[Daniel Holbert [:dholbert]]

Any chance this can get reviewed & landed in time for Firefox 4 beta 1?

Comment 18

[Steffen Wilberg]

Attachment: alternate approach, v3

Sorry, couldn't work on this while on vacation.

I misunderstood you in comment 10, thought you meant removing the entire "multicol, odd, selected" line instead of just the "multicol" and "odd" selectors from that line (and the same for the "selected, focus" line). I now did that and it works fine indeed.

I also changed Pinstripe's tree.css by removing "multicol" from the "selected" and "selected, focus" lines.

Tested on Linux but not Mac.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 454257 [details] [diff] [review]
alternate approach, v3

>--- a/toolkit/themes/pinstripe/global/tree.css
>+++ b/toolkit/themes/pinstripe/global/tree.css

> treechildren::-moz-tree-row(selected),
> treechildren::-moz-tree-row(odd, selected),
>-treechildren::-moz-tree-row(multicol, odd, selected) {
>+treechildren:not(.autocomplete-treebody)::-moz-tree-row(odd, selected) {
>   background-color: -moz-mac-secondaryhighlight;
> }

'odd' seems redundant here, including the preexisting selector.

> treechildren::-moz-tree-row(selected, focus), 
> treechildren::-moz-tree-row(odd, selected, focus),
>-treechildren::-moz-tree-row(multicol, odd, selected, focus) {
>+treechildren:not(.autocomplete-treebody)::-moz-tree-row(odd, selected, focus) {
>   background-color: Highlight;
>   color: HighlightText;
> }

ditto

Gnomestripe looks good.

Comment 20

[Steffen Wilberg]

Attachment: v4 (with changes from comment 19)

Here you are. I can't test on Mac though.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 454671 [details] [diff] [review]
v4 (with changes from comment 19)

> treechildren::-moz-tree-row(selected),
> treechildren::-moz-tree-row(odd, selected),
>-treechildren::-moz-tree-row(multicol, odd, selected) {
>+treechildren:not(.autocomplete-treebody)::-moz-tree-row(selected) {
>   background-color: -moz-mac-secondaryhighlight;
> }
> 
> treechildren::-moz-tree-row(selected, focus), 
> treechildren::-moz-tree-row(odd, selected, focus),
>-treechildren::-moz-tree-row(multicol, odd, selected, focus) {
>+treechildren:not(.autocomplete-treebody)::-moz-tree-row(selected, focus) {
>   background-color: Highlight;
>   color: HighlightText;
> }

In comment 19 I was also trying to refer to treechildren::-moz-tree-row(odd, selected) and treechildren::-moz-tree-row(odd, selected, focus) -- they seem redundant, please drop them while you're at it.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 454671 [details] [diff] [review]
v4 (with changes from comment 19)

>diff --git a/toolkit/themes/gnomestripe/global/tree.css b/toolkit/themes/gnomestripe/global/tree.css

>+treechildren::-moz-tree-row(selected),
>+treechildren:not(.autocomplete-treebody)::-moz-tree-row(selected) {

This doesn't look right... isn't this supposed to be a replacement rather than an addition?

Comment 24

[Steffen Wilberg]

Attachment: v5 (comments 21 and 23 addressed)

OK, I blame the existing code for tricking me into thinking this needs to be so complicated ;-)

Tested on Linux only.

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 455259 [details] [diff] [review]
v5 (comments 21 and 23 addressed)

I see, autocomplete.css provides its own styling for ::-moz-tree-row(selected), otherwise the double selectors from the previous patches would actually have made sense.

Comment 26

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/b0f99f426843

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with trunk builds on OS X and Linux like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100712 Minefield/4.0b2pre

Can we get an automated test to make sure we don't regress it again?