Bug 26856 – AnchorElement, ButtonElement, InputButton and Document should fire focus event when it is clicked.

• Component: DOM
• Status: RESOLVED DUPLICATE

[Hedger Wang]

When click on a <input type="button" />, <button /> or an <a href="..." />, 
the browser should fire the focusin event since these elements does get focus.
If no elements found to be focusable, then either the Document or the BODY should
fire the focusin event.

This is working on Firefox and IE, but not on WebKit browsers.

[Chang Shu]

Created attachment 40659 [details]
patch

[Erik Arvidsson]

Comment on attachment 40659 [details]
patch

Although this patch gives the right behavior I think we need to be a bit careful here.

With this patch links gets the focus outline when the user clicks them. This is correct when it comes to CSS because we have something like :link:focus { outline: ...}. However, I doubt that we want this heavy visual clutter on links by default.

You can remove isMouseFocusable from HTMLAnchorElement now since all it does is call super.

Also, you need to include a layout test with your patch.

[Chang Shu]

(In reply to comment #2)

Thanks for the review. I put some of my thoughts below.

> (From update of attachment 40659 [details])
> Although this patch gives the right behavior I think we need to be a bit
> careful here.
> 
> With this patch links gets the focus outline when the user clicks them. This is
> correct when it comes to CSS because we have something like :link:focus {
> outline: ...}. However, I doubt that we want this heavy visual clutter on links
> by default.

As it affects the click behavior not the hover behavior, the performance degrade is probably ok. Other browsers, such as Firefox, also support this.

> 
> You can remove isMouseFocusable from HTMLAnchorElement now since all it does is
> call super.

Yes, will do.

> 
> Also, you need to include a layout test with your patch.

I will work on that.

[Chang Shu]

Created attachment 40824 [details]
patch with test case

[Adele Peterson]

Comment on attachment 40824 [details]
patch with test case

I don't think we want this behavior on the Mac.  Let me find a related bug with more explanation.

[Erik Arvidsson]

(In reply to comment #5)
> (From update of attachment 40824 [details])
> I don't think we want this behavior on the Mac.  Let me find a related bug with
> more explanation.

This actually fixes web compat issues.

I think the main problem is the visual clutter the focus outline introduces

[Adele Peterson]

This is very similar to https://bugs.webkit.org/show_bug.cgi?id=22261. You should read some of the discussion there.  This particular patch addresses the issue described in https://bugs.webkit.org/show_bug.cgi?id=18425.  We should probably dupe some of these bugs.

[Alexey Proskuryakov]

> This actually fixes web compat issues.

What are the sites that get fixed?

I agree with Adele that we have way too many bugs tracking this.

[Chang Shu]

Yes, they are very similar bugs and similar fixes.
With all this in mind, I am thinking if we can do
#if PLATFORM(MAC)
    if (isLink())
        return false;
#endif
    // Allow tab index etc to control focus.
    return HTMLElement::isMouseFocusable();

However, personally, I don't feel drawing the outline is a big performance overhead. The drawing happens only if the user click the link. Comparing to the time spent on human interaction, the drawing is negligible. On the other hand, it could even be a positive thing: the user gets feedback on what he/she clicked and even is able to stop the load if the link is accidentally clicked.
Well, this is just my two cents.

[Chang Shu]

It seems this bug 26856 is a superbug of 22261 and 18425. My patch fixes 18425 only. And Viatcheslav Ostapenko's patch fixes 22261.

[Erik Arvidsson]

I'm not concerned about performance. My concern is about the visual clutter.

All other browsers allows the user to use the mouse to focus anchor elements, buttons and inputs. The difference is they have a more subtle focus outline than WebKit does so it is not so visually distracting.

I think it is clear we should do this on all platforms. What we might want to do is to come up with a solution where it does not show the focused state when the user uses the mouse to focus an element. IE7 used to do this but it wasn't compatible with CSS so they always show the focus state in IE8, no matter how the element received focus.

[Chang Shu]

(In reply to comment #11)
> I'm not concerned about performance. My concern is about the visual clutter.
> 
> All other browsers allows the user to use the mouse to focus anchor elements,
> buttons and inputs. The difference is they have a more subtle focus outline
> than WebKit does so it is not so visually distracting.
> 
> I think it is clear we should do this on all platforms. What we might want to
> do is to come up with a solution where it does not show the focused state when
> the user uses the mouse to focus an element. IE7 used to do this but it wasn't
> compatible with CSS so they always show the focus state in IE8, no matter how
> the element received focus.

I thought it was the performance caused by visual clutter. Understood. So what we want to come up is not to render/paint the visual effect when mouse-focused but all other things, such as fire events, should still work.

[Chang Shu]

Created attachment 41053 [details]
apply change only to non-Mac platform

[Erik Arvidsson]

Comment on attachment 41053 [details]
apply change only to non-Mac platform

Having FAIL in the platform/mac/ expectation seems bad to me.

[Chang Shu]

(In reply to comment #14)
> (From update of attachment 41053 [details])
> Having FAIL in the platform/mac/ expectation seems bad to me.

As Erik and I discussed through IRC, it seems ok at the moment. The special expectation will be removed once the final solution is provided to Mac.

[Adele Peterson]

Comment on attachment 41053 [details]
apply change only to non-Mac platform

We should keep discussing the Mac behavior.  Maybe we could turn it on as a trial at some point to see how it feels in practice.

[WebKit Commit Bot]

Comment on attachment 41053 [details]
apply change only to non-Mac platform

Clearing flags on attachment: 41053

Committed r49619: <http://trac.webkit.org/changeset/49619>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brian Weinstein]

Updated Windows results were committed in http://trac.webkit.org/changeset/49667 and http://trac.webkit.org/changeset/49640.

[Erik Arvidsson]

I think we need to roll this back until bug 30523 is fixed. If rollback is not acceptable this needs to be exposed in the webkit API.

[Erik Arvidsson]

Created attachment 41971 [details]
Make links mouse focusable only on GTK and QT only

[Chang Shu]

(In reply to comment #21)
> Created an attachment (id=41971) [details]
> Make links mouse focusable only on GTK and QT only

The test case change in this patch will break Qt, won't it?

[Erik Arvidsson]

(In reply to comment #22)
> (In reply to comment #21)
> > Created an attachment (id=41971) [details] [details]
> > Make links mouse focusable only on GTK and QT only
> 
> The test case change in this patch will break Qt, won't it?

Yup, adding a new expectation to platform/qt/

[Erik Arvidsson]

Created attachment 41977 [details]
Updated test expectations

[Erik Arvidsson]

Created attachment 41978 [details]
Updated test expectations

[Eric Seidel (no email)]

I'm confused by the result output being:
PASS
FAIL

Is this test intended to partially fail?

[Erik Arvidsson]

Created attachment 41991 [details]
Added GTK and QT specific tests

[Eric Seidel (no email)]

Comment on attachment 41991 [details]
Added GTK and QT specific tests

Lets make this a single test, and check in separate results for the separate platforms, as you original suggested.

LGTM.  Fix it as you land.

[Erik Arvidsson]

Created attachment 42149 [details]
Changed test to not say PASS/FAIL and only make the expectations platform specific

[Eric Seidel (no email)]

Comment on attachment 42149 [details]
Changed test to not say PASS/FAIL and only make the expectations platform specific

ChangeLog is wrong, otherwise this looks fine.

[Erik Arvidsson]

Created attachment 42159 [details]
Fixed issues with tabIndex and contentEditable and expanded the tests to cover these

[Eric Seidel (no email)]

Comment on attachment 42159 [details]
Fixed issues with tabIndex and contentEditable and expanded the tests to cover these

It's a bit strange to see this in the results:
 Anchor with tab index
 6 
 7 Anchor without tab index
 8 
 9 Link without tab index
 10 
 11 Link with tab index
 12 
 13 Link with contentEditable
 14 
 15 Link inside contentEditable

it's just noise.  For bonus points you could wrap it in a div and hwne running under DRT remove it at the end of the test.

 74         // Make sure that links with tabIndex or contentEditable are still focusable.

Maybe something more like: // Only allow links with a tabIndex or contentEditable to be mouse-focusuable.

the "make sure" comment only makes sense given the previous code. :)

In general this looks fine though.

[Erik Arvidsson]

Created attachment 42163 [details]
Took care of Eric's comments

[Eric Seidel (no email)]

Comment on attachment 42163 [details]
Took care of Eric's comments

Works for me.

[WebKit Commit Bot]

Comment on attachment 42163 [details]
Took care of Eric's comments

Clearing flags on attachment: 42163

Committed r50315: <http://trac.webkit.org/changeset/50315>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Erik Arvidsson]

This is only working as expected on GTK and QT. We need to add support for a -webkit-focusring pseudo class (bug 30523) before we can make links mouse focusable again.

[Adele Peterson]

With r50315, any time you cmd-click on someone's profile pic on Facebook, you focus the link around the photo.  Looks pretty terrible.

[Erik Arvidsson]

Facebook is now setting the tabindex to -1 on the anchor element which makes it
mouse focusable so they are getting what they are asking for. Seriously though,
I think bug 30523 is the only sane way forward.

[Beth Dakin]

The same change caused this regression:

https://bugs.webkit.org/show_bug.cgi?id=38548

[Adele Peterson]

It doesn't seem practical to rely on developers adopting a new pseudo class in the future to fix the problem.   Is there some intermediate step we should take to fix Facebook and UW in the short term?  Should we consider rolling this change out for Mac?

(In reply to comment #39)
> Facebook is now setting the tabindex to -1 on the anchor element which makes it
> mouse focusable so they are getting what they are asking for. Seriously though,
> I think bug 30523 is the only sane way forward.

[Erik Arvidsson]

The UW bug does is not about the look. It is breaking because the link gets focused. Links are mouse focusable in all other browsers so the UW site must be doing some WebKit specific thing to cause this.

I'll dissect the UW site tomorrow and see why they get this behavior.

I think it might be acceptable to make links non mouse focusable (ignoring tabindex) again but only temporary. If the site declares that they want links to be focusable we should respect that.

The issue with the focus ring is different. If we implement focus right correctly it would not require web sites to change anything. The difference is just that we would not show the focus ring on links when the user mouse focuses them, only when they use the keyboard to focus them.

[Beth Dakin]

(In reply to comment #42)
> The UW bug does is not about the look. It is breaking because the link gets
> focused. Links are mouse focusable in all other browsers so the UW site must be
> doing some WebKit specific thing to cause this.

I just updated the UW bug with various reductions and pieces of information that attempt to show that there is not WebKit-specific code in the UW test. Steven and I attached a few test cases that fail in WebKit even though they work in Firefox.

[Alexey Proskuryakov]

See also: bug 26856.

[Alexey Proskuryakov]

Sorry, discussion above already has more and better links to related bugs.

[Alexey Proskuryakov]

*** Bug 55975 has been marked as a duplicate of this bug. ***

[Sam Sneddon [:gsnedders]]

*** Bug 92029 has been marked as a duplicate of this bug. ***

[Kyle Simpson]

Is there a plan to revive this bug and finally address it? I ask because the bug I was part of (about radio buttons) was just marked as a dupe and merged into this one, but here this bug appears dormant for 11 years.

Also, reading this thread, I find it unusual that the original objection was that elements which are focused "look bad" if there is some visual indication they are focused. I'm fairly certain that A11Y guidelines strongly push for elements which are focused to be visually obvious that they are focused. The fact that someone doesn't like what it looks like for a link or radio button to be focused is a CSS styling question, not a functionality question.

When devs remove the `outline` on a focused element (probably at the request of some client or designer) and don't replace it with some other visual indication -- I prefer using an inner box shadow to make an element sort of "glow" -- that omission is pretty universally considered a bad A11Y practice, as far as I can tell.

----

I had long since forgotten that back in 2012 I was participating in that other bug about focus not working on radio buttons, and was only reminded when I saw the email indicating the dupe/merge here.

But I fairly recently had this same problem in a PWA I built.

I created a click handler for all buttons, checkboxes, and radio buttons in my app, that forces the element to be focused, so that mouse clicks or taps cause my CSS :focus styling to be applied in safari the same as it was automatically being applied in other browsers/platforms.

So yeah, this is STILL a bug that code is still having to work-around.

[Alexey Proskuryakov]

Is there any difference between this and bug 22261 at this point? Seems like another dupe to me.

[Ryosuke Niwa]

(In reply to Alexey Proskuryakov from comment #49)
> Is there any difference between this and bug 22261 at this point? Seems like
> another dupe to me.

Pretty much.

[Ryosuke Niwa]

*** This bug has been marked as a duplicate of bug 22261 ***