Make anchors mouse-focusable

Make anchors mouse-focusable

Anchors are now also mouse-focusable, to be consistent with Gecko,
Trident and Presto. When an anchor is focused by mouse, the default
focus ring is not shown (unless tabindex is set), as before.

The expectation of fast/events/click-focus-anchor has changed as follows:
- a3 is also focusable by mouse, because it is a link.
- a7 is focused instead of p7 because a7 is focusable, and therefore get
  the focus instead of the parent <p> element.

BUG=388666
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181127

[pdr., 2014-08-10 20:21:52]

What about touch events? Do the other browsers have the same behavior for touch
events?

https://codereview.chromium.org/455223002/diff/20001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/455223002/diff/20001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:742: crbug.com/388666
fast/events/click-focus-anchor-no-ring.html [ Missing ]
Why are these being added as Missing?

https://codereview.chromium.org/455223002/diff/20001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/455223002/diff/20001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:88: bool
HTMLAnchorElement::shouldHaveFocusAppearance() const
This will be the third time we've copied the exact same three virtual functions
with the same implementations (HTMLFormControlElement, HTMLAnchorElement,
SVGAElement). Should we consider a different design?

https://codereview.chromium.org/455223002/diff/20001/Source/core/svg/SVGAElem...
File Source/core/svg/SVGAElement.cpp (right):

https://codereview.chromium.org/455223002/diff/20001/Source/core/svg/SVGAElem...
Source/core/svg/SVGAElement.cpp:167: return !m_wasFocusedByMouse ||
Element::supportsFocus();
This should be SVGElement::supportsFocus();

[pdr., 2014-08-10 21:00:17]

+cc tkent who added the original mouse focus code. (see:
https://codereview.chromium.org/16194013#msg12)

[robwu, 2014-08-10 23:37:19]

https://codereview.chromium.org/455223002/diff/20001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/455223002/diff/20001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:742: crbug.com/388666
fast/events/click-focus-anchor-no-ring.html [ Missing ]
On 2014/08/10 20:21:52, pdr wrote:
> Why are these being added as Missing?

This is my first Blink patch with pixel tests. I thought that I need pixel tests
for all platforms, to be retrieved via try bots. I put "[ Missing ]" here as
suggested by
https://groups.google.com/a/chromium.org/d/msg/blink-dev/sIqXs-soEEE/gs1CP-1-...

What should I have done instead?

https://codereview.chromium.org/455223002/diff/20001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/455223002/diff/20001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:88: bool
HTMLAnchorElement::shouldHaveFocusAppearance() const
On 2014/08/10 20:21:52, pdr wrote:
> This will be the third time we've copied the exact same three virtual
functions
> with the same implementations (HTMLFormControlElement, HTMLAnchorElement,
> SVGAElement). Should we consider a different design?

Yes, certainly. A template class seems like a great choice, to minimize code
duplication. I'll check whether that way is viable.

https://codereview.chromium.org/455223002/diff/20001/Source/core/svg/SVGAElem...
File Source/core/svg/SVGAElement.cpp (right):

https://codereview.chromium.org/455223002/diff/20001/Source/core/svg/SVGAElem...
Source/core/svg/SVGAElement.cpp:167: return !m_wasFocusedByMouse ||
Element::supportsFocus();
On 2014/08/10 20:21:52, pdr wrote:
> This should be SVGElement::supportsFocus();

Neither of the two, it should be SVGGraphicsElement. If
SVGElement::supportsFocus is used, then the focus ring would never show up for
mouse focus, even if tabIndex is set (SVGElement::supportsFocus returns false).
I have replaced Element with SVGGraphicsElement, which in turn calls to
Element::supportsFocus.

[pdr., 2014-08-10 23:43:41]

On 2014/08/10 23:37:19, robwu wrote:
>
https://codereview.chromium.org/455223002/diff/20001/LayoutTests/TestExpectat...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/455223002/diff/20001/LayoutTests/TestExpectat...
> LayoutTests/TestExpectations:742: crbug.com/388666
> fast/events/click-focus-anchor-no-ring.html [ Missing ]
> On 2014/08/10 20:21:52, pdr wrote:
> > Why are these being added as Missing?
> 
> This is my first Blink patch

Welcome!

> with pixel tests. I thought that I need pixel tests
> for all platforms, to be retrieved via try bots. I put "[ Missing ]" here as
> suggested by
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/sIqXs-soEEE/gs1CP-1-...
> 
> What should I have done instead?

Ah, that thread predates the rebaseline bot. I recommend using [ NeedsRebaseline
] instead of [ Missing ] which will cause a bot to come around after all
platforms have run the test and update the expectations.

> 
>
https://codereview.chromium.org/455223002/diff/20001/Source/core/html/HTMLAnc...
> File Source/core/html/HTMLAnchorElement.cpp (right):
> 
>
https://codereview.chromium.org/455223002/diff/20001/Source/core/html/HTMLAnc...
> Source/core/html/HTMLAnchorElement.cpp:88: bool
> HTMLAnchorElement::shouldHaveFocusAppearance() const
> On 2014/08/10 20:21:52, pdr wrote:
> > This will be the third time we've copied the exact same three virtual
> functions
> > with the same implementations (HTMLFormControlElement, HTMLAnchorElement,
> > SVGAElement). Should we consider a different design?
> 
> Yes, certainly. A template class seems like a great choice, to minimize code
> duplication. I'll check whether that way is viable.

Templates are an option but I think there may be something simpler here. Can all
of this be moved into FocusController?

> 
>
https://codereview.chromium.org/455223002/diff/20001/Source/core/svg/SVGAElem...
> File Source/core/svg/SVGAElement.cpp (right):
> 
>
https://codereview.chromium.org/455223002/diff/20001/Source/core/svg/SVGAElem...
> Source/core/svg/SVGAElement.cpp:167: return !m_wasFocusedByMouse ||
> Element::supportsFocus();
> On 2014/08/10 20:21:52, pdr wrote:
> > This should be SVGElement::supportsFocus();
> 
> Neither of the two, it should be SVGGraphicsElement. If
> SVGElement::supportsFocus is used, then the focus ring would never show up for
> mouse focus, even if tabIndex is set (SVGElement::supportsFocus returns
false).
> I have replaced Element with SVGGraphicsElement, which in turn calls to
> Element::supportsFocus.

Excellent, thanks.

[robwu, 2014-08-11 00:07:59]

On 2014/08/10 23:43:41, pdr wrote:
> On 2014/08/10 23:37:19, robwu wrote:
> >
>
https://codereview.chromium.org/455223002/diff/20001/LayoutTests/TestExpectat...
> > File LayoutTests/TestExpectations (right):
> > 
> >
>
https://codereview.chromium.org/455223002/diff/20001/LayoutTests/TestExpectat...
> > LayoutTests/TestExpectations:742: crbug.com/388666
> > fast/events/click-focus-anchor-no-ring.html [ Missing ]
> > On 2014/08/10 20:21:52, pdr wrote:
> > > Why are these being added as Missing?
> > 
> > This is my first Blink patch
> 
> Welcome!

Hi :)

> 
> > with pixel tests. I thought that I need pixel tests
> > for all platforms, to be retrieved via try bots. I put "[ Missing ]" here as
> > suggested by
> >
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/sIqXs-soEEE/gs1CP-1-...
> > 
> > What should I have done instead?
> 
> Ah, that thread predates the rebaseline bot. I recommend using [
NeedsRebaseline
> ] instead of [ Missing ] which will cause a bot to come around after all
> platforms have run the test and update the expectations.

That looks much simpler, thanks. Do I need to do anything else, e.g. adding th
*-expected.txt, or will the bot automatically add the files to the patch? What
can I do to make sure that the bot-generated baseline matches my expectations?

> 
> > 
> >
>
https://codereview.chromium.org/455223002/diff/20001/Source/core/html/HTMLAnc...
> > File Source/core/html/HTMLAnchorElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/455223002/diff/20001/Source/core/html/HTMLAnc...
> > Source/core/html/HTMLAnchorElement.cpp:88: bool
> > HTMLAnchorElement::shouldHaveFocusAppearance() const
> > On 2014/08/10 20:21:52, pdr wrote:
> > > This will be the third time we've copied the exact same three virtual
> > functions
> > > with the same implementations (HTMLFormControlElement, HTMLAnchorElement,
> > > SVGAElement). Should we consider a different design?
> > 
> > Yes, certainly. A template class seems like a great choice, to minimize code
> > duplication. I'll check whether that way is viable.
> 
> Templates are an option but I think there may be something simpler here. Can
all
> of this be moved into FocusController?

FocusController looks like the right tool for this task. I will investigate
whether it is possible to add a kind of wasFocusedByMouse() method to this class
and use the method within HTMLFormControlElement HTMLAnchorElement and
SVGAElement.

> 
> > 
> >
>
https://codereview.chromium.org/455223002/diff/20001/Source/core/svg/SVGAElem...
> > File Source/core/svg/SVGAElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/455223002/diff/20001/Source/core/svg/SVGAElem...
> > Source/core/svg/SVGAElement.cpp:167: return !m_wasFocusedByMouse ||
> > Element::supportsFocus();
> > On 2014/08/10 20:21:52, pdr wrote:
> > > This should be SVGElement::supportsFocus();
> > 
> > Neither of the two, it should be SVGGraphicsElement. If
> > SVGElement::supportsFocus is used, then the focus ring would never show up
for
> > mouse focus, even if tabIndex is set (SVGElement::supportsFocus returns
> false).
> > I have replaced Element with SVGGraphicsElement, which in turn calls to
> > Element::supportsFocus.
> 
> Excellent, thanks.

[pdr., 2014-08-11 00:25:54]

On 2014/08/11 00:07:59, robwu wrote:
> > Ah, that thread predates the rebaseline bot. I recommend using [
> NeedsRebaseline
> > ] instead of [ Missing ] which will cause a bot to come around after all
> > platforms have run the test and update the expectations.
> 
> That looks much simpler, thanks. Do I need to do anything else, e.g. adding th
> *-expected.txt, or will the bot automatically add the files to the patch? What
> can I do to make sure that the bot-generated baseline matches my expectations?

"git log -S NeedsRebaseline" is your friend ;) You can use this to see what
other patches have done.


In general, we avoid pixel tests unless they are absolutely. I just looked in
more detail at your tests and I think they can be refactored to be
platform-independent "reference" tests instead of pixel tests. Can you
investigate using only reference tests for this patch, and not adding any lines
to TestExpectations?

A reference test has a test file (mytest.html) and an expected file
(mytest-expected.html). The tests in Tkent's original patch are good examples to
follow: https://codereview.chromium.org/16194013

> 
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/455223002/diff/20001/Source/core/html/HTMLAnc...
> > > File Source/core/html/HTMLAnchorElement.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/455223002/diff/20001/Source/core/html/HTMLAnc...
> > > Source/core/html/HTMLAnchorElement.cpp:88: bool
> > > HTMLAnchorElement::shouldHaveFocusAppearance() const
> > > On 2014/08/10 20:21:52, pdr wrote:
> > > > This will be the third time we've copied the exact same three virtual
> > > functions
> > > > with the same implementations (HTMLFormControlElement,
HTMLAnchorElement,
> > > > SVGAElement). Should we consider a different design?
> > > 
> > > Yes, certainly. A template class seems like a great choice, to minimize
code
> > > duplication. I'll check whether that way is viable.
> > 
> > Templates are an option but I think there may be something simpler here. Can
> all
> > of this be moved into FocusController?
> 
> FocusController looks like the right tool for this task. I will investigate
> whether it is possible to add a kind of wasFocusedByMouse() method to this
class
> and use the method within HTMLFormControlElement HTMLAnchorElement and
> SVGAElement.

I'm not sure if it will work out, but that's exactly what I was thinking.

[tkent, 2014-08-11 02:15:56]

> When an anchor is focused by mouse, the default
> focus ring is not shown (unless tabindex is set), as before.

So, if <a> has no tabindex, we won't show focus ring even if a user focused on
it with TAB?

[robwu, 2014-08-11 07:49:06]

On 2014/08/11 02:15:56, tkent wrote:
> > When an anchor is focused by mouse, the default
> > focus ring is not shown (unless tabindex is set), as before.
> 
> So, if <a> has no tabindex, we won't show focus ring even if a user focused on
> it with TAB?

Read the line as "the default focus ring is not shown (unless tabindex is set;
then the ring is shown),"

[robwu, 2014-08-11 10:50:13]

On 2014/08/11 00:25:54, pdr wrote:
>
> In general, we avoid pixel tests unless they are absolutely. I just looked in
> more detail at your tests and I think they can be refactored to be
> platform-independent "reference" tests instead of pixel tests. Can you
> investigate using only reference tests for this patch, and not adding any
lines
> to TestExpectations?
> 
> A reference test has a test file (mytest.html) and an expected file
> (mytest-expected.html). The tests in Tkent's original patch are good examples
to
> follow: https://codereview.chromium.org/16194013

This CL has a reference test that verifies that the anchors are indeed
focusable. The pixel tests exist to ensure that the focus ring is (not) shown
when needed. AFAIK, there is no way in JS to detect whether the default focus
ring has been show; getComputedStyle returns something like "rgb(x,y,z) auto
5px" regardless of whether the focus ring is shown.

> > > Templates are an option but I think there may be something simpler here.
Can
> > all
> > > of this be moved into FocusController?
> > 
> > FocusController looks like the right tool for this task. I will investigate
> > whether it is possible to add a kind of wasFocusedByMouse() method to this
> class
> > and use the method within HTMLFormControlElement HTMLAnchorElement and
> > SVGAElement.
> 
> I'm not sure if it will work out, but that's exactly what I was thinking.

Done.

[pdr., 2014-08-12 02:02:23]

I like the direction you took with this patch. A bunch of minor comments below.

https://codereview.chromium.org/455223002/diff/60001/LayoutTests/fast/events/...
File LayoutTests/fast/events/click-focus-anchor-no-ring.html (right):

https://codereview.chromium.org/455223002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/click-focus-anchor-no-ring.html:4: <p>This test ensures
that the focus ring is not drawn for anchors that received the focus by
mouse.</p>
Can you change this to say:
This test ensures that the focus ring is not drawn for anchors without a
tabindex that received the focus by mouse.

Similarly for the SVG test.

https://codereview.chromium.org/455223002/diff/60001/LayoutTests/fast/events/...
File LayoutTests/fast/events/click-focus-svganchor-has-ring.html (right):

https://codereview.chromium.org/455223002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/click-focus-svganchor-has-ring.html:6: <svg>
Can you add an explicit width and height to this? What you have is correct but
an explicit size will help us be resilient to svg default sizing changes in the
future.

https://codereview.chromium.org/455223002/diff/60001/LayoutTests/fast/events/...
File LayoutTests/fast/events/resources/focus-anchor-by-mouse.js (right):

https://codereview.chromium.org/455223002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/resources/focus-anchor-by-mouse.js:5:
testRunner.dumpAsTextWithPixelResults();
I don't think we need any pixel tests are needed here. For your reftest
expectation files, just explicitly call focus() on it and the focus ring will be
drawn.

https://codereview.chromium.org/455223002/diff/60001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/455223002/diff/60001/Source/core/dom/Node.cpp...
Source/core/dom/Node.cpp:2202: renderer()->paintInvalidationForWholeRenderer();
Is this paintInvalidationForWholeRenderer() actually needed?

After seeing the first iteration of this review, I investigated removing it in
https://codereview.chromium.org/461483002. You now know this code as well as I
do; please feel free to comment on my review. I've been unable to come up with a
testcase that fails without it, but I may be missing something.

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (left):

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:89: // Links are focusable by default,
but only allow links with tabindex or contenteditable to be mouse focusable.
Both here and in SVGAElement we're removing this comment about contenteditable
elements being mouse focusable.

Can you add a test for content editable svg and html links that are selected and
not selected by the mouse?

https://codereview.chromium.org/455223002/diff/60001/Source/core/page/FocusCo...
File Source/core/page/FocusController.h (right):

https://codereview.chromium.org/455223002/diff/60001/Source/core/page/FocusCo...
Source/core/page/FocusController.h:121: bool m_wasFocusedByMouse;
It may be clearer to rename this "m_elementWasFocusedByMouse" (and the
supporting functions) to make it clear that the page focus event isn't affected.
I don't feel strongly about this though.

https://codereview.chromium.org/455223002/diff/60001/Source/core/svg/SVGAElem...
File Source/core/svg/SVGAElement.cpp (right):

https://codereview.chromium.org/455223002/diff/60001/Source/core/svg/SVGAElem...
Source/core/svg/SVGAElement.cpp:167: return
!document().page()->focusController().wasFocusedByMouse() ||
SVGGraphicsElement::supportsFocus();
Why is this SVGGraphicsElement::supportsFocus instead of just supportsFocus()?

[pdr., 2014-08-12 02:18:54]

https://codereview.chromium.org/455223002/diff/60001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/455223002/diff/60001/Source/core/dom/Node.cpp...
Source/core/dom/Node.cpp:2202: renderer()->paintInvalidationForWholeRenderer();
On 2014/08/12 02:02:22, pdr wrote:
> Is this paintInvalidationForWholeRenderer() actually needed?
> 
> After seeing the first iteration of this review, I investigated removing it in
> https://codereview.chromium.org/461483002. You now know this code as well as I
> do; please feel free to comment on my review. I've been unable to come up with
a
> testcase that fails without it, but I may be missing something.

Ignore me--this is needed after all.

[Xianzhu, 2014-08-12 03:58:01]

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:92: return
!document().page()->focusController().wasFocusedByMouse() ||
HTMLElement::supportsFocus();
The above code will have side-effect. For example:

<a href="#" style="outline:red auto 5px">...</a>
<a href="#">...</a>

When the second element is focused by mouse, the first element will lose its
outline because of the global wasFocusedByMouse status becomes false. Depending
on whether the tile(s) containing the first element is invalidated when the
second element is repainted, the first element will appear still having outline
(unstable), not having outline, or corrupted.

The flag introduces another factor besides DOM structure and styles affecting
element appearance, which might be agile. Can we let it trigger a style change?
(e.g. sth like :focus[_bymouse] { outline: none })

[Xianzhu, 2014-08-12 04:01:11]

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:92: return
!document().page()->focusController().wasFocusedByMouse() ||
HTMLElement::supportsFocus();
s/agile/fragile/ ..

[robwu, 2014-08-12 14:27:49]

https://codereview.chromium.org/455223002/diff/60001/LayoutTests/fast/events/...
File LayoutTests/fast/events/click-focus-anchor-no-ring.html (right):

https://codereview.chromium.org/455223002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/click-focus-anchor-no-ring.html:4: <p>This test ensures
that the focus ring is not drawn for anchors that received the focus by
mouse.</p>
On 2014/08/12 02:02:22, pdr wrote:
> Can you change this to say:
> This test ensures that the focus ring is not drawn for anchors without a
> tabindex that received the focus by mouse.
> 
> Similarly for the SVG test.

Done.

https://codereview.chromium.org/455223002/diff/60001/LayoutTests/fast/events/...
File LayoutTests/fast/events/click-focus-svganchor-has-ring.html (right):

https://codereview.chromium.org/455223002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/click-focus-svganchor-has-ring.html:6: <svg>
On 2014/08/12 02:02:22, pdr wrote:
> Can you add an explicit width and height to this? What you have is correct but
> an explicit size will help us be resilient to svg default sizing changes in
the
> future.

Done.

https://codereview.chromium.org/455223002/diff/60001/LayoutTests/fast/events/...
File LayoutTests/fast/events/resources/focus-anchor-by-mouse.js (right):

https://codereview.chromium.org/455223002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/resources/focus-anchor-by-mouse.js:5:
testRunner.dumpAsTextWithPixelResults();
On 2014/08/12 02:02:22, pdr wrote:
> I don't think we need any pixel tests are needed here. For your reftest
> expectation files, just explicitly call focus() on it and the focus ring will
be
> drawn.

I cannot think of any other method besides pixel tests to verify that a focus
ring is drawn when needed, and not drawn when not needed.

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (left):

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:89: // Links are focusable by default,
but only allow links with tabindex or contenteditable to be mouse focusable.
On 2014/08/12 02:02:22, pdr wrote:
> Both here and in SVGAElement we're removing this comment about contenteditable
> elements being mouse focusable.
> 
> Can you add a test for content editable svg and html links that are selected
and
> not selected by the mouse?

LayoutTests/fast/events/click-focus-anchor.html already has tests for
contentEditable on HTML <a> elements. Do you want similar tests for SVG <a>
elements as well, or did you mean something else?

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:92: return
!document().page()->focusController().wasFocusedByMouse() ||
HTMLElement::supportsFocus();
On 2014/08/12 03:58:01, Xianzhu wrote:
> The above code will have side-effect. For example:
> 
> <a href="#" style="outline:red auto 5px">...</a>
> <a href="#">...</a>
> 
> When the second element is focused by mouse, the first element will lose its
> outline because of the global wasFocusedByMouse status becomes false.
Depending
> on whether the tile(s) containing the first element is invalidated when the
> second element is repainted, the first element will appear still having
outline
> (unstable), not having outline, or corrupted.

What is "outline-style:auto" supposed to do when an element has no focus?
The spec (CSS3-UI section 7.4) allows "auto" to be treated as "solid".
Currently (before my changes), setting `outline:auto` causes the default focus
ring to be displayed (note: this style does *not* look like "solid"),
irrespective of whether the element is actually focusable. Further outline-width
and outline-offset have no influence on the rendering of the special focus ring
(reported as crbug.com/402874),

> 
> The flag introduces another factor besides DOM structure and styles affecting
> element appearance, which might be agile. Can we let it trigger a style
change?
> (e.g. sth like :focus[_bymouse] { outline: none })

Are you suggesting to introduce a new pseudo-class, e.g. :-webkit-mouse-focus,
as in:

/* default style = outline-style:none; */
/* UA stylesheet: */
textarea:focus:not(:-webkit-mouse-focus),
input:focus:not(:-webkit-mouse-focus) {
:link:focus:not(:-webkit-mouse-focus) {
    outline-style: auto;
}
/*  (perhaps -webkit-focus-ring-style instead of "auto" ?) */

https://codereview.chromium.org/455223002/diff/60001/Source/core/svg/SVGAElem...
File Source/core/svg/SVGAElement.cpp (right):

https://codereview.chromium.org/455223002/diff/60001/Source/core/svg/SVGAElem...
Source/core/svg/SVGAElement.cpp:167: return
!document().page()->focusController().wasFocusedByMouse() ||
SVGGraphicsElement::supportsFocus();
On 2014/08/12 02:02:23, pdr wrote:
> Why is this SVGGraphicsElement::supportsFocus instead of just supportsFocus()?

I noticed that the HTMLAnchorElement refers to HTMLElement::supportsFocus, so
for symmetry I also uses SVGAElement::supportsFocus in SVGAElement, to make sure
that any SVG-specific additions to supportFocus are also applied to the SVG <a>
element.

[Xianzhu, 2014-08-12 16:03:08]

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:92: return
!document().page()->focusController().wasFocusedByMouse() ||
HTMLElement::supportsFocus();
On 2014/08/12 14:27:49, robwu wrote:
> On 2014/08/12 03:58:01, Xianzhu wrote:
> > The above code will have side-effect. For example:
> > 
> > <a href="#" style="outline:red auto 5px">...</a>
> > <a href="#">...</a>
> > 
> > When the second element is focused by mouse, the first element will lose its
> > outline because of the global wasFocusedByMouse status becomes false.
> Depending
> > on whether the tile(s) containing the first element is invalidated when the
> > second element is repainted, the first element will appear still having
> outline
> > (unstable), not having outline, or corrupted.
> 
> What is "outline-style:auto" supposed to do when an element has no focus?
> The spec (CSS3-UI section 7.4) allows "auto" to be treated as "solid".
> Currently (before my changes), setting `outline:auto` causes the default focus
> ring to be displayed (note: this style does *not* look like "solid"),
> irrespective of whether the element is actually focusable. Further
outline-width
> and outline-offset have no influence on the rendering of the special focus
ring
> (reported as crbug.com/402874),

I think 'outline-style:auto' was first introduced in WebKit just for focus
rings, so the only appearance of it focus ring for now. However, as now it has
been a property value published in spec, we should support it formally.
Corrupted rendering caused by unstable repainting (that is, inconsistent
invalidation and repaint causing for example half outline around an element) is
not acceptable.

We are on the way to remove paintInvalidationForWholeRenderer because it breaks
the principle of repaint-after-layout (now actually 'invalidate at the end of
each frame). We really don't want sources other than style and layout triggering
repaint.

> > 
> > The flag introduces another factor besides DOM structure and styles
affecting
> > element appearance, which might be agile. Can we let it trigger a style
> change?
> > (e.g. sth like :focus[_bymouse] { outline: none })
> 
> Are you suggesting to introduce a new pseudo-class, e.g. :-webkit-mouse-focus,
> as in:
> 
> /* default style = outline-style:none; */
> /* UA stylesheet: */
> textarea:focus:not(:-webkit-mouse-focus),
> input:focus:not(:-webkit-mouse-focus) {
> :link:focus:not(:-webkit-mouse-focus) {
>     outline-style: auto;
> }
> /*  (perhaps -webkit-focus-ring-style instead of "auto" ?) */

Yes, something like that, to ensure the change of appearance of elements only
controlled by dom structure and css. Your proposal is much better than my
example :)

[pdr., 2014-08-12 17:53:43]

> > I don't think we need any pixel tests are needed here. For your reftest
> > expectation files, just explicitly call focus() on it and the focus ring
will
> be
> > drawn.
> 
> I cannot think of any other method besides pixel tests to verify that a focus
> ring is drawn when needed, and not drawn when not needed.

For testing that there is a focus ring:
test.html: <input><script>/* your test here */</script>
test-expected.html: <input id="input"><script>input.focus();</script>

For testing that there is not a focus ring:
test.html: <input><script>/* your test here */</script>
test-expected.html: <input>

[Xianzhu, 2014-08-14 19:12:49]

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:92: return
!document().page()->focusController().wasFocusedByMouse() ||
HTMLElement::supportsFocus();
I no longer insist the pseudo-class method. The current method also looks good.
:)

About paintInvalidationForWholeRenderer(), you can change it to
setShouldDoFullPaintInvalidation(true) after
https://codereview.chromium.org/478433002/.

[robwu, 2014-08-15 22:51:33]

https://codereview.chromium.org/455223002/diff/60001/LayoutTests/fast/events/...
File LayoutTests/fast/events/resources/focus-anchor-by-mouse.js (right):

https://codereview.chromium.org/455223002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/resources/focus-anchor-by-mouse.js:5:
testRunner.dumpAsTextWithPixelResults();
On 2014/08/12 02:02:22, pdr wrote:
> I don't think we need any pixel tests are needed here. For your reftest
> expectation files, just explicitly call focus() on it and the focus ring will
be
> drawn.

I've replaced the pixel tests with ref tests, and added focus tests for SVG.
WDYT now?

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/455223002/diff/60001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:92: return
!document().page()->focusController().wasFocusedByMouse() ||
HTMLElement::supportsFocus();
On 2014/08/14 19:12:49, Xianzhu wrote:
> I no longer insist the pseudo-class method. The current method also looks
good.
> :)
> 
> About paintInvalidationForWholeRenderer(), you can change it to
> setShouldDoFullPaintInvalidation(true) after
> https://codereview.chromium.org/478433002/.

Great. Anything else besides that?

[pdr., 2014-08-19 00:35:05]

On 2014/08/15 22:51:33, robwu wrote:
> Great. Anything else besides that?

I apologize for the delay, as I was on vacation for a 4-day weekend.


I think the codechange here looks great and we're pretty much ready to land
this. I'm a little confused about the tests though, because they all pass even
without the code change for me. Can you investigate this?


@tkent, do you mind doing a quick review as well?

[tkent, 2014-08-19 02:11:38]

The C++ code change looks ok.

I tested this CL locally, and I think the behavior is ok.

[robwu, 2014-08-19 14:37:15]

On 2014/08/19 00:35:05, pdr wrote:
> On 2014/08/15 22:51:33, robwu wrote:
> > Great. Anything else besides that?
> 
> I apologize for the delay, as I was on vacation for a 4-day weekend.
> 
> 
> I think the codechange here looks great and we're pretty much ready to land
> this. I'm a little confused about the tests though, because they all pass even
> without the code change for me. Can you investigate this?

fast/events/click-focus-anchor.html fails for me because these assert new
behavior, namely that elements that are focused by mouse receive the "focus"
event.

fast/events/click-focus-anchor-has-ring.html,
fast/events/click-focus-anchor-no-ring.html,
fast/events/click-focus-svganchor-has-ring.html and
fast/events/click-focus-svganchor-no-ring.html pass because they are used to
verify that the visual focus ring behavior does not change. This CL touches the
focus ring rendering logic, so I have created these tests to make sure that I
don't introduce any regressions.

[pdr., 2014-08-19 14:40:27]

On 2014/08/19 14:37:15, robwu wrote:
> On 2014/08/19 00:35:05, pdr wrote:
> > On 2014/08/15 22:51:33, robwu wrote:
> > > Great. Anything else besides that?
> > 
> > I apologize for the delay, as I was on vacation for a 4-day weekend.
> > 
> > 
> > I think the codechange here looks great and we're pretty much ready to land
> > this. I'm a little confused about the tests though, because they all pass
even
> > without the code change for me. Can you investigate this?
> 
> fast/events/click-focus-anchor.html fails for me because these assert new
> behavior, namely that elements that are focused by mouse receive the "focus"
> event.
> 
> fast/events/click-focus-anchor-has-ring.html,
> fast/events/click-focus-anchor-no-ring.html,
> fast/events/click-focus-svganchor-has-ring.html and
> fast/events/click-focus-svganchor-no-ring.html pass because they are used to
> verify that the visual focus ring behavior does not change. This CL touches
the
> focus ring rendering logic, so I have created these tests to make sure that I
> don't introduce any regressions.

LGTM

[pdr., 2014-08-19 14:40:44]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-08-19 14:41:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob@robwu.nl/455223002/100001

[commit-bot: I haz the power, 2014-08-19 14:41:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-19 14:41:53]

Failed to apply patch for Source/core/html/HTMLFormControlElement.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/html/HTMLFormControlElement.cpp
  Hunk #1 FAILED at 33.
  Hunk #2 succeeded at 58 (offset 2 lines).
  Hunk #3 FAILED at 330.
  2 out of 3 hunks FAILED -- saving rejects to file
Source/core/html/HTMLFormControlElement.cpp.rej

Patch:       Source/core/html/HTMLFormControlElement.cpp
Index: Source/core/html/HTMLFormControlElement.cpp
diff --git a/Source/core/html/HTMLFormControlElement.cpp
b/Source/core/html/HTMLFormControlElement.cpp
index
a4e37a36cc362f9912230017706c1f74ee3793f4..6e01ff07ddb1831c499c1ee56e74a867f00f5d13
100644
--- a/Source/core/html/HTMLFormControlElement.cpp
+++ b/Source/core/html/HTMLFormControlElement.cpp
@@ -33,6 +33,7 @@
 #include "core/html/HTMLLegendElement.h"
 #include "core/html/ValidityState.h"
 #include "core/frame/UseCounter.h"
+#include "core/page/FocusController.h"
 #include "core/page/Page.h"
 #include "core/page/ValidationMessageClient.h"
 #include "core/rendering/RenderBox.h"
@@ -56,7 +57,6 @@ HTMLFormControlElement::HTMLFormControlElement(const
QualifiedName& tagName, Doc
     , m_willValidate(true)
     , m_isValid(true)
     , m_wasChangedSinceLastFormControlChangeEvent(false)
-    , m_wasFocusedByMouse(false)
 {
     setHasCustomStyleCallbacks();
     associateByParser(form);
@@ -331,31 +331,12 @@ bool
HTMLFormControlElement::shouldShowFocusRingOnMouseFocus() const
     return false;
 }
 
-void HTMLFormControlElement::dispatchFocusEvent(Element* oldFocusedElement,
FocusType type)
-{
-    if (type != FocusTypePage)
-        m_wasFocusedByMouse = type == FocusTypeMouse;
-    HTMLElement::dispatchFocusEvent(oldFocusedElement, type);
-}
-
 bool HTMLFormControlElement::shouldHaveFocusAppearance() const
 {
     ASSERT(focused());
-    return shouldShowFocusRingOnMouseFocus() || !m_wasFocusedByMouse;
+    return shouldShowFocusRingOnMouseFocus() ||
!document().page()->focusController().wasFocusedByMouse();
 }
 
-void HTMLFormControlElement::willCallDefaultEventHandler(const Event& event)
-{
-    if (!event.isKeyboardEvent() || event.type() != EventTypeNames::keydown)
-        return;
-    if (!m_wasFocusedByMouse)
-        return;
-    m_wasFocusedByMouse = false;
-    if (renderer())
-        renderer()->paintInvalidationForWholeRenderer();
-}
-
-
 short HTMLFormControlElement::tabIndex() const
 {
     // Skip the supportsFocus check in HTMLElement.

[robwu, 2014-08-19 14:54:44]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-08-19 14:54:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob@robwu.nl/455223002/120001

[Xianzhu, 2014-08-19 15:25:47]

Thanks for the change.

Note that this change will still cause corrupted display if other elements have
'outline-style: auto' (details in #13). I'd suggest a follow-up change to make
the focusedByMouse flag not shared, or use pseudo-class.

[commit-bot: I haz the power, 2014-08-19 16:05:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/23458)

[robwu, 2014-08-19 17:03:56]

https://codereview.chromium.org/455223002/diff/120001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/455223002/diff/120001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:2190: }
These lines are bugging me a little bit.
Previously, setShouldDoFullPaintInvalidation() was only called when the mouse
focus state changed for some nodes (form control elements and links). Now it is
called for *every* node.

That doesn't sound great in terms of performance, does it?
I'm thinking of moving back the focus logic to nodes via a template classes,
which also fixes the bug mentioned in c13. WDYT?

[commit-bot: I haz the power, 2014-08-19 17:07:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-19 17:08:01]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

[pdr., 2014-08-19 17:41:47]

On 2014/08/19 17:03:56, robwu wrote:
> https://codereview.chromium.org/455223002/diff/120001/Source/core/dom/Node.cpp
> File Source/core/dom/Node.cpp (right):
> 
>
https://codereview.chromium.org/455223002/diff/120001/Source/core/dom/Node.cp...
> Source/core/dom/Node.cpp:2190: }
> These lines are bugging me a little bit.
> Previously, setShouldDoFullPaintInvalidation() was only called when the mouse
> focus state changed for some nodes (form control elements and links). Now it
is
> called for *every* node.
> 
> That doesn't sound great in terms of performance, does it?
> I'm thinking of moving back the focus logic to nodes via a template classes,
> which also fixes the bug mentioned in c13. WDYT?

Template classes in Node scare me a bit because of the mental complexity they
add which reduces maintainability. I could be convinced if there's a clean
design though.

This was rejected by the CQ due to a flake. Would you like to continue with this
review with the design you're proposing instead of landing this right now?

[robwu, 2014-08-22 23:39:04]

pdr@
I have updated the CL, and put the logic in Element. If the trybot passes, could
you take a look?

To review the changes, I suggest to look at the original logic in
Source/core/html/HTMLFormControlElement.cpp, and compare it with my changes in
Element.{cpp,h}.

If that change doesn't look appealing, then I can also try to use templates.
This is my initial template:
http://pastebin.com/VstsQ3mJ (FocusableElement.h)
http://pastebin.com/8i47F5zy (FocusableElement.cpp)
to be used as follows:
class HTMLAnchorElement : public FoucsableElement<HTMLElement> ...

[pdr., 2014-08-25 17:46:18]

This looks good to me.

@tkent and Xianzhu, would you take another look as well?

https://codereview.chromium.org/455223002/diff/140001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/455223002/diff/140001/Source/core/dom/Element...
Source/core/dom/Element.h:643: bool m_wasFocusedByMouse;
Should this be put in m_elementData instead?

[Xianzhu, 2014-08-25 17:56:24]

Change about invalidation lgtm.

[robwu, 2014-08-25 22:40:03]

https://codereview.chromium.org/455223002/diff/140001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/455223002/diff/140001/Source/core/dom/Element...
Source/core/dom/Element.h:643: bool m_wasFocusedByMouse;
On 2014/08/25 17:46:18, pdr wrote:
> Should this be put in m_elementData instead?

Probably yes. I found it a bit odd that there were so little members in the
Element class. I've just moved this property to elementData(), and on the way
renamed it. To prevent creating unnecessary UniqueElementData objects, the new
member is only set when the element supports optional focus rings.

Could you review my newest patch set?

[pdr., 2014-08-25 22:57:37]

https://codereview.chromium.org/455223002/diff/160001/Source/core/dom/Element...
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/455223002/diff/160001/Source/core/dom/Element...
Source/core/dom/ElementData.h:92: unsigned m_arraySize : 19;
You cannot just take 9 bits off m_arraySize :)

It looks like we will need to find an extra bit elsewhere.

[robwu, 2014-08-25 23:14:59]

https://codereview.chromium.org/455223002/diff/160001/Source/core/dom/Element...
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/455223002/diff/160001/Source/core/dom/Element...
Source/core/dom/ElementData.h:92: unsigned m_arraySize : 19;
On 2014/08/25 22:57:37, pdr wrote:
> You cannot just take 9 bits off m_arraySize :)
> 
> It looks like we will need to find an extra bit elsewhere.

Oops, should have removed only one bit. 28 -> 27 is fine, right?

[pdr., 2014-08-26 00:50:55]

(not lgtm, just to clear the lgtm bit so nobody accidentally hits commit)

On 2014/08/25 23:14:59, robwu wrote:
>
https://codereview.chromium.org/455223002/diff/160001/Source/core/dom/Element...
> File Source/core/dom/ElementData.h (right):
> 
>
https://codereview.chromium.org/455223002/diff/160001/Source/core/dom/Element...
> Source/core/dom/ElementData.h:92: unsigned m_arraySize : 19;
> On 2014/08/25 22:57:37, pdr wrote:
> > You cannot just take 9 bits off m_arraySize :)
> > 
> > It looks like we will need to find an extra bit elsewhere.
> 
> Oops, should have removed only one bit. 28 -> 27 is fine, right?

If only it were that easy :) I recommend using blame to do some investigation
and find out why it is 28. It looks like wkbug.com/103349 did some gross
contortions just to get it from 29 to 28 so I think it's unlikely we will be
able to change it for focus.

m_shouldHideFocusRingOnMouseFocus is only used in 3 places, all of them
virtuals. What do you think about moving the storage to SVGAElement,
HTMLAnchorElement, and HTMLFormControlElement? This duplicates a small amount of
code since you will have a virtual setter and getter on Element, and one bool
for each of those classes, but it lets us avoid bloating Element or ElementData.

[tkent, 2014-08-26 01:06:46]

On 2014/08/26 00:50:55, pdr wrote:
> m_shouldHideFocusRingOnMouseFocus is only used in 3 places, all of them
> virtuals. What do you think about moving the storage to SVGAElement,
> HTMLAnchorElement, and HTMLFormControlElement? This duplicates a small amount
of
> code since you will have a virtual setter and getter on Element, and one bool
> for each of those classes, but it lets us avoid bloating Element or
ElementData.

I support this.  ElementData is basically for attribute data storage.
m_shouldHideFocusRingOnMouseFocus shouldn't be in it.

[robwu, 2014-08-26 08:36:27]

On 2014/08/26 01:06:46, tkent wrote:
> On 2014/08/26 00:50:55, pdr wrote:
> > m_shouldHideFocusRingOnMouseFocus is only used in 3 places, all of them
> > virtuals. What do you think about moving the storage to SVGAElement,
> > HTMLAnchorElement, and HTMLFormControlElement? This duplicates a small
amount
> of
> > code since you will have a virtual setter and getter on Element, and one
bool
> > for each of those classes, but it lets us avoid bloating Element or
> ElementData.
> 
> I support this.  ElementData is basically for attribute data storage.
> m_shouldHideFocusRingOnMouseFocus shouldn't be in it.

Ah, back to patch set 3? Or shall I try to use a template instead (see comment
36)?

[pdr., 2014-08-26 20:31:17]

On 2014/08/26 08:36:27, robwu wrote:
> On 2014/08/26 01:06:46, tkent wrote:
> > On 2014/08/26 00:50:55, pdr wrote:
> > > m_shouldHideFocusRingOnMouseFocus is only used in 3 places, all of them
> > > virtuals. What do you think about moving the storage to SVGAElement,
> > > HTMLAnchorElement, and HTMLFormControlElement? This duplicates a small
> amount
> > of
> > > code since you will have a virtual setter and getter on Element, and one
> bool
> > > for each of those classes, but it lets us avoid bloating Element or
> > ElementData.
> > 
> > I support this.  ElementData is basically for attribute data storage.
> > m_shouldHideFocusRingOnMouseFocus shouldn't be in it.
> 
> Ah, back to patch set 3? Or shall I try to use a template instead (see comment
> 36)?

Pretty much patch set 3. The difference is that the logic is all shared on
Element but the storage is duplicated in each of the three classes. Patch set 3
duplicated the logic as well, which I do not think is a good idea.

[robwu, 2014-08-29 23:39:44]

Done. I reverted back to patch set 8, then moved the m_wasFocusedByMouse member
to the three subclasses.

[pdr., 2014-08-30 00:55:54]

On 2014/08/29 23:39:44, robwu wrote:
> Done. I reverted back to patch set 8, then moved the m_wasFocusedByMouse
member
> to the three subclasses.

LGTM

[pdr., 2014-08-30 00:56:04]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-08-30 00:57:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob@robwu.nl/455223002/200001

[commit-bot: I haz the power, 2014-08-30 02:03:33]

Committed patchset #11 (id:200001) as 181127