Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Custom scrollbars for Windows based on win 8 #6305

Merged
merged 3 commits into from
Jan 9, 2014
Merged

Conversation

TomMalbran
Copy link
Contributor

This PR adds custom scrollbars to windows based on the windows 8 scrollbars. It also fixes the gutter filler for Linux.

@marcelgerber
Copy link
Contributor

I'd suggest this content for brackets_scrollbars.less (taken from #5438):

.platform-win {
    ::-webkit-scrollbar {
        width: 15px;
        height: 15px;
        background-color: rgb(240, 240, 240);
    }

    ::-webkit-scrollbar-thumb {
        box-shadow: none;
        border: 1px solid rgb(240, 240, 240);
        background-color: rgb(206, 206, 206);
    }

    :hover::-webkit-scrollbar-thumb,
    :focus::-webkit-scrollbar-thumb {
        background-color: rgb(166, 166, 166);
    }

    :active::-webkit-scrollbar-thumb {
        background-color: rgb(96, 96, 96);
    }

    ::-webkit-scrollbar-track:vertical {
        margin: 0 0 15px 0;
        min-height: 20px;
    }

    ::-webkit-scrollbar-track:horizontal {
        margin: 0 15px 0 0;
        min-width: 20px;
    }

    ::-webkit-scrollbar-corner {
        background-color: transparent;
    }
}

It adds a hover/active effect, just like in Win8. There is a little border as well to differ the scrollbar from the right sidebar. And the width is set to 15px, just as it is in Win8.

@TomMalbran
Copy link
Contributor Author

Woops, I totally forgot about that other PR, but it hasn't been updated in a long time, and this one fixes most of the issues the other one had. So I guess that is ok.

Thanks for the suggestions, but I don't think that we need to replicate the scrollbars, making some based on that win8 style seems good enough to me. The hover state looks good, I didn't notices it before, but I don't really like the extra border or that the bars are so wide.

Maybe @larz0 could give a final say on this?

@marcelgerber
Copy link
Contributor

Look at this Win8 screenshot where you can see the original Win8 scrollbars. They have a little border (or margin, whatever) so you can differ the scrollbar-thumb from the right/bottom sidebar.
It's ok to me to change the width.

EDIT:
I kinda like this:

.platform-win {
    ::-webkit-scrollbar {
        width: 12px;
        height: 12px;
        background-color: rgb(240, 240, 240);
    }

    ::-webkit-scrollbar-thumb {
        box-shadow: none;
        background-color: rgb(206, 206, 206);
    }

    ::-webkit-scrollbar-thumb:vertical {
        border-right: 1px solid rgb(240, 240, 240);
    }

    ::-webkit-scrollbar-thumb:horizontal {
        border-bottom: 1px solid rgb(240, 240, 240);
    }

    :hover::-webkit-scrollbar-thumb,
    :focus::-webkit-scrollbar-thumb {
        background-color: rgb(166, 166, 166);
    }

    :active::-webkit-scrollbar-thumb {
        background-color: rgb(96, 96, 96);
    }

    ::-webkit-scrollbar-track:vertical {
        margin: 1px 0 12px 0;
        min-height: 20px;
    }

    ::-webkit-scrollbar-track:horizontal {
        margin: 0 12px 0 0;
        min-width: 20px;
    }

    ::-webkit-scrollbar-corner {
        background-color: transparent;
    }
}

@TomMalbran
Copy link
Contributor Author

I know how the win8 scrollbars look... I got a Windows 8 machine. I am still not convinced about the border... the rest looks good, although you need to move the hover, focus and active after the pseudoclass so that it acts when you hover over the scrollbar and not when you hover the content.

@ghost ghost assigned larz0 Jan 2, 2014
@larz0
Copy link
Member

larz0 commented Jan 2, 2014

@TomMalbran ahhh you figured it out! Looks good! Review complete.

@@ -39,6 +40,7 @@
}

::-webkit-scrollbar-thumb {
background-color: transparent;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this break anything on other platforms? It seems like this means the thumb will be invisible on Linux and on Macs where the mouse is plugged in, since background-color is only overridden in the .platform-win rules below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see any changes on Linux (Ubuntu):
Scrollbars in Ubuntu

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plugged in a mouse on my 10.8.5 laptop, and the thumb appears fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That line is only applied on windows. So it shouldn't affect any other platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we are using box-shadow to add the background color of the scrollbar. This is done as a "hack" to make it possible to have a margin around the scrollbar. Since I used background colors for the Windows scrollbars I had to overwrite it here with transparent so that that background isn't shown on this bars. But thinking it better, maybe I could just use box-shadow on win too.

@bchintx
Copy link
Contributor

bchintx commented Jan 3, 2014

Done with code review. However, on Windows only, I noticed that the scrollbar thumb does not highlight when you hover the mouse cursor over or drag the thumb. Makes it difficult to see on Windows. Submitted issue #6352.

@marcelgerber
Copy link
Contributor

@bchintx See #6305 (comment) for a Win8-like hover effect.

@bchintx
Copy link
Contributor

bchintx commented Jan 3, 2014

@SAplayer Yup -- that CSS seems to implement the highlighting as I'd expect.

@TomMalbran
Copy link
Contributor Author

Yes. I was waiting for a full review before adding the hover and active effects.

@LarZ is the width ok? Do we need the border?

@marcelgerber
Copy link
Contributor

But @bchintx, on Linux we have no hover effect, too (and we never had).

@bchintx
Copy link
Contributor

bchintx commented Jan 6, 2014

@TomMalbran So far your change looks fine. IMHO, if you can go ahead and add the highlighted effects, it'll be ready to merge.

BTW, @larz0 already posted a comment above that he approved of the changes, but I'll let him respond to your most recent question re: the width and border.

@SAplayer I haven't checked Linux, but Mac and Win both currently have hover effects. (On Mac, I think you need a plugged-in mouse, as @peterflynn already noted above.) Without the highlighting, the scrollbar thumb is very faint, so the extra contrast is very helpful.

@TomMalbran
Copy link
Contributor Author

@bchintx Done. Added the hover and active effects. I switched to box-shadow so I didn't had to change to overwrite the backgrounds in the quiet scrollbars for all the effects.

Should the colors be moved to variables?

Linux doesn't have a hover effect. You can check it in the code, but can be added in the same way as I just added it for windows.

@peterflynn
Copy link
Member

@bchintx Can we close #6352 then? Seems like we should make sure that's fixed before merging this.

@bchintx
Copy link
Contributor

bchintx commented Jan 6, 2014

@TomMalbran Thanks for the hover effects. Yes, actually, they probably should be in variables. How about in 'brackets_theme_default.less'?

@peterflynn Good point. I've confirmed that the latest changes address my concerns about the highlighting.

@larz0
Copy link
Member

larz0 commented Jan 6, 2014

@bchintx @TomMalbran checked it out just then and it's good.

@TomMalbran
Copy link
Contributor Author

@bchintx Added the variables.

I tried to improve the corner when we have both scrollbars, since I didn't liked having a margin when there was just 1 scrollbar. The solution might look ok, but isn't perfect, but the problem has to first be fixed in CodeMirror before we completely fix it here. Since we use custom scrollbars CodeMirror doesn't know the size of it, so it extends the scrollbars to the bottom/right, instead of before the corner. I think that a good solution would be to have a class added somewhere to tell us when there are both scrollbars, so we can use it to add the margin in css when required.

@peterflynn
Copy link
Member

I found an old user story related to this: https://trello.com/c/jhCb7dvV/909-windows-scroll-bars. I think we can move it to the 'Community Done' list once this PR lands! :-)

@peterflynn
Copy link
Member

One other thought: given the UI's sensitivity around scrolling performance, should we do some testing to verify that the effects like box-shadow, etc. don't impact scrolling speed? (We'd probably have to use the high-speed camera for this though).

@bchintx
Copy link
Contributor

bchintx commented Jan 7, 2014

@TomMalbran Thanks for adding the variables. The change looks great.

@peterflynn I've played around with the scrolling performance both with and without this change, and I don't see any noticeable difference in responsiveness or speed as I scroll using the thumb. Was this an issue in the past for a particular system or is there something specific we can do to validate your concern?

@TomMalbran
Copy link
Contributor Author

Thanks. I still don't like that I can't make it stop at the corner when there are both vertical and horizontal scrollbars, but that is more of a problem with CodeMirror.

Box shadows can give bad scroll performance, but I think the main issue is when it is applied to elements and is semi-transparent. Since here is just applied to the scrollbar and opaque, it shouldn't give much problems. I can easily switch this one to background colors, but we can't easily do the same with the linux custom scrollbars since those have a margin around them.

@bchintx
Copy link
Contributor

bchintx commented Jan 8, 2014

@TomMalbran based on @peterflynn's concerns, yes, can you please go ahead and switch to background colors on Windows? Since there's not an easy alternative on Linux, just leave those as is.

@bchintx
Copy link
Contributor

bchintx commented Jan 9, 2014

After further discussion, since there doesn't seem to be a noticeable performance impact, we're ok with merging this now. Thanks for the work, @TomMalbran!

bchintx added a commit that referenced this pull request Jan 9, 2014
Custom scrollbars for Windows based on win 8
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants