Closed Bug 927779 Opened 11 years ago Closed 11 years ago

Windows Classic theme XUL tree cell padding regression

Categories

(SeaMonkey :: Themes, defect)

x86_64
Windows 7
defect
Not set
major

Tracking

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

VERIFIED FIXED
seamonkey2.25
Tracking Status
seamonkey2.21 --- unaffected
seamonkey2.22 --- wontfix
seamonkey2.23 --- fixed
seamonkey2.24 --- fixed

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

References

Details

(Keywords: regression, relnote)

Attachments

(4 files, 1 obsolete file)

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.)
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.
(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.
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).
I'm already watching the SeaMonkey product in bugzilla. No need to CC me for any SeaMonkey bug but thanks anyway.
Attached 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.
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?
Attached 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).
(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+
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: 11 years ago
Flags: needinfo?(jh)
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.25
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 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+
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.
Blocks: 886732
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: