Closed Bug 257437 Opened 17 years ago Closed 14 years ago

[GTK2] NS_THEME_SPLITTER implementation

Categories

(Core Graveyard :: Skinability, defect)

Other Branch
x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: p_ch, Assigned: ispence)

References

Details

Attachments

(7 files, 3 obsolete files)

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.
Attached patch patch v1.0 (obsolete) — Splinter Review
also, it's rather difficult not to duplicate code, because every horizontal or
vertical widget can be styled differently by the gtk theme.
(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.
(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.
(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.
(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
:) )
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.
Status: NEW → ASSIGNED
Assignee: p_ch → ispence
Status: ASSIGNED → NEW
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.
Attachment #157422 - Attachment is obsolete: true
Attachment #293858 - Flags: superreview?(roc)
Attachment #293858 - Flags: review?(roc)
This gets added to /toolkit/themes/gnomestripe/global/
This file gets added to /toolkit/themes/gnomestripe/mozapps/extensions

That directory does not currently exist.
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.
Status: NEW → ASSIGNED
Attached image Before the patch
Attached image After the patch
+// A solid separator.  Can be horizontal or vertical.
+#define NS_THEME_SEPARATOR                                 18
+

Take this out.

The patch seems to end in the middle.
Attached patch Updated patch (obsolete) — Splinter Review
That's odd. This patch is complete and removes the line requested
Attachment #293858 - Attachment is obsolete: true
Attachment #293959 - Flags: superreview?(roc)
Attachment #293959 - Flags: review?(roc)
Attachment #293858 - Flags: superreview?(roc)
Attachment #293858 - Flags: review?(roc)
+  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!
I think this is what you meant.
Attachment #293959 - Attachment is obsolete: true
Attachment #293991 - Flags: superreview?(roc)
Attachment #293991 - Flags: review?(roc)
Attachment #293959 - Flags: superreview?(roc)
Attachment #293959 - Flags: review?(roc)
Attachment #293991 - Flags: superreview?(roc)
Attachment #293991 - Flags: superreview+
Attachment #293991 - Flags: review?(roc)
Attachment #293991 - Flags: review+
Comment on attachment 293991 [details] [diff] [review]
Addresses comment

More Linux native theming goodness.
Attachment #293991 - Flags: approval1.9?
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?)
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
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
Duplicate of this bug: 406975
Duplicate of this bug: 404974
@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...
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
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 on attachment 293991 [details] [diff] [review]
Addresses comment

a=beltzner for 1.9
Attachment #293991 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
I really screwed up the commit message. Filed bug 409377 on fixing it.
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 on attachment 293859 [details]
The new /toolkit/themes/gnomestripe/global/splitter.css

>/* ::::: splitter grippy ::::: */
>
>grippy {
>  display: none;
>}
Why did you regress bug 382591?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
(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.
(In reply to comment #33)
> I'm reopening this because I never actually got to implement
> NS_THEME_SEPARATOR.

Filed bug 433058 for that.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Summary: [GTK2] NS_THEME_SEPARATOR, NS_THEME_SPLITTER implementation → [GTK2] NS_THEME_SPLITTER implementation
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.