New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Complete 3.26 light/dark themes, and various improvements #21

Closed
smithfred opened this Issue Jan 16, 2018 · 20 comments

Comments

Projects
None yet
3 participants
@smithfred

smithfred commented Jan 16, 2018

https://github.com/smithfred/firefox-gnome-theme-3.26-layer

It has working/visually accurate 3.26 light/dark themes, and it also addresses the following issues: #18, #19, #20.

Yeah, it's a separate add-on project, not a pull request, since it was just a bunch of fixes I was keeping separate originally.

It's designed to sit next to an untouched copy of firefox-gnome-theme and add to/override things as necessary (see its README). Maybe I'll re-organise things later/fork firefox-gnome-theme and merge everything into the fork, don't know.

@ghost

This comment has been minimized.

ghost commented Jan 16, 2018

Nice work! One thing I noticed: if you zoom in on a page, the scrollbar is zoomed as well.

@smithfred

This comment has been minimized.

smithfred commented Jan 16, 2018

That was fast ;)

But yeah, I know about the zoom issue:

https://github.com/smithfred/firefox-gnome-theme-3.26-layer/blob/master/userChrome.css#L61

I don't know if there's much that can be done about it without resorting to JS, since there's nothing I could find that exposes the browser zoom level to CSS, and the default "don't scale" behaviour is buried behind -moz-appearance.

@ghost

This comment has been minimized.

ghost commented Jan 16, 2018

Oh, I totally missed that. I'll see it as a feature to indicate page zoom.

@smithfred

This comment has been minimized.

smithfred commented Jan 17, 2018

Additional improvement now: auto-detects whether CSD is enabled and the number of window controls.

@kurogetsusai

This comment has been minimized.

Owner

kurogetsusai commented Feb 3, 2018

Hi! Sorry for disappearing for quite a while, I didn't have much time for my open source projects recently. Thanks for all those new features and fixes, I'll try to merge them with this repo, unless you want to create pull requests instead?

The general rule is everything which matches normal GTK behavior and doesn't break anything goes to theme.css and all extra features need to be disabled by default and manually enabled in customChrome.css by the user. Everything needs to be opt-in, because there is no way to opt-out in CSS. For example your repo enables some features by default by @importing them in userChrome.css and there is no way to turn them off. So keep that in mind if you decide to add features in a pull request.

That CSD auto-detect code is pretty clever, good job. :D Do you know if it works for both Fedora's FF 58 and normal FF 59?

@smithfred

This comment has been minimized.

smithfred commented Feb 4, 2018

My repo's overriding a bunch of stuff since it's just layering on this one, so I'd need to incorporate them into a clone of this repo first.

Also the FF58 fixes are similarly layered on via a separate file; nothing else is touched except the default imports and one extra one in userChrome.css, from memory.

The CSD detection from the first commit of my repo doesn't work past FedFox 57 - the state of "CSD available" was determined by Fedora's about:config key in their 57. FedFox 58 just backports the upstream CSD implementation, which always reports "CSD available" - the FF58 fixes file has info on the updated detection approach, which should also work in vanilla 59 (which Fedora should also end up using). The condition in userChrome.css avoids importing the CSD stuff for vanilla 58 though.

I think it's a good idea to decide on a branching/versioning policy BTW - branches for diff. FF versions (Fedora's CSD stuff/backporting is detectable so doesn't need to be treated separately), and tags/releases following FF's versioning, probably. Since 57 is obsolete now, a working version can be branched and tagged then ignored :D

@smithfred

This comment has been minimized.

smithfred commented Feb 4, 2018

BTW depending how much time you have for this project in future, you might want to think about how to allow the project to keep moving forward (other contributors, looking again at the integration team suggestion, etc.)

@kurogetsusai

This comment has been minimized.

Owner

kurogetsusai commented Feb 4, 2018

Okay, thanks. I started merging your changes. There are many unrelated changes in a single commit (the initial commit in your repo) and seems like your editor reformatted all the whitespace (tabs → 4 spaces), even lines which weren't changed, so the diffs are messy. I gotta format it back and compare, it's gonna take some time. Most of that code works for FF 57 too, I think I'll just release a final version of this theme for FF 57 after this and we can move on to FF 58.

The GNOME Integration Team didn't answer me last time, looks like they are busy too. I'll continue maintaining this project for now and others can contribute through pull requests. It's not much work to merge a pull request if it's properly formatted (one feature or a bug fix per pull request) and I'd like to review every commit here to understand the changes. Don't worry, I'll let you know if I decide to abandon it, so someone else will take over this project, maybe even you. ;p

kurogetsusai added a commit that referenced this issue Feb 4, 2018

kurogetsusai added a commit that referenced this issue Feb 4, 2018

@smithfred

This comment has been minimized.

smithfred commented Feb 4, 2018

Well the explanation for it being a big initial commit is in the first post on this issue ;)

I replaced tabs with spaces in my proj. because tabs are the work of the devil ;) But it's mostly my own CSS except for one file I copied because it was unavoidable if I wanted to avoid modifying the original project files, I think. Though if it takes a while for you to convert back to evil indentation, I'd be a bit worried by your editor's (non-)capabilty ;)

Oh wait, you mean the 3.26 vs. 3.18 themes? I started with 3.18 light, made changes, then synthesised a new 3.26 dark theme from 3.18 dark and 3.26 light, I think... can't remember. I can check my private repo if it's really useful to know (the GH Git project is not my "real" one because I'm a nutcase and don't like the GH monoculture :D)

The concern is that with a SPOF, if you're gone for another month (which you're entitled to do!) then things grind to a halt. I'll follow up on the Integration Team ticket.

@kurogetsusai

This comment has been minimized.

Owner

kurogetsusai commented Feb 4, 2018

Tabs are "evil" only if you use Emacs instead of a real editor, so if your editor can't handle tabs, I guess your editor is the problem, not mine. ;p I can convert spaces to tabs, but it's not 100% accurate because spaces are not semantic indentation characters. Tabs are semantically for indentation, spaces are for alignment, then you can display the code with whatever tab width you want and it's gonna always work properly. But it doesn't matter for this project since I don't align CSS anyway, just indent. Besides, I noticed you misspelled "color" as "colour" in your 3.26 themes, so I corrected it back to "color"~

Wait, you made the 3.26 dark theme from 3.18 dark instead of the 3.26 which was already available in my repo? The original 3.26 dark theme was created by @rafaelmardojai, I have no way to check it since I don't use GNOME 3.26, so I trust your version is accurate too. The code seems to be way different. I don't think I need your private repo's history tho, I already started merging a snapshot of your repo, so I'll figure it out. But thanks for the offer.

It's open source and I picked the most open license possible - public domain. There really is no SPOF, if I get hit by a bus tomorrow, you can just fork it and move on. 4 people already forked it and several hundreds more cloned it. GitHub teams don't really help, according to what I read on GitHub's support pages, still just a single person owns the repo, not the whole team. And that's not a native git feature, I thought you didn't like the GitHub monoculture. >:D

@ghost

This comment has been minimized.

ghost commented Feb 4, 2018

'Color' and 'colour' are both correct. As long as a consistent spelling is used, it's all okay.

@kurogetsusai

This comment has been minimized.

Owner

kurogetsusai commented Feb 4, 2018

I know, I just said that in response to claiming tabs are "the work of the devil". I noticed @smithfred changed all occurrences of "color" to "colour", as well as all tabs to spaces, so I thought that was pretty funny. :D

@rafaelmardojai

This comment has been minimized.

Contributor

rafaelmardojai commented Feb 4, 2018

I am currently using @smithfred 3.26 theme since i was too busy to finish mine xD. And works properly.

Pd: Github Organizations can have multiple owners =v.
Pd2: Please don't remove tabs... legions of spaces everywhere are the real devil... i will pray for it ='u.

@smithfred

This comment has been minimized.

smithfred commented Feb 4, 2018

Tabs are "evil" only...

I called them "work of the devil" winking at the fact that tabs vs. spaces is an intractable holy war, don't take it too seriously. Reality is they both have advantages/disadvantages.

Besides, I noticed you misspelled "color" as "colour" in your 3.26 themes

It's OK, I no Americans kan't spel. I won't hold it against you ;)

Wait, you made the 3.26 dark theme from 3.18 dark instead of the 3.26 which was already available in my repo?

No, I made 3.26 light from 3.18 light, then 3.26 dark from my 3.26 light since the 3.26 dark was broken for me.

so I trust your version is accurate too.

Not pixel-perfect but much more accurate than the other one on Fedora/FF57.

There really is no SPOF

Just in terms of momentum

4 people already forked it

Yep and did sod-all after they did ;)

GitHub teams don't really help

See other issue

I thought you didn't like the GitHub monoculture. >:D

Yep, more of Satan's work. Repent!

@kurogetsusai

This comment has been minimized.

Owner

kurogetsusai commented Feb 4, 2018

Ok, I'll use @smithfred's version of that theme then, thanks.

I actually tested GitHub Organizations a while ago, the repo owner was still just a single person and others were Collaborators (push access), but that's exactly the same you can do with regular repos. Not sure if they changed it tho. But that's kinda off-topic, I'll focus on merging all that code now.

kurogetsusai added a commit that referenced this issue Feb 7, 2018

kurogetsusai added a commit that referenced this issue Feb 7, 2018

@smithfred

This comment has been minimized.

smithfred commented Feb 15, 2018

Just tested Firefox 58 with the current version of the 3.26 light theme in this project without my fixes; quite a few things wrong vs. before your recent commits with my layer added; some is stuff I fixed a couple of weeks ago on my layer for FF58 (see #22); some not:

Fixed by importing my firefox-58-fixes.css after your main userChrome:

  • Inactive button borders wrong
  • Too-thick border just above content

Not fixed by importing that file:

  • Button background gradients wrong (with or without my FF58 fixes applied)
  • URL bar text shifts on focus (with or without my FF58 fixes applied)
  • Border to left of CSD window controls too high
  • Popover menus too high
  • Hovered tab upper animated border
  • Tab close button, window close button very light grey instead of black
  • Hovered tab close button completely wrong
  • Toolbar button icons don't have native-looking drop shadows, and are too grey (should be more black).

Also TBH the toolbar looks much nicer with GNOME's native icons as available in my additions (reload, new tab, bookmarks, overflow, hamburger) ;)

kurogetsusai added a commit that referenced this issue Feb 24, 2018

kurogetsusai added a commit that referenced this issue Feb 24, 2018

kurogetsusai added a commit that referenced this issue Feb 24, 2018

@kurogetsusai

This comment has been minimized.

Owner

kurogetsusai commented Feb 24, 2018

Just merged your theme.css overrides, except the part with the icon filter hack, since I don't understand your changes, and I changes a few minor things too. This is probably the final version for FF57, I'll edit README later, maybe apply some quick bugfixes if I find some and then I'll create a firefox-57 tag or something like that, so we can forget about FF 57 and merge those fixes for FF 58.

@smithfred

This comment has been minimized.

smithfred commented Feb 26, 2018

Which "icon filter hack" bit(s) specifically?

Haven't tested since your latest commits; I'll test once you move on to a current Firefox version ;)

@kurogetsusai

This comment has been minimized.

Owner

kurogetsusai commented Mar 31, 2018

Which "icon filter hack" bit(s) specifically?

All the stuff related to inverting icon colors you changed. But nevermind, I updated the repo to FF 58 and then realized I'm late again coz there already id FF 59... I think I fixed all of those bugs on your list except for the popup background color (they don't seem to react to my styling attempts) and CSD stuff (looks like Fedora removed FF 58 from their repos?), so I'll just ignore it, tag 58 in like 2 days and move on to 59. I've got more free time now so I want to make this repo up to date again.

I'm finally closing this since there probably won't be more fixes for FF 58 anyway.

@smithfred

This comment has been minimized.

smithfred commented Apr 4, 2018

I'll try and re-test your latest commits when I get a chance then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment