927779 - Windows Classic theme XUL tree cell padding regression

Status: VERIFIED FIXED

(SeaMonkey :: Themes, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Windows Classic theme XUL tree cell padding regression

Starting with SM 2.22, the Default (Classic) theme on Windows exposes a regression regarding XUL tree cell paddings.

The attached screen shot has SM 2.21 on the left and 2.22b1 on the right, produced with the same profile that started as a fresh profile and only has a feed account with the SM project feed added.

Since Modern is not affected, I guess some change to the Toolkit Windows theme CSS during the time Mozilla trunk was Mozilla 19 caused this, but I didn't investigate which one.

Since this is a very noticeable UX regression we should do our best to fix this in time for SM 2.22 final.

NB: The screen shot also shows another regression, namely that the "Hide the tab bar when only one tab is open" has no effect anymore. AFAICS that regression was fixed on trunk but not on Beta or Aurora yet. [That's for another bug but should ideally be addressed in time for SM 2.22 final, too.]

Comment 1

[[e-mail]]

I don't suppose you're running with accelerated graphics by any chance? (I don't have a Windows 7 VM handy but my unaccelerated Windows 2003 VM looks OK here.)

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Oh sorry, forgot. I've tested with a Win7 x64 VM inside a Win7 x64 host. Both have hardware acceleration and Aero enabled and working. Didn't have a chance to test without accel yet since I guess I'd have to restart my VM after changing the VM's settings, which I cannot do right now. Will try later.

Comment 3

[[e-mail]]

If it's Win7 Pro then you can try remote desktop from the host to the guest, that will present a different video driver to Gecko and might turn off acceleration.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #2)
> Didn't have a chance to test without accel yet

Now started the VM without accel (deselected "Accelerate 3D graphics", VMWare 9). No change.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

BTW TB is also affected, although not as obviously (the cell/row height is already increased compared to SM, probably due to the star icon).

Comment 6

[Philip Chee]

I'm already watching the SeaMonkey product in bugzilla. No need to CC me for any SeaMonkey bug but thanks anyway.

Comment 7

[[e-mail]]

Attachment: Stack backtrace

So, I stepped through the tree body frame code starting at GetRowHeight and got to MapRuleInfoInto where I found a height rule with a 1.8em value.

Comment 8

[[e-mail]]

This rule was added to tree-aero.css by bug 855998, apparently by design, so as to be able to use the aero style tree item highlighting. If you compare Explorer windows on, say, XP and 7, you should notice that the rows are noticeably further apart on 7 than they are on XP.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Thanks for the analysis Neil. I compared with Windows Explorer (just for the fun of it, I don't think it is something we should use as a design target for MailNews trees) but even there less space is wasted (i.e. more items fit vertically) than within MailNews.

So, where shall we override this? I guess we have more trees that are affected, e.g. the categories in Preferences, Data Manager etc. - even the Profile Manager, but I guess we're out of luck with that one.

I could probably live with the others staying regressed, but MailNews surely needs to be fixed.

FTR CSS style rules comparison:

/* toolkit windows tree-aero.css */
media (-moz-windows-default-theme) {
  treechildren::-moz-tree-row {
    height: 1.8em;
    -moz-margin-start: 1px;
    -moz-margin-end: 1px;
    border-width: 2px;
    border-style: solid !important;
    border-color: transparent;
    border-radius: 3px;
    background-repeat: no-repeat;
    background-size: 100% 100%;
    -moz-outline-radius: 3px;
  }
}

/* toolkit windows tree.css */
treechildren::-moz-tree-row {
  border: 1px solid transparent;
  min-height: 18px;
  height: 1.3em;
}

/* suite modern tree.css */
treechildren::-moz-tree-row {
  border: 1px solid transparent;
  background-color: transparent;
  min-height: 18px;
  height: 1.3em;
}

Comment 10

[[e-mail]]

(In reply to Jens Hatlak from comment #9)
> So, where shall we override this? I guess we have more trees that are
> affected, e.g. the categories in Preferences, Data Manager etc. - even the
> Profile Manager, but I guess we're out of luck with that one.
Actually profile manager inlcudes communicator.css too.

You'll have to be careful with your override. You can't set the height to 1.3em gloablly as that's too small to show tree icons with the other Aero changes, so you'll need to pick an intermediate height and restrict the change to Windows Vista and later.

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

I'd like to note that I'm not actively working on this (might be obvious to some - this bug has no assignee - but not all). Can anyone else provide a quick fix (mainly for 2.22 the release of which is approaching fast)?

Maybe add something like this to communicator.css:

treechildren::-moz-tree-row { height: 1.3em !important; }

(but mind Neil's comment 10) or

#folderTree > treechildren::-moz-tree-row,
#threadTree > treechildren::-moz-tree-row {
  height: 1.3em !important;
}

for MailNews only (where it probably hurts the most)

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Relnoted with the rule from comment 11.

(In reply to [e-mail] from comment #10)
> You can't set the height to 1.3em gloablly as that's too small to show tree
> icons with the other Aero changes

Confirmed (BTW the Bookmarks Manager is affected, too). Any idea which CSS rule one would have to add/modify in order to keep the 1.3em height and still get the icons back uncut at the bottom? I experimented a bit with padding and margin but those seem to have no effect, at least on treechildren::-moz-tree-row.

[Too bad treechildren cannot be inspected with DOMI. Does anyone know an alternate method of inspection/debugging?]

Comment 13

[[e-mail]]

Border width perhaps?

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #13)
> Border width perhaps?

Yay! So this should do the trick:

treechildren::-moz-tree-row {
  height: 1.3em !important;
  border-width: 1px !important;
}

Will update the relnotes later today.

Now do we still need to find a way to restrict this rule to Aero or could we apply this to the Default theme on all platforms?

Comment 15

[[e-mail]]

No, I think those are the defaults on all platforms, so you should be OK.

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

OK, then we just need to make sure not to regress other things. The rule from comment 14 is too broad; e.g. it enables borders on the location bar autocomplete list. The below is an attempt to fix that.

treechildren::-moz-tree-row {
  height: 1.3em !important;
  border-width: 1px !important;
}
#urlbar treechildren::-moz-tree-row {
  height: 1.4em !important;
  border-width: 0 !important;
}

I guess this needs much more testing.

Comment 17

[[e-mail]]

I think autocomplete lists are excluded from the tree changes using a :not(.autocomplete-treebody) selector.

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #17)
> I think autocomplete lists are excluded from the tree changes using a
> :not(.autocomplete-treebody) selector.

Works. Neil++

So I'll relnote this:

treechildren::-moz-tree-row:not(.autocomplete-treebody) {
  height: 1.3em !important;
  border-width: 1px !important;
}

Comment 19

[Jens Hatlak (:InvisibleSmiley)]

Bah, sorry, should be

treechildren:not(.autocomplete-treebody)::-moz-tree-row {
  height: 1.3em !important;
  border-width: 1px !important;
}

of course. This time I actually tested it. ;-)

Comment 20

[Frank Wein [:mcsmurf]]

BTW: To raise my opinion here, I did not file a bug back then as I actually liked the new look. Maybe the height of the items could be reduced a bit, but other than that the mouse over effect is nice and I would like to keep it.

Comment 21

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Frank Wein [:mcsmurf] from comment #20)
> BTW: To raise my opinion here, I did not file a bug back then as I actually
> liked the new look. Maybe the height of the items could be reduced a bit

I'd be completely happy if the rule from comment 19 could land in some way. Personally I don't need any changes on top of that, but the height change definitely needs to be undone. It's a waste of space, especially in MailNews. No-one ever raised a request to have the height changed; it was merely forced upon us. The platform devs' reasoning (Windows Explorer compat I think) doesn't change anything about those two facts.

> but other than that the mouse over effect is nice and I would like to keep
> it.

Didn't we have that before? Anyway, doesn't bother me so can stay AFAIC.

So... Rule from comment 19 -> communicator.css + comment 15 = OK?

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

We need some coverage in order not to regress Linux, Mac, or non-Aero Windows.

Trees to test (list may be incomplete):
* Address book (left; top right)
* Bookmarks Manager (left; top right)
* Data Manager (left; right)
* Download Manager
* DOM Inspector (left; right)
* Help
* History
* Location bar fast bookmarking widget (expanded folder selector)
* MailNews main window (left; top right)
* Preferences (left)
* Sidebar (e.g. Bookmarks)
* Switch Profile

Comment 23

[[e-mail]]

Comment on attachment 831078 [details] [diff] [review]
patch

r=me assuming we get consensus that it doesn't break anything and doesn't impact the Aero look too much (I noticed a subtle difference).

Comment 24

[Stefan [:stefanh]]

Comment on attachment 831078 [details] [diff] [review]
patch

This file is linux-/win-only, so you don't need any mac feedback (mac classic use suite/themes/classic/mac/communicator/communicator.css).

Comment 25

[Frank Wein [:mcsmurf]]

Attachment: Screenshots comparing trunk build with patch against SM 2.21 on Windows XP

I took a few screenshots to compare SeaMonkey 2.21 with a trunk build with the patch on Windows XP (so non-aero). Looks the same to me. Maybe we should make the new CSS rule apply/change to
:-moz-system-metric(windows-default-theme)
only? After all the new CSS rules in toolkit/ tree-aero.css apply to Windows using Aero only. Therefore on Windows XP SeaMonkey 2.22 looks the same as SeaMonkey 2.21 (I checked, though I did not take screenshots for that).

Comment 26

[Philip Chee]

(In reply to Frank Wein [:mcsmurf] from comment #25)
> Created attachment 8333577 [details]
> Screenshots comparing trunk build with patch against SM 2.21 on Windows XP
> 
> I took a few screenshots to compare SeaMonkey 2.21 with a trunk build with
> the patch on Windows XP (so non-aero). Looks the same to me. Maybe we should
> make the new CSS rule apply/change to
> :-moz-system-metric(windows-default-theme)
> only? After all the new CSS rules in toolkit/ tree-aero.css apply to Windows
> using Aero only. Therefore on Windows XP SeaMonkey 2.22 looks the same as
> SeaMonkey 2.21 (I checked, though I did not take screenshots for that).

does:
@media all and (-moz-windows-classic)
Work? e.g. http://hg.mozilla.org/comm-central/rev/300882418350#l1.18

Comment 27

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Philip Chee from comment #26)
> does:
> @media all and (-moz-windows-classic)
> Work? e.g. http://hg.mozilla.org/comm-central/rev/300882418350#l1.18

No (checked with Stylish). Not surprisingly, though, since we don't want to affect the Windows classic theme here but Aero.

Comment 28

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #27)
> (In reply to Philip Chee from comment #26)
> > does:
> > @media all and (-moz-windows-classic)
> > Work? e.g. http://hg.mozilla.org/comm-central/rev/300882418350#l1.18
> 
> No (checked with Stylish). Not surprisingly, though, since we don't want to
> affect the Windows classic theme here but Aero.

Those that work for me in place of -moz-windows-classic are:
-moz-windows-default-theme (cf. comment 25)
-moz-windows-compositor

(cf. <https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Media_queries>)

Don't know which one would be more appropriate.

Comment 29

[Frank Wein [:mcsmurf]]

Comment on attachment 831078 [details] [diff] [review]
patch

So all in all feedback+ as it also looks ok on Windows 7, but I would prefer a
@media (-moz-windows-default-theme) {
...
}

around the new CSS rule as Windows XP was not affected by the new CSS rule from tree-aero.css (see http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/themes/windows/global/tree-aero.css) because of that CSS selector (I think that's the correct term :).

Comment 30

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch with media query

Neil, please let me know if you like this one better and whether you'd like to have a comment added pointing to bug 855998. Technically the checkin comment (which will point to this bug) would suffice I guess.

Comment 31

[[e-mail]]

Comment on attachment 8334157 [details] [diff] [review]
patch with media query

The query is fine by me. (It actually includes XP; toolkit excludes XP by serving it tree.css instead of tree-aero.css but that's harder for us to achieve.) I guess you could mention the tree-aero.css override in a comment but a bug number would be unnecessary.

Comment 32

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 8334157 [details] [diff] [review]
patch with media query

https://hg.mozilla.org/comm-central/rev/8e2ff917958d

Decided to go without an extra comment.

Comment 33

[Jens Hatlak (:InvisibleSmiley)]

Note to self: Request branch approval once this has baked on trunk for a bit.

Comment 35

[Jens Hatlak (:InvisibleSmiley)]

Checked all points from comment 22. All except DOMI (which has its own code / repository) are fixed. Hence marking VERIFIED.

I found that about:config is still affected AFAICS. If anyone feels this needs to be fixed, too, please file a new bug so we can check what can be done there.

Comment 36

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 8334157 [details] [diff] [review]
patch with media query

[Approval Request Comment]
Regression caused by (bug #): bug 855998
User impact if declined: Huge waste of space, especially in MailNews
Testing completed (on m-c, etc.): Trunk + locally
Risk to taking this patch (and alternatives if risky): Low
String changes made by this patch: None

Comment 37

[Philip Chee]

Comment on attachment 8334157 [details] [diff] [review]
patch with media query

a=me for aurora and beta.

Comment 38

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 8334157 [details] [diff] [review]
patch with media query

https://hg.mozilla.org/releases/comm-aurora/rev/67cca6617820
https://hg.mozilla.org/releases/comm-beta/rev/4e718ada995d

Comment 39

[David E. Ross]

Windows 7 (x64)
SeaMonkey 2.22.1

Regarding the last paragraph in the original Description:  I do not see this problem.  I have the checkbox checked for "Hide the tab bar when only one tab is open".  I do not get the tab bar unless I have two or more tabs open.

Comment 40

[Jens Hatlak (:InvisibleSmiley)]

(In reply to David E. Ross from comment #39)
> Regarding the last paragraph in the original Description:  I do not see this
> problem.

That's because corresponding bug 902312 had been fixed in SM 2.22.