search

daattali / beautiful-jekyll

✨ Build a beautiful and simple website in literally minutes. Demo at https://beautifuljekyll.com
https://beautifuljekyll.com
MIT License
5.4k stars 16.27k forks source link

.navbar-toggle:focus color persistence after hidden.bs.collapse #268

Closed jennydaman closed 6 years ago

jennydaman commented 7 years ago

Addresses the issue mentioned here: https://github.com/daattali/beautiful-jekyll/pull/256#issuecomment-337092241

screenshot from 2017-10-16 21-28-33

I noticed some awkward attribute stuff going on. Bootstrap is probably to blame (though I did not review its 6567 + 2307 SLOC).

  1. Assuming the screen width is less than 760px, navbar-toggle will be shown.
  2. Clicking on navbar-toggle once will expand navlinks-children (JS event: show.bs.collapse). A new HTML attribute is initialized, aria-expanded="true"
  3. Clicking on navbar-toggle a second time will hide navlinks-children (JS event: hidden.bs.collapse). aria-expanded="false", and a new class is added as well: collapsed

I tried to fix the bug: https://github.com/daattali/beautiful-jekyll/compare/master...jennydaman:fix-navbar-toggle

How my patch works is that, when the collapsed class is introduced, the new CSS rule for button.navbar-toggle.collapsed:hover takes priority.

The solution works on desktops, even though it's essentially duct tape over a crack.

However, this patch does not work on mobile/touchscreen devices, defeating the whole purpose. This is what I think is happening. When a user taps on navbar-toggle to hide the navlinks-children, their virtual pointer lingers on the button so the :hover state persists.

I tried to cover this up using JavaScript in js/main.js, but it didn't work. (jquery-1.11.2.min.js is very outdated. Might be an issue?)

I predict a functional fix for this bug on touchscreen devices would require impractical workarounds or direct modification of third party scripts. Might be best to just ignore it too, or to delete rules about :hover altogether for the sake of consistency... The perfectionist in me cringes.

(I'll worry about pull request #256 after this issue is resolved.)

daattali commented 7 years ago

Wow I never realied bootstrap's navbar toggle feature was this awkward. Sorry you have to go through this! If it doesn't fix it on mobile, then I wouldn't want to merge it. The perfectionist in me just can't do that :)

This bug with the navbar toggle focus presisting doesn't actually have anything to do with your PR though, does it? Your PR changes the text colour of navbar, and the hover colour of links. This navbar toggle button is not a hyperlink and isn't affected. It's a separate issue. I think you can still continue with your original PR as it doesn't change anything about that bug. Am I mistaken?

jennydaman commented 7 years ago

I intend on making another commit to my PR so that the navbar-toggle button changes color to the link hover color.

Depending on your decision about this issue, then how I'd go about handling that would be different. I'll just leave this issue alone for now and make one last commit tomorrow about the navbar-toggle button color.

I'll do that tomorrow.

daattali commented 7 years ago

I wouldn't want a fix that only fixed desktops but not smaller screens. I'd prefer to have broken functionality on both :p

jennydaman commented 7 years ago

Found a solution while working on my own site! I pushed a commit to https://github.com/jennydaman/beautiful-jekyll/tree/colors that fixes this issue (related PR: #256). Color works on touchscreen and mobile devices.

Though it's probably bad practice to be fixing a bug in a mostly unrelated PR...

daattali commented 7 years ago

Awesome! Could you actually submit a separate PR that just fixes this issue please?

daattali commented 6 years ago

@jennydaman let me know if there's a PR for this please

jennydaman commented 6 years ago

Nope... I don't have a perfect solution that works on all platforms, just silly workarounds.

If your goal is consistency, I advise that you get rid of any :hover and :focus effects on the element, and keep the darker color when the menu is expanded. That is the quickest "fix." (my opinion is that this is reasonable, shading on hover isn't an important feature. The bug negates any aesthetic value the hover shading tries to add.)

To achieve the optimal solution, someone would have to completely rewrite how the bootstrap classes are being used. I don't have the time to read all that documentation right now... (might come back and look at this, but like in a few months)

jennydaman commented 6 years ago

Let me get this clear, the ideal rules for color would be:

The issue comes from how touchscreens manage:hover. When tapping somewhere, the virtual pointer clicks once, then hovers over that point until somewhere else is clicked.

button:hover {
    background-color: blue;
}

The code segment above works as expected on small, non-touchscreen devices. However, the button will stay blue on touchscreens if the button is tapped.

What I want to achieve is for :hover rules to be ignored on small touchscreens. That's not possible with vanilla CSS (@media only screen and (max-width: 768px)).

https://v4-alpha.getbootstrap.com/components/collapse/

Bootstrap's demo has its color linger on touchscreen collapse events, too (because :hover). What they did right (that's still wrong with beautiful-jekyll:master) is that at least the color won't linger on mouse-pointer desktop screens.

How would you want this fixed? We could... A. keep it as it is (color lingers on touchscreen and desktop) B. get rid of :hover rules (consistent on all platforms) C. get rid of :focus rules, add rules for aria-expanded (fix for desktop, color lingers on touchscreen) D. correct usage of bootstrap in _includes/*.html (same result as C. cleaner code, but this solution involves the reading of pages of doc :unamused:)

Edit: option D seems infeasible, bootstrap v3.3.2 documentation makes me miserable.

daattali commented 6 years ago

I didn't notice your edit on the last comment. Is there anything else you'd like to address or are we ok now after discussing this in the PR?

jennydaman commented 6 years ago

I have a bad habit of editing comments, whatever. I'm going to work on #268 (colors). The commit history is messy, I'm going to start with a new branch...

daattali commented 6 years ago

Sounds good. I do find that after more than 5-10 minutes it's safer to just add a new comment, even though it feels spammy

Dean Attali President & CEO AttaliTech Ltd http://AttaliTech.com http://attalitech.com

On 6 January 2018 at 18:02, Jennings Zhang notifications@github.com wrote:

I have a bad habit of editing comments, whatever. I'm going to work on

268 https://github.com/daattali/beautiful-jekyll/issues/268 (colors).

The commit history is messy, I'm going to start with a new branch...

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/daattali/beautiful-jekyll/issues/268#issuecomment-355784719, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6IFHysZgGaoNpoETAAli2-qBfZw5fkks5tH_uegaJpZM4P9Ck4 .