Bug fixes #988

Merged
merged 4 commits into from Apr 26, 2016

Conversation

Projects
None yet
2 participants
@mfairchild365
Member

mfairchild365 commented Apr 19, 2016

No description provided.

+.wdn-dropdown-widget-label.focused .wdn-icon-share {
+ /* firefox */
+ outline: 1px dotted #212121;
+ /* -webkit-focus-ring-color = '#5B9DD9' */

This comment has been minimized.

@kabel

kabel Apr 19, 2016

Contributor

Can you explain this?

@kabel

kabel Apr 19, 2016

Contributor

Can you explain this?

This comment has been minimized.

@mfairchild365

mfairchild365 Apr 19, 2016

Member

what about it? The icon is absolutely positioned, not the label (which usually gets the outline). So, when the control was focused, the label was being outlined but had no width (resulting in a line in some browsers, and no outline in others). This simply tries to simulate the default outline styles in firefox and webkit based browsers.

This all stems from using the 'pure css' dropdown pattern, which utilizes a checkbox to track the visibility state. Browsers don't give focus to labels, so in order to provide a visual signal that you are current focused on something, we have to do this weird hack.

I'm all for ditching the 'pure css' dropdown pattern we have going on.

@mfairchild365

mfairchild365 Apr 19, 2016

Member

what about it? The icon is absolutely positioned, not the label (which usually gets the outline). So, when the control was focused, the label was being outlined but had no width (resulting in a line in some browsers, and no outline in others). This simply tries to simulate the default outline styles in firefox and webkit based browsers.

This all stems from using the 'pure css' dropdown pattern, which utilizes a checkbox to track the visibility state. Browsers don't give focus to labels, so in order to provide a visual signal that you are current focused on something, we have to do this weird hack.

I'm all for ditching the 'pure css' dropdown pattern we have going on.

This comment has been minimized.

@kabel

kabel Apr 19, 2016

Contributor

I meant explain what this line comment is for and why webkit hacks are necessary. Focus-like styles make perfect sense.

But it you want to try for getting rid of the label/checkbox approach. I'm all for it. That approach was to support javascript disabled browsers. And that simply doesn't make sense for the social share that almost certainly requires JS.

@kabel

kabel Apr 19, 2016

Contributor

I meant explain what this line comment is for and why webkit hacks are necessary. Focus-like styles make perfect sense.

But it you want to try for getting rid of the label/checkbox approach. I'm all for it. That approach was to support javascript disabled browsers. And that simply doesn't make sense for the social share that almost certainly requires JS.

This comment has been minimized.

@mfairchild365

mfairchild365 Apr 19, 2016

Member

Ah, webkit has a different default outline color/style. This was an attempt to simulate it if it existed. I will be honest, this was a copy/paste from stack overflow. The comment is simply stating what the value of -webkit-focus-ring-color should be. If you want, the comment can be removed as it isn't super helpful.

@mfairchild365

mfairchild365 Apr 19, 2016

Member

Ah, webkit has a different default outline color/style. This was an attempt to simulate it if it existed. I will be honest, this was a copy/paste from stack overflow. The comment is simply stating what the value of -webkit-focus-ring-color should be. If you want, the comment can be removed as it isn't super helpful.

wdn/templates_4.1/scripts/navigation.js
@@ -665,6 +665,10 @@ define(['jquery', 'wdn', 'modernizr', 'require'], function($, WDN, Modernizr, re
switchSiteNavigation($homepageCrumbLink[0], false);
});
+ $(navSel + ' a').last().focusout(function(){

This comment has been minimized.

@kabel

kabel Apr 19, 2016

Contributor

This will probably work for a majority of the cases, but it will fail with the navigation switcher (and probably the other event listeners that are bound the individual elements). It could be made better by binding the event to a "safe" parent element and only responding when the trigger matches the expectation. Make sense?

@kabel

kabel Apr 19, 2016

Contributor

This will probably work for a majority of the cases, but it will fail with the navigation switcher (and probably the other event listeners that are bound the individual elements). It could be made better by binding the event to a "safe" parent element and only responding when the trigger matches the expectation. Make sense?

This comment has been minimized.

@mfairchild365

mfairchild365 Apr 19, 2016

Member

Gotcha. It will fail when the navigation switcher puts new navigation in the navigation container, thus dropping the old nav and the listener on the <a>.

So you are saying add a listener to the parent nav container and listen to all focusout events, when the trigger is the last <a>, close the nav.

That makes sense, but it seems like a lot of overhead. I can't think of a better way of doing it.

@mfairchild365

mfairchild365 Apr 19, 2016

Member

Gotcha. It will fail when the navigation switcher puts new navigation in the navigation container, thus dropping the old nav and the listener on the <a>.

So you are saying add a listener to the parent nav container and listen to all focusout events, when the trigger is the last <a>, close the nav.

That makes sense, but it seems like a lot of overhead. I can't think of a better way of doing it.

This comment has been minimized.

@kabel

kabel Apr 19, 2016

Contributor

It's a trivial amount of overhead; events are always bubbling and the more events we can condense into a single element reduces the memory footprint.

@kabel

kabel Apr 19, 2016

Contributor

It's a trivial amount of overhead; events are always bubbling and the more events we can condense into a single element reduces the memory footprint.

@kabel kabel merged commit 0d676c2 into unl:develop Apr 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mfairchild365 mfairchild365 deleted the mfairchild365:bug-fixes branch May 18, 2016

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