eoger / tabcenter-redux

Vertical Tabs extension for Firefox
Mozilla Public License 2.0
381 stars 67 forks source link

Change close button appearence to be closer to native #328

Closed ariasuni closed 6 years ago

ariasuni commented 6 years ago

fix #270 fix #350

Close button is smaller, closer to the right of the sidebar and less rounded.

Before: screenshot_20180331_003536

After: screenshot_20180331_003551

eoger commented 6 years ago

This looks like a good endeavor to pursue, I think we might as well go all the way and use Firefox's .svg and measurements: https://searchfox.org/mozilla-central/rev/42b34ba1961e37e0d2236deafdd126a0ba21b9ec/toolkit/themes/shared/close-icon.inc.css#8

ariasuni commented 6 years ago

I updated Photon icons from here. Note that now, the close icon is black as opposed to some shade of gray in Firefox, but since I do not know well either the CSS of Tab Center Redux nor the CSS properties involved, I’ll probably let that to another person.

ariasuni commented 6 years ago

I re-did my work to match current version of Tab Center Redux. It seems to me that the current appearance of the close button matches the rest of the extension, while being reasonably similar to the Firefox tab bar.

eoger commented 6 years ago

Apologies for the delay, I've tried the PR and it's looking good. However, I think the color of the X should match the one present in the native Firefox tabs.

ariasuni commented 6 years ago

I tried to use the same code that in the link of your previous comment, then made a few searches and tried different ways to change the color used to fill the SVG, with no success.

Basically, we can’t do it this way, because we can’t use -moz-content-properties. So I edited the SVG. :woman_shrugging: I changed the fill value of the SVG to the color value used by the light Firefox theme.

Note that in the future, improving the icons for dark theme will simply be a matter of copying each icon and changing the fill attribute of the SVG, which is very easy and doesn’t bloat the extension since the SVGs are so tiny.

ariasuni commented 6 years ago

Are you satisfied with the proposed changes?

eoger commented 6 years ago

Sorry I'm currently in a place with no computer, I'll try to get back to you ASAP.We use a hack to get "fill" working on svgs, search for "filter" in the tabcenter.css

On Sat, May 12, 2018, at 2:29 PM, Mélanie Chauvel (ariasuni) wrote:

Are you satisfied with the proposed changes?

Keith94 commented 6 years ago

One flaw I should mention is how the close button retains the :active background color even though it is not being hovered. For example, click and hold on the close button and move your cursor to the left on the tab. At this point, the background should turn transparent while not being hovered, but it stays the same.

ariasuni commented 6 years ago

@Keith94 Right, I noticed that Tab Center Redux’s behavior was slightly different than Firefox native one. So in the last commit, I changed the CSS to match Firefox’s behavior in this regards.

Keith94 commented 6 years ago

Nicely done! Works well for me, but I wasn't sure if that .tab-close:hover:hover:active is a typo or not (does it need two :hover?)

ariasuni commented 6 years ago

Oops, it’s a mistake. I’ll fix that.

Keith94 commented 6 years ago

@ariasuni Since you are updating some photon icons, do you mind looking at #350?

Keith94 commented 6 years ago

As well, the native tab close button has a transparent background if it's not hovered. Perhaps Tab Center should reflect that, too.

ariasuni commented 6 years ago

I got the icon from Firefox’s source code, because it’s not on Photon icons website. I modified tab.js to add or remove a class — .default-favicon — so that I can change default favicon’s color without affecting other favicons.

I removed background-size when only tab is hovered, so that tab close icon has a background only when the icon is hovered, to match Firefox’s behavior. @eoger, tell me if you’re okay with this change, otherwise I’ll revert it.

screenshot_20180519_150400_croppedscreenshot_20180519_150416_cropped screenshot_20180519_150437_croppedscreenshot_20180519_150446_cropped

Keith94 commented 6 years ago

Nice, I appreciate all your efforts! I just noticed a couple issues.

In your screenshot, about:debugging should be using the default favicon. As well, the default favicon itself looks grayish, but elsewhere it looks black (or white depending on theme).

firefox_2018-05-19_09-54-23

ariasuni commented 6 years ago

I simplified and fixed my code so that at first load, about:debugging favicon is displayed like before.

I removed the CSS that made the default favicon theme dependent, since it’s always loaded on a white background and to match how other about: pages favicons are displayed.

Keith94 commented 6 years ago

Thanks!

I removed the CSS that made the default favicon theme dependent, since it’s always loaded on a white background and to match how other about: pages favicons are displayed.

There is one issue in that the favicon is hard to see against the dark theme background; however, any dark favicons all share this same problem... not sure if there's a good solution.

ariasuni commented 6 years ago

I fixed it. It adds a fill-color class to trigger color filling, when favicon is default favicon or is internal to the navigator (chrome:// urls) and finish by .svg (to avoid recoloring .png), otherwise removes it.

ariasuni commented 6 years ago

It should be all good now!

Keith94 commented 6 years ago

This is how my search bar looks compared to the bookmarks sidebar one on Windows 10.

firefox_2018-05-23_16-24-06 firefox_2018-05-23_16-23-03

It could use some minor polish (e.g. no italics) but otherwise looks good.

ariasuni commented 6 years ago

The icon could be tweaked a bit, but it seems the intent was to make it look like the URL bar, not like the native search field used in other panes. We could probably remove italic for Windows for consistency since it’s not used in others bars, though.

Also, we don’t want this pull request to become too big, at least I don’t. So, let’s focus on tabs (favicons, close buttons) for this one.

eoger commented 6 years ago

Merged in https://github.com/eoger/tabcenter-redux/commit/b99e90507b787be11320f2943bde5ed24ab0f0b1. Thank you for your patience and dedication! What we are experiencing here with the search box is probably platform differences (looks fine to me on macOS), we should address this in another PR.