Bug 136121

Summary: [GTK] Remove a focus ring on anchor node when focused by mouse.
Product: WebKit Reporter: ChangSeok Oh <changseok>
Component: WebKitGTKAssignee: ChangSeok Oh <changseok>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, changseok, commit-queue, esprehn+autocc, gustavo, gyuyoung.kim, hs85.jeong, jdiggs, jh718.park, mario, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
mcatanzaro: review+
Patch none

Description ChangSeok Oh 2014-08-20 22:34:18 PDT
Safari, Chrome and FF don't show a focus ring, the dotted rectangle on anchor node for mouse click.
I think the behavior is reasonable, no reason for gtk port to keep the focus on anchor node. Of course, the focus ring for tab navigation should not be affected.

How to reproduce
1. Visit http://www.raspberrypi.org
2. click the search icon on right-top of the page. The dotted focus ring appears on the icon.
3. click the icon again. The focus ring keeps staying on the icon.
Comment 1 ChangSeok Oh 2014-08-21 00:44:09 PDT
Created attachment 236916 [details]
Patch
Comment 2 Martin Robinson 2014-08-21 07:39:24 PDT
Does caret browsing still work after this change?
Comment 3 ChangSeok Oh 2014-08-21 10:43:45 PDT
(In reply to comment #2)
> Does caret browsing still work after this change?

Yes, it does. The patch only affects the mouse event related focus ring.
Comment 4 ChangSeok Oh 2014-08-26 20:30:58 PDT
Any concern?
Comment 5 Carlos Garcia Campos 2014-08-28 00:33:42 PDT
(In reply to comment #4)
> Any concern?

I'm not sure, I personally find it useful, sometimes when I'm opening links in a new tabs and then go back to the previous page, I quickly find the last opened link because it's focused.
Comment 6 Gustavo Noronha (kov) 2014-08-28 04:38:47 PDT
Maybe we should ask the GNOME designers what their take on this is?
Comment 7 ChangSeok Oh 2014-08-28 07:32:04 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Any concern?
> 
> I'm not sure, I personally find it useful, sometimes when I'm opening links in a new tabs and then go back to the previous page, I quickly find the last opened link because it's focused.

Aha, I got your point and partially agree on your thought. For a text link, :visited selector is helpful in that case, but it's not for an image link.. 
BTW do we really need to remember the last position what I clicked even when using a mouse? Hrm.. I don't know.
Some people could feel the dotted line is useful, but some others feels it just looks ugly. Personally I lean to the latter and think it would be ok that following the behavior of other browsers. 
However I don't want to push my thought, yeah. as Gustavo said, it would be good to hear GNOME designers' opinion. Who is the best person? :)
Comment 8 Michael Catanzaro 2016-01-06 20:08:18 PST
Comment on attachment 236916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236916&action=review

I vote for following what other browsers do. Especially for following what the Mac port does. Questions like these are not the expertise of GNOME designers.

Please, wait a few days or for a comment from Carlos before committing, in case he wants to object.

> Source/WebCore/html/HTMLAnchorElement.cpp:99
> +#if !PLATFORM(EFL)

I suspect EFL is only doing this because it's what we did. I'd say you should just remove this whole #if, and also remove the EFL test expectation. I doubt EFL wants to be the only port with different behavior here.
Comment 9 Gyuyoung Kim 2016-01-06 20:58:15 PST
Comment on attachment 236916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236916&action=review

>> Source/WebCore/html/HTMLAnchorElement.cpp:99
>> +#if !PLATFORM(EFL)
> 
> I suspect EFL is only doing this because it's what we did. I'd say you should just remove this whole #if, and also remove the EFL test expectation. I doubt EFL wants to be the only port with different behavior here.

Hunseop and Joongheon, do you need to keep this feature for Tizen ?
Comment 10 Joonghun Park 2016-01-06 21:32:47 PST
(In reply to comment #9)
> Comment on attachment 236916 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=236916&action=review
> 
> >> Source/WebCore/html/HTMLAnchorElement.cpp:99
> >> +#if !PLATFORM(EFL)
> > 
> > I suspect EFL is only doing this because it's what we did. I'd say you should just remove this whole #if, and also remove the EFL test expectation. I doubt EFL wants to be the only port with different behavior here.
> 
> Hunseop and Joongheon, do you need to keep this feature for Tizen ?

We don't think it is necessary to keep this behaviour in Tizen.
And IMHO, it would be better to have consistency with other ports, and looks more neat without the focus ring.
Comment 11 Carlos Garcia Campos 2016-01-06 23:37:02 PST
I prefer to keep it, but if you all think it's better to remove it, I won't oppose.
Comment 12 ChangSeok Oh 2016-01-20 23:23:57 PST
(In reply to comment #11)
> I prefer to keep it, but if you all think it's better to remove it, I won't
> oppose.

Good to hear that! Let me rebase the patch. Thanks everyone for bringing this up again. ;)
Comment 13 ChangSeok Oh 2016-01-20 23:38:38 PST
Created attachment 269436 [details]
Patch
Comment 14 Michael Catanzaro 2016-01-21 06:31:46 PST
Comment on attachment 269436 [details]
Patch

r=me
Comment 15 WebKit Commit Bot 2016-01-22 01:19:55 PST
Comment on attachment 269436 [details]
Patch

Clearing flags on attachment: 269436

Committed r195447: <http://trac.webkit.org/changeset/195447>
Comment 16 ChangSeok Oh 2016-04-06 21:37:16 PDT
Patch landed. Close.