Bug 26856 - AnchorElement, ButtonElement, InputButton and Document should fire focus event when it is clicked.
: AnchorElement, ButtonElement, InputButton and Document should fire focus even...
Status: REOPENED
: WebKit
HTML DOM
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
: http://hedgerwow.appspot.com/bug/webk...
: HasReduction
: 30523
:
  Show dependency treegraph
 
Reported: 2009-06-30 13:38 PST by
Modified: 2011-03-09 11:07 PST (History)


Attachments
patch (1.16 KB, patch)
2009-10-05 14:11 PST, Chang Shu
arv: review-
Review Patch | Details | Formatted Diff | Diff
patch with test case (3.54 KB, patch)
2009-10-07 14:46 PST, Chang Shu
no flags Review Patch | Details | Formatted Diff | Diff
apply change only to non-Mac platform (4.04 KB, patch)
2009-10-12 11:37 PST, Chang Shu
no flags Review Patch | Details | Formatted Diff | Diff
Make links mouse focusable only on GTK and QT only (3.92 KB, patch)
2009-10-27 11:55 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Updated test expectations (4.47 KB, patch)
2009-10-27 13:40 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Updated test expectations (4.94 KB, patch)
2009-10-27 13:45 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Added GTK and QT specific tests (8.10 KB, patch)
2009-10-27 15:25 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Changed test to not say PASS/FAIL and only make the expectations platform specific (7.46 KB, patch)
2009-10-29 14:45 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Fixed issues with tabIndex and contentEditable and expanded the tests to cover these (7.56 KB, patch)
2009-10-29 16:10 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Took care of Eric's comments (7.21 KB, patch)
2009-10-29 17:56 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-06-30 13:38:17 PST
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.
------- Comment #1 From 2009-10-05 14:11:08 PST -------
Created an attachment (id=40659) [details]
patch
------- Comment #2 From 2009-10-05 14:47:25 PST -------
(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.

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.
------- Comment #3 From 2009-10-06 06:50:42 PST -------
(In reply to comment #2)

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

> (From update of attachment 40659 [details] [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.
------- Comment #4 From 2009-10-07 14:46:17 PST -------
Created an attachment (id=40824) [details]
patch with test case
------- Comment #5 From 2009-10-07 15:01:34 PST -------
(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.
------- Comment #6 From 2009-10-07 15:09:07 PST -------
(In reply to comment #5)
> (From update of attachment 40824 [details] [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
------- Comment #7 From 2009-10-07 15:56:41 PST -------
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.
------- Comment #8 From 2009-10-07 21:10:19 PST -------
> 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.
------- Comment #9 From 2009-10-08 06:35:34 PST -------
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.
------- Comment #10 From 2009-10-08 06:59:41 PST -------
It seems this bug 26856 is a superbug of 22261 and 18425. My patch fixes 18425 only. And Viatcheslav Ostapenko's patch fixes 22261.
------- Comment #11 From 2009-10-08 09:51:24 PST -------
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.
------- Comment #12 From 2009-10-08 11:44:12 PST -------
(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.
------- Comment #13 From 2009-10-12 11:37:26 PST -------
Created an attachment (id=41053) [details]
apply change only to non-Mac platform
------- Comment #14 From 2009-10-12 12:11:44 PST -------
(From update of attachment 41053 [details])
Having FAIL in the platform/mac/ expectation seems bad to me.
------- Comment #15 From 2009-10-13 11:30:12 PST -------
(In reply to comment #14)
> (From update of attachment 41053 [details] [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.
------- Comment #16 From 2009-10-14 16:17:06 PST -------
(From update of attachment 41053 [details])
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.
------- Comment #17 From 2009-10-15 05:49:37 PST -------
(From update of attachment 41053 [details])
Clearing flags on attachment: 41053

Committed r49619: <http://trac.webkit.org/changeset/49619>
------- Comment #18 From 2009-10-15 05:49:44 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #19 From 2009-10-15 16:46:53 PST -------
Updated Windows results were committed in http://trac.webkit.org/changeset/49667 and http://trac.webkit.org/changeset/49640.
------- Comment #20 From 2009-10-27 10:09:00 PST -------
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.
------- Comment #21 From 2009-10-27 11:55:02 PST -------
Created an attachment (id=41971) [details]
Make links mouse focusable only on GTK and QT only
------- Comment #22 From 2009-10-27 12:22:26 PST -------
(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?
------- Comment #23 From 2009-10-27 12:25:35 PST -------
(In reply to comment #22)
> (In reply to comment #21)
> > Created an attachment (id=41971) [details] [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/
------- Comment #24 From 2009-10-27 13:40:31 PST -------
Created an attachment (id=41977) [details]
Updated test expectations
------- Comment #25 From 2009-10-27 13:45:52 PST -------
Created an attachment (id=41978) [details]
Updated test expectations
------- Comment #26 From 2009-10-27 14:55:27 PST -------
I'm confused by the result output being:
PASS
FAIL

Is this test intended to partially fail?
------- Comment #27 From 2009-10-27 15:25:59 PST -------
Created an attachment (id=41991) [details]
Added GTK and QT specific tests
------- Comment #28 From 2009-10-29 13:47:01 PST -------
(From update of attachment 41991 [details])
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.
------- Comment #29 From 2009-10-29 14:45:57 PST -------
Created an attachment (id=42149) [details]
Changed test to not say PASS/FAIL and only make the expectations platform specific
------- Comment #30 From 2009-10-29 14:49:16 PST -------
(From update of attachment 42149 [details])
ChangeLog is wrong, otherwise this looks fine.
------- Comment #31 From 2009-10-29 16:10:56 PST -------
Created an attachment (id=42159) [details]
Fixed issues with tabIndex and contentEditable and expanded the tests to cover these
------- Comment #32 From 2009-10-29 16:48:31 PST -------
(From update of attachment 42159 [details])
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.
------- Comment #33 From 2009-10-29 17:56:24 PST -------
Created an attachment (id=42163) [details]
Took care of Eric's comments
------- Comment #34 From 2009-10-29 18:09:31 PST -------
(From update of attachment 42163 [details])
Works for me.
------- Comment #35 From 2009-10-29 19:39:09 PST -------
(From update of attachment 42163 [details])
Clearing flags on attachment: 42163

Committed r50315: <http://trac.webkit.org/changeset/50315>
------- Comment #36 From 2009-10-29 19:39:15 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #37 From 2009-10-30 09:58:36 PST -------
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.
------- Comment #38 From 2010-05-02 18:27:51 PST -------
With r50315, any time you cmd-click on someone's profile pic on Facebook, you focus the link around the photo.  Looks pretty terrible.
------- Comment #39 From 2010-05-03 16:25:18 PST -------
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.
------- Comment #40 From 2010-05-04 14:16:32 PST -------
The same change caused this regression:

https://bugs.webkit.org/show_bug.cgi?id=38548
------- Comment #41 From 2010-05-04 14:48:15 PST -------
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.
------- Comment #42 From 2010-05-04 17:01:57 PST -------
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.
------- Comment #43 From 2010-05-04 17:42:47 PST -------
(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.
------- Comment #44 From 2011-01-08 14:05:52 PST -------
See also: bug 26856.
------- Comment #45 From 2011-01-08 14:09:13 PST -------
Sorry, discussion above already has more and better links to related bugs.
------- Comment #46 From 2011-03-09 11:07:29 PST -------
*** Bug 55975 has been marked as a duplicate of this bug. ***