Windows Classic theme XUL tree cell padding regression

VERIFIED FIXED in seamonkey2.25

Status

defect
--
major
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

Tracking

({regression, relnote})

Trunk
seamonkey2.25
x86_64
Windows 7
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.21 unaffected, seamonkey2.22 wontfix, seamonkey2.23 fixed, seamonkey2.24 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

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.]
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.)
Assignee

Comment 2

6 years ago
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.
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.
Assignee

Comment 4

6 years ago
(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.
Assignee

Comment 5

6 years ago
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

6 years ago
I'm already watching the SeaMonkey product in bugzilla. No need to CC me for any SeaMonkey bug but thanks anyway.
Posted file 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.
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.
Assignee

Comment 9

6 years ago
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;
}
(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.
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)
Relnoted with the rule from comment 11.

(In reply to neil@parkwaycc.co.uk 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?]
Keywords: relnote
Border width perhaps?
(In reply to neil@parkwaycc.co.uk 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?
No, I think those are the defaults on all platforms, so you should be OK.
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.
I think autocomplete lists are excluded from the tree changes using a :not(.autocomplete-treebody) selector.
(In reply to neil@parkwaycc.co.uk 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;
}
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. ;-)
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.
(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?
Posted patch patch (obsolete) — Splinter Review
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
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #831078 - Flags: review?(neil)
Attachment #831078 - Flags: feedback?(stefanh)
Attachment #831078 - Flags: feedback?(philip.chee)
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).
Attachment #831078 - Flags: review?(neil)
Attachment #831078 - Flags: review+
Attachment #831078 - Flags: feedback?(bugzilla)
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).
Attachment #831078 - Flags: feedback?(stefanh)
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

6 years ago
(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
(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.
(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 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 :).
Attachment #831078 - Flags: feedback?(bugzilla) → feedback+
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.
Attachment #8334157 - Flags: review?(neil)
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.
Attachment #8334157 - Flags: review?(neil) → review+
Assignee

Updated

6 years ago
Attachment #831078 - Attachment is obsolete: true
Attachment #831078 - Flags: feedback?(philip.chee)
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.
Note to self: Request branch approval once this has baked on trunk for a bit.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(jh)
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.25

Updated

6 years ago
Duplicate of this bug: 942030
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jh)
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
Attachment #8334157 - Flags: approval-comm-beta?
Attachment #8334157 - Flags: approval-comm-aurora?

Comment 37

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

a=me for aurora and beta.
Attachment #8334157 - Flags: approval-comm-beta?
Attachment #8334157 - Flags: approval-comm-beta+
Attachment #8334157 - Flags: approval-comm-aurora?
Attachment #8334157 - Flags: approval-comm-aurora+

Comment 39

6 years ago
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.
(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.

Updated

6 years ago
Blocks: 886732
You need to log in before you can comment on or make changes to this bug.