fix(button): properly show focus effect #9826

devversion · 2016-10-12T17:34:22Z

Only apply the focus effect for keyboard interaction (as same as it does currently, but not with a random timeout).

Similar to #9827

Fixes #8749. References #7963

devversion · 2016-10-12T17:35:00Z

src/components/button/button.js

-      .on('blur', function(ev) {
+      });
+
+      element.on('blur', function() {


Note: This is intentionally another on statement, because otherwise the indention is ugly and this is just more clear..

clshortfuse · 2016-10-13T14:30:24Z

src/components/button/button.js

+
+      element.on('focus', function() {
+        // Only show the focus effect when being focused through keyboard interaction.
+        if ($mdInteraction.getLastInteractionType() === 'keyboard') {


Wouldn't this change the existing interaction if a developer focuses an element over Javascript (such as when opening a dialog or a custom component)?

I think if you want to target operations related to the mouse, you should target those events specifically. Tracking the element's last interaction seems to better than tracking to body, since a click event somewhere else on a page would prevent a button from getting a md-focused class (again like a custom popup).

This does not change anything to the existing behavior, just that we now check the global interaction to detect the last interaction.

Focusing manually per .focus does not work right now as well, because the mousedown event didn't get fired.

This is the approach we wanted to go with the $mdInteraction service, as proposed by Marcy. Similar to #9827

This does not change anything to the existing behavior, just that we now check the global interaction to detect the last interaction.

You can see the behavior change with this codepen: http://codepen.io/shortfuse/pen/LRAarO

I just some testing and mousedown gets fired after focus on Chrome. Instead of tracking mouseactive as a boolean, I would suggest a timestamp on last mousedown event. Then attach a timeout to the adding of md-focused. If mousedown was triggered within a certain time (say 100ms), don't add the class.

Edit: You also have to cancel said timeout from 'blur'

crisbeto · 2016-10-14T18:23:13Z

src/components/checkbox/checkbox.js

@@ -54,7 +54,7 @@ angular
 * </hljs>
 *
 */
-function MdCheckboxDirective(inputDirective, $mdAria, $mdConstant, $mdTheming, $mdUtil, $timeout) {
+function MdCheckboxDirective(inputDirective, $mdAria, $mdConstant, $mdTheming, $mdUtil, $timeout, $mdInteraction) {


Seems like the $mdInteraction import is unused.

crisbeto · 2016-10-14T18:23:37Z

src/core/services/interaction/interaction.js

+ */
+MdInteractionService.prototype.isProgrammatic = function(checkDelay) {
+  var delay = angular.isDefined(checkDelay) ? checkDelay : 15;


angular.isNumber might be more appropriate here.

crisbeto · 2016-10-14T18:26:01Z

src/core/services/interaction/interaction.js

+ * @returns {boolean} Whether there was any interaction or not.
+ */
+MdInteractionService.prototype.isProgrammatic = function(checkDelay) {


I know that we decided on the naming for this together, but I'm still not sure that isProgrammatic is very obvious. It seems like it implies that the service is programmatic. That being said, I don't have any better suggestions at the moment, perhaps other team member can pitch in?

Yeah, Carlos had a great idea here. - isUserInvoked.

crisbeto

LGTM.

crisbeto · 2016-10-14T18:58:27Z

src/components/button/button.spec.js

+
+    button.triggerHandler('focus');
+
+    expect(button[0]).toHaveClass('md-focused');


AFAIK you don't have to fetch the DOM node here. expect(button).toHaveClass('md-focused') should work as well.

Valid and changed it. But FYI: a lot of other tests are doing the same and I don't want to diff the whole test now.

That's why I didn't make it a blocking request. It's basically the same, it just looks nicer.

Yeah, changed it for the tests I've added.

* Only apply the focus effect for programmatic focus or by keyboard interaction (as same as it does currently, but not with a random timeout). References angular#7963 Fixes angular#8749

devversion added the needs: review This PR is waiting on review from the team label Oct 12, 2016

devversion assigned ThomasBurleson Oct 12, 2016

devversion commented Oct 12, 2016

View reviewed changes

devversion force-pushed the fix/button-focus-effect branch from b3d2f2d to 984d61b Compare October 12, 2016 17:36

devversion changed the title ~~fix(button): only apply focus effect for keyboard interaction.~~ fix(button): properly show focus effect Oct 12, 2016

clshortfuse reviewed Oct 13, 2016

View reviewed changes

devversion added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Oct 13, 2016

devversion force-pushed the fix/button-focus-effect branch from 984d61b to c77660e Compare October 14, 2016 17:46

devversion removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Oct 14, 2016

devversion assigned crisbeto Oct 14, 2016

devversion force-pushed the fix/button-focus-effect branch 2 times, most recently from c0629cd to 6774229 Compare October 14, 2016 18:11

crisbeto suggested changes Oct 14, 2016

View reviewed changes

devversion force-pushed the fix/button-focus-effect branch 2 times, most recently from 4463449 to 7bcc991 Compare October 14, 2016 18:50

crisbeto approved these changes Oct 14, 2016

View reviewed changes

fix(button): only apply focus effect for keyboard interaction.

1630d42

* Only apply the focus effect for programmatic focus or by keyboard interaction (as same as it does currently, but not with a random timeout). References angular#7963 Fixes angular#8749

devversion force-pushed the fix/button-focus-effect branch from 7bcc991 to 1630d42 Compare October 14, 2016 19:02

ThomasBurleson added needs: presubmit and removed needs: review This PR is waiting on review from the team labels Oct 14, 2016

jelbourn merged commit 34823ac into angular:master Oct 24, 2016

devversion deleted the fix/button-focus-effect branch October 24, 2016 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(button): properly show focus effect #9826

fix(button): properly show focus effect #9826

Uh oh!

devversion commented Oct 12, 2016 •

edited

Loading

Uh oh!

devversion Oct 12, 2016 •

edited

Loading

Uh oh!

clshortfuse Oct 13, 2016

Uh oh!

clshortfuse Oct 13, 2016 •

edited

Loading

Uh oh!

devversion Oct 13, 2016

Uh oh!

clshortfuse Oct 13, 2016

Uh oh!

clshortfuse Oct 13, 2016 •

edited

Loading

Uh oh!

crisbeto Oct 14, 2016

Uh oh!

crisbeto Oct 14, 2016

Uh oh!

crisbeto Oct 14, 2016

Uh oh!

devversion Oct 14, 2016

Uh oh!

crisbeto left a comment

Uh oh!

crisbeto Oct 14, 2016

Uh oh!

devversion Oct 14, 2016 •

edited

Loading

Uh oh!

crisbeto Oct 14, 2016

Uh oh!

devversion Oct 14, 2016

Uh oh!

Uh oh!


		button.triggerHandler('focus');

		expect(button[0]).toHaveClass('md-focused');

fix(button): properly show focus effect #9826

fix(button): properly show focus effect #9826

Uh oh!

Conversation

devversion commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devversion Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clshortfuse Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clshortfuse Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devversion commented Oct 12, 2016 •

edited

Loading

devversion Oct 12, 2016 •

edited

Loading

clshortfuse Oct 13, 2016 •

edited

Loading

clshortfuse Oct 13, 2016 •

edited

Loading

devversion Oct 14, 2016 •

edited

Loading