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
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
http://hedgerwow.appspot.com/bug/webk...
: HasReduction
Depends on: 30523
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-30 13:38 PDT by Hedger Wang
Modified: 2011-03-09 11:07 PST (History)
12 users (show)

See Also:


Attachments
patch (1.16 KB, patch)
2009-10-05 14:11 PDT, Chang Shu
arv: review-
Details | Formatted Diff | Diff
patch with test case (3.54 KB, patch)
2009-10-07 14:46 PDT, Chang Shu
no flags Details | Formatted Diff | Diff
apply change only to non-Mac platform (4.04 KB, patch)
2009-10-12 11:37 PDT, Chang Shu
no flags Details | Formatted Diff | Diff
Make links mouse focusable only on GTK and QT only (3.92 KB, patch)
2009-10-27 11:55 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Updated test expectations (4.47 KB, patch)
2009-10-27 13:40 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Updated test expectations (4.94 KB, patch)
2009-10-27 13:45 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Added GTK and QT specific tests (8.10 KB, patch)
2009-10-27 15:25 PDT, Erik Arvidsson
no flags 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 PDT, Erik Arvidsson
no flags 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 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Took care of Eric's comments (7.21 KB, patch)
2009-10-29 17:56 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hedger Wang 2009-06-30 13:38:17 PDT
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 Chang Shu 2009-10-05 14:11:08 PDT
Created attachment 40659 [details]
patch
Comment 2 Erik Arvidsson 2009-10-05 14:47:25 PDT
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.
Comment 3 Chang Shu 2009-10-06 06:50:42 PDT
(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.
Comment 4 Chang Shu 2009-10-07 14:46:17 PDT
Created attachment 40824 [details]
patch with test case
Comment 5 Adele Peterson 2009-10-07 15:01:34 PDT
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.
Comment 6 Erik Arvidsson 2009-10-07 15:09:07 PDT
(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
Comment 7 Adele Peterson 2009-10-07 15:56:41 PDT
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 Alexey Proskuryakov 2009-10-07 21:10:19 PDT
> 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 Chang Shu 2009-10-08 06:35:34 PDT
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 Chang Shu 2009-10-08 06:59:41 PDT
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 Erik Arvidsson 2009-10-08 09:51:24 PDT
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 Chang Shu 2009-10-08 11:44:12 PDT
(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 Chang Shu 2009-10-12 11:37:26 PDT
Created attachment 41053 [details]
apply change only to non-Mac platform
Comment 14 Erik Arvidsson 2009-10-12 12:11:44 PDT
Comment on attachment 41053 [details]
apply change only to non-Mac platform

Having FAIL in the platform/mac/ expectation seems bad to me.
Comment 15 Chang Shu 2009-10-13 11:30:12 PDT
(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.
Comment 16 Adele Peterson 2009-10-14 16:17:06 PDT
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.
Comment 17 WebKit Commit Bot 2009-10-15 05:49:37 PDT
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>
Comment 18 WebKit Commit Bot 2009-10-15 05:49:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Brian Weinstein 2009-10-15 16:46:53 PDT
Updated Windows results were committed in http://trac.webkit.org/changeset/49667 and http://trac.webkit.org/changeset/49640.
Comment 20 Erik Arvidsson 2009-10-27 10:09:00 PDT
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 Erik Arvidsson 2009-10-27 11:55:02 PDT
Created attachment 41971 [details]
Make links mouse focusable only on GTK and QT only
Comment 22 Chang Shu 2009-10-27 12:22:26 PDT
(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?
Comment 23 Erik Arvidsson 2009-10-27 12:25:35 PDT
(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/
Comment 24 Erik Arvidsson 2009-10-27 13:40:31 PDT
Created attachment 41977 [details]
Updated test expectations
Comment 25 Erik Arvidsson 2009-10-27 13:45:52 PDT
Created attachment 41978 [details]
Updated test expectations
Comment 26 Eric Seidel 2009-10-27 14:55:27 PDT
I'm confused by the result output being:
PASS
FAIL

Is this test intended to partially fail?
Comment 27 Erik Arvidsson 2009-10-27 15:25:59 PDT
Created attachment 41991 [details]
Added GTK and QT specific tests
Comment 28 Eric Seidel 2009-10-29 13:47:01 PDT
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.
Comment 29 Erik Arvidsson 2009-10-29 14:45:57 PDT
Created attachment 42149 [details]
Changed test to not say PASS/FAIL and only make the expectations platform specific
Comment 30 Eric Seidel 2009-10-29 14:49:16 PDT
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.
Comment 31 Erik Arvidsson 2009-10-29 16:10:56 PDT
Created attachment 42159 [details]
Fixed issues with tabIndex and contentEditable and expanded the tests to cover these
Comment 32 Eric Seidel 2009-10-29 16:48:31 PDT
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.
Comment 33 Erik Arvidsson 2009-10-29 17:56:24 PDT
Created attachment 42163 [details]
Took care of Eric's comments
Comment 34 Eric Seidel 2009-10-29 18:09:31 PDT
Comment on attachment 42163 [details]
Took care of Eric's comments

Works for me.
Comment 35 WebKit Commit Bot 2009-10-29 19:39:09 PDT
Comment on attachment 42163 [details]
Took care of Eric's comments

Clearing flags on attachment: 42163

Committed r50315: <http://trac.webkit.org/changeset/50315>
Comment 36 WebKit Commit Bot 2009-10-29 19:39:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 Erik Arvidsson 2009-10-30 09:58:36 PDT
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 Adele Peterson 2010-05-02 18:27:51 PDT
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 Erik Arvidsson 2010-05-03 16:25:18 PDT
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 Beth Dakin 2010-05-04 14:16:32 PDT
The same change caused this regression:

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