257437 - [GTK2] NS_THEME_SPLITTER implementation

Status: RESOLVED FIXED

(Core Graveyard :: Skinability, defect)

Description

[Pierre Chanial]

Some comments on the patch that will follow:
1) GTK has the ability to draw solid separator between widgets. Our equivalent
is <separator class="groove"/>. Given that GTK and mac make no difference
between a separator in toolbar or elsewhere, I removed
NS_THEME_TOOLBAR_SEPARATOR for now, and introduced NS_THEME_SEPARATOR that is
triggered with -moz-appearance: separator (same as before, for the mac
<toolbarseparator/>'s).

2) There is still an inconsistency between XUL <separator/> and the 'separator'
property value. However, the element <separator/> is misleading. Currently,
<separator/> only adds a gap between XUL elements and <spacer/> (without |flex|
attribute) just do nothing. <separator/> should always materialize the
separation by drawing a line and <spacer/> (without |flex| attribute) should
behave like <separator/> currently behaves, and then deprecate <separator
class="groove"/>. Doing so would be consistent with the behaviour of the
elements <toolbarseparator/> and <toolbarspacer/>.

3) But, in order to not break things, in the gnomestripe, I only used
NS_THEME_SEPARATOR for <menuseparator/> and <toolbarseparator/>, but not yet for
<separator/>.

4) I am not happy with the ugly hack in GetParentBoxOrientation. To speed up
things I didn't used the computed style to get the box orientation of the parent
container. I instead relied on the box frames bits. I would prefer to query a
box interface and then use nsIBox::GetOrientation, but that's not possible
currently. Please tell me how to proceed. dbaron told me that the native theme
code should not belong to gfx, but that's imo out of the scope of this bug (I'd
be glad to do the work, though).

5) The NS_THEME_SPLITTER implementation is somewhat disruptive with the
non-native one. Non-native splitters draw lines. GTK Paned widget instead leave
to the children the responsibility of drawing the borders. And my implementation
of NS_THEME_SPLITTER just do that: it only draws the splitter background and
handle but no borders. I think that's a much better approach, because in the
case in which we have two splitters:
<vbox>
  <hbox flex="1">
    <hbox id="p1" flex="1" style="border: 1px solid -moz-DialogText"/>
    <splitter/>
    <hbox id="p2" flex="1" style="border: 1px solid -moz-DialogText"/>
  </hbox>
  <splitter/>
  <hbox flex="1" style="border: 1px solid -moz-DialogText"/>
<vbox/>
the drawing of the intersection of the two splitters is "clean", in that there
would be no horizontal line between the 'p1' and 'p2' box on the top border of
horizontal splitter.

6) I removed the -moz-appearance: menu; in the classic mac menu.css. That's a
left over from bug 118296. It should be 'menupopup' instead of 'menu', but
anyways... even if there is some code to handle it, NS_THEME_MENUPOPUP is not
supported for mac.

Comment 1

[Pierre Chanial]

Attachment: patch v1.0

also, it's rather difficult not to duplicate code, because every horizontal or
vertical widget can be styled differently by the gtk theme.

Comment 2

[[e-mail]]

(In reply to comment #0)
>2) There is still an inconsistency between XUL <separator/> and the
>'separator' property value. However, the element <separator/> is misleading.
>Currently, <separator/> only adds a gap between XUL elements and <spacer/>
>(without |flex| attribute) just do nothing. <separator/> should always
>materialize the separation by drawing a line and <spacer/> (without |flex|
>attribute) should behave like <separator/> currently behaves, and then
>deprecate <separator class="groove"/>. Doing so would be consistent with the
>behaviour of the elements <toolbarseparator/> and <toolbarspacer/>.
Giving a spacer a default or minimum width would be a bad idea, but perhaps a
<spacer class="separator"> or something.

>3) But, in order to not break things, in the gnomestripe, I only used
>NS_THEME_SEPARATOR for <menuseparator/> and <toolbarseparator/>, but not yet
>for <separator/>.
What about <separator class="groove"/>?

>4) I am not happy with the ugly hack in GetParentBoxOrientation.
At least for splitters you can just check the orient attribute, as the splitter
sets this itself automatically.

>5) The NS_THEME_SPLITTER implementation is somewhat disruptive with the
>non-native one. Non-native splitters draw lines. GTK Paned widget instead
>leave to the children the responsibility of drawing the borders.
So did the old 4.x splitters. I've always wondered why Mozilla was different.

Comment 3

[Pierre Chanial]

(In reply to comment #2)
> Giving a spacer a default or minimum width would be a bad idea, but perhaps a
> <spacer class="separator"> or something.
Why would it be a bad idea? <spring/> should be used instead of <spacer/> in the
case of flexible width.
In my opinion, <separator/>, <spacer/> and <spring/> should behave like
<toolbarseparator/>, <toolbarspacer/> and <toolbarspring/>.
We should have classes like "thin" or "thick" for <spacer/> elements.

> >3) But, in order to not break things, in the gnomestripe, I only used
> >NS_THEME_SEPARATOR for <menuseparator/> and <toolbarseparator/>, but not yet
> >for <separator/>.
> What about <separator class="groove"/>?
Good point. Even if I am advocating the deprecation of <separator
class="groove"/> (and doing so would add -moz-appearance:separator for
<separator/> elements, and it would work for class="groove"/>) that should not
be done in this bug.

> >4) I am not happy with the ugly hack in GetParentBoxOrientation.
> At least for splitters you can just check the orient attribute, as the splitter
> sets this itself automatically.
Right, but there would be two implementations to get the orientation, and the
hack would still be there for NS_THEME_SEPARATOR.

Comment 4

[[e-mail]]

(In reply to comment #3)
>(In reply to comment #2)
>>Giving a spacer a default or minimum width would be a bad idea, but perhaps a
>><spacer class="separator"> or something.
>Why would it be a bad idea? <spring/> should be used instead of <spacer/> in
>the case of flexible width.
<spring/> was deprecated aeons ago. I was surprised to see so many still left in
seamonkey when I lxr'd just now.

Comment 5

[Pierre Chanial]

(In reply to comment #4)
> (In reply to comment #3)
> <spring/> was deprecated aeons ago. I was surprised to see so many still left in
> seamonkey when I lxr'd just now.

I found the bug number (bug 99876). Unfortunately, there is not a lot of discussion.

A compromise would be to add a rule like:
spacer:not([flex]) {
  height: 1em;
}

doing so would not break the gazillion of uses of <spacer flex="1"/> in the code
base and <spacer/> would add a space. (and <spring/> could still be undeprecated
:) )

Comment 6

[Ian Spence]

Would it be ok if I took this bug and patch and ran with it? He doesn't seem to have been around in a while and it'd be nice to get splitters working.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Of couse. Go for it.

Comment 8

[Ian Spence]

Attachment: Implements native looking splitters

The first of three files needed for this patch. This implements NS_THEME_RESIZER and adds some css fixes to make the places dialog and theme dialog look better.

The next two files are just css files that need to get uploaded.

Comment 9

[Ian Spence]

Attachment: The new /toolkit/themes/gnomestripe/global/splitter.css

This gets added to /toolkit/themes/gnomestripe/global/

Comment 10

[Ian Spence]

Attachment: The new /toolkit/themes/gnomestripe/mozapps/extensions/extensions.css

This file gets added to /toolkit/themes/gnomestripe/mozapps/extensions

That directory does not currently exist.

Comment 11

[Ian Spence]

In an attempt at the elusive quadruple post, it should be noted that this checkin wouldn't resolve the bug. This does not implement NS_THEME_SEPARATOR due to some things I wasn't sure about. I didn't want the resizer not to get in while I waited on the separator. I'll submit a patch for separators once this gets landed.

Comment 12

[Ian Spence]

Attachment: Before the patch

Comment 13

[Ian Spence]

Attachment: After the patch

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+// A solid separator.  Can be horizontal or vertical.
+#define NS_THEME_SEPARATOR                                 18
+

Take this out.

The patch seems to end in the middle.

Comment 15

[Ian Spence]

Attachment: Updated patch

That's odd. This patch is complete and removes the line requested

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  return !aFrame->GetContent()->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::orient,
+                                            NS_LITERAL_STRING("vertical"), 
+                                            eCaseMatters);

Use the nsIATom version of this (add your atom to nsWidgetAtomList.h). Otherwise great!

Comment 17

[Ian Spence]

Attachment: Addresses comment

I think this is what you meant.

Comment 18

[Reed Loden [:reed]]

Comment on attachment 293991 [details] [diff] [review]
Addresses comment

More Linux native theming goodness.

Comment 19

[Michael Monreal [:micmon]]

Can you check that this fixes all the cases that are listed in 404974 and then mark that bug as a duplicate of this one? Thanks!

(Note: using the latest nightly, things look much worse than what is shown in the "before" shot above. does this already have some patch applied?)

Comment 20

[Ian Spence]

That before shot was taken directly from the latest nightly. Perhaps if you are using a different GTK theme, the problems were made more apparent.

As for your issues, everything looks great, except for the sidebar, which looks better. The handle is painted correctly, but since neither the sidebar nor the part holding the webpage specifies a border, it kinda looks like the sidebar in nautilus

Comment 21

[Ian Spence]

Attachment: Screenshot of patches addons dialog

Comment 22

[Ian Spence]

Attachment: Screenshot of patched page info dialog

My only complaint with this is the fact that it isn't obvious where it is unless you look for it. This is because handles are supposed to be painted where there is already a clear divide

Comment 25

[Michael Monreal [:micmon]]

@Ian:

I tested again with other gtk engines (was using clearlooks with a custom color scheme) and found out that even default clearlooks works better... I will compare again after your fixes land, right now I don't really see any obvious problem in my color scheme...

Comment 26

[Ian Spence]

I wasn't suggesting it was a problem with your color scheme, I just know that some GTK themes show the errors in our rendering more than others.

One last note is that this will not completely fix your complaint with thunderbird splitters [ attachment 291666 [details] ] since this will not restore the border that was removed. This is likely due to the addition of a "-moz-appearance: none" for the top box. I'd have to look into the Thunderbird source to fix that and I don't even know how to find that

Comment 27

[Michael Monreal [:micmon]]

Yeah, I was just saying that my color scheme is probably broken ;)

I can file another bug for thunderbird and link this one... but I already found out that it is kind of hard to get in touch with the thunderbird developers, at least compared to the firefox devs...

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 293991 [details] [diff] [review]
Addresses comment

a=beltzner for 1.9

Comment 29

[Reed Loden [:reed]]

Checking in browser/themes/gnomestripe/browser/places/organizer.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/places/organizer.css,v  <--  organizer.css
new revision: 1.7; previous revision: 1.6
done
Checking in layout/style/nsCSSKeywordList.h;
/cvsroot/mozilla/layout/style/nsCSSKeywordList.h,v  <--  nsCSSKeywordList.h
new revision: 3.96; previous revision: 3.95
done
Checking in layout/style/nsCSSProps.cpp;
/cvsroot/mozilla/layout/style/nsCSSProps.cpp,v  <--  nsCSSProps.cpp
new revision: 3.156; previous revision: 3.155
done
Checking in toolkit/themes/gnomestripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/jar.mn,v  <--  jar.mn
new revision: 1.25; previous revision: 1.24
done
RCS file: /cvsroot/mozilla/toolkit/themes/gnomestripe/global/splitter.css,v
done
Checking in toolkit/themes/gnomestripe/global/splitter.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/splitter.css,v  <--  splitter.css
initial revision: 1.1
done
Checking in toolkit/themes/gnomestripe/mozapps/jar.mn;
/cvsroot/mozilla/toolkit/themes/gnomestripe/mozapps/jar.mn,v  <--  jar.mn
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css,v
done
Checking in toolkit/themes/gnomestripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css,v  <--  extensions.css
initial revision: 1.1
done
Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.58; previous revision: 1.57
done
Checking in widget/src/gtk2/gtkdrawing.h;
/cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v  <--  gtkdrawing.h
new revision: 1.45; previous revision: 1.44
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.126; previous revision: 1.125
done
Checking in widget/src/xpwidgets/nsNativeTheme.cpp;
/cvsroot/mozilla/widget/src/xpwidgets/nsNativeTheme.cpp,v  <--  nsNativeTheme.cpp
new revision: 1.43; previous revision: 1.42
done
Checking in widget/src/xpwidgets/nsNativeTheme.h;
/cvsroot/mozilla/widget/src/xpwidgets/nsNativeTheme.h,v  <--  nsNativeTheme.h
new revision: 1.26; previous revision: 1.25
done
Checking in widget/src/xpwidgets/nsWidgetAtomList.h;
/cvsroot/mozilla/widget/src/xpwidgets/nsWidgetAtomList.h,v  <--  nsWidgetAtomList.h
new revision: 1.23; previous revision: 1.22
done

Comment 30

[Reed Loden [:reed]]

I really screwed up the commit message. Filed bug 409377 on fixing it.

Comment 31

[Reed Loden [:reed]]

Checking in gfx/public/nsThemeConstants.h;
/cvsroot/mozilla/gfx/public/nsThemeConstants.h,v  <--  nsThemeConstants.h
new revision: 1.23; previous revision: 1.22
done

Comment 32

[[e-mail]]

Comment on attachment 293859 [details]
The new /toolkit/themes/gnomestripe/global/splitter.css

>/* ::::: splitter grippy ::::: */
>
>grippy {
>  display: none;
>}
Why did you regress bug 382591?

Comment 33

[Ian Spence]

There is no real equivalent in GTK. However, replicating the behavior of GTK (or at least nautilus), calls for grippy to be invisible, but usable.  I had based my patch off of existing code and hadn't really taken that into consideration.


That being said, I assume that the following css would be appropriate

grippy {
  visibility: hidden;
}

However, I'm not sure if that hidden objects respond to events.

I'm reopening this because I never actually got to implement NS_THEME_SEPARATOR.  The fix for the grippy should be included in that patch, which I should be able to post next weekend.

Comment 34

[[e-mail]]

(In reply to comment #33)
> That being said, I assume that the following css would be appropriate
> 
> grippy {
>   visibility: hidden;
> }
> 
> However, I'm not sure if that hidden objects respond to events.
If it's the right thing to do then it's easier than that; like XUL boxes it defaults to transparent, so all it needs is an appropriate size.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #33)
> That being said, I assume that the following css would be appropriate
> 
> grippy {
>   visibility: hidden;
> }
> 
> However, I'm not sure if that hidden objects respond to events.

They don't.

You can use opacity:0 to make something invisible but still responding to events.

Comment 36

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

(In reply to comment #33)
> I'm reopening this because I never actually got to implement
> NS_THEME_SEPARATOR.

Filed bug 433058 for that.