A focused element should lose focus when it becomes unfocusable.

A focused element should lose focus when it becomes unfocusable.

According to the specification, we should remove focus when a focused element
becomes unforcusable, and Internet Explorer follows it.

We check focusable state after layout asynchronously because setFocusedNode(0)
dispatches synchronous events and callers of Document::updateLayout don't expect
DOM state changes. Also, we introduce m_didPostCheckFocusedNodeTask flag to avoid
duplicated tasks in the pending task queue.

BUG=178146
TEST=automated; This patch adds a layout test.

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

[tkent, 2013-04-23 03:21:48]

I'm not sure if this is the beast way to fix the bug.

[eseidel, 2013-05-09 03:42:29]

I'm really confused what this is trying to do.  This appear to queue a task on
document construction to confirm that the focused node is still visible?  Why is
that the right time?  Is a task the right way to do this?  Should this be a
side-effect of didChangeStyle instead?

[tkent, 2013-05-09 03:46:55]

On 2013/05/09 03:42:29, Eric Seidel wrote:
> I'm really confused what this is trying to do.  This appear to queue a task on
> document construction to confirm that the focused node is still visible?  Why
is
> that the right time?  Is a task the right way to do this?  Should this be a
> side-effect of didChangeStyle instead?

Thank you for the comment.
It's not document construction.  postTask() is in Document::updateLayout.
didChangeStyle is too early because HTMLFormControlElement::isFocusable checks
renderer()->size().isEmpty().  We need layout result.

[eseidel, 2013-05-09 07:24:02]

So I guess the idea is that you want to mark the document as "needing a focus
update" when the layout of a form control changes?

[eseidel, 2013-05-09 07:24:26]

Can we view the "updating the focus" as a separate phase (like painting, layer
construction, etc?)

[tkent, 2013-05-09 21:37:42]

On 2013/05/09 07:24:02, Eric Seidel wrote:
> So I guess the idea is that you want to mark the document as "needing a focus
> update" when the layout of a form control changes?

I think this patch is simpler than introducing such mark.

> Can we view the "updating the focus" as a separate phase (like painting, layer
> construction, etc?)

Yeah, this CL means we had better add such phase in your "Phases of Rendering"
document.

[tkent, 2013-05-20 22:00:31]

eseidel, ojan, do you have any other comments?

[esprehn, 2013-05-20 22:03:44]

Some style nits.

https://codereview.chromium.org/14248006/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/14248006/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:388: class FocusedNodeChecker: public
ScriptExecutionContext::Task {
Missing a space before : and this should be FINAL

https://codereview.chromium.org/14248006/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:397: FocusedNodeChecker() { }
It be nicer if this was called CheckFocusedNodeTask or something of that nature.

[tkent, 2013-05-20 22:29:45]

https://codereview.chromium.org/14248006/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/14248006/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:388: class FocusedNodeChecker: public
ScriptExecutionContext::Task {
On 2013/05/20 22:03:44, esprehn wrote:
> Missing a space before : and this should be FINAL

Done.

https://codereview.chromium.org/14248006/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:397: FocusedNodeChecker() { }
On 2013/05/20 22:03:44, esprehn wrote:
> It be nicer if this was called CheckFocusedNodeTask or something of that
nature.

Done.

[eseidel, 2013-05-22 05:11:48]

lgtm

I don't think this is the right way to fix the bug.  But I don't have a better
suggestion, and I believe the behavior change is correct.

It seems focus-updating should be a post-layout task of some form?  I guess
changing focus causes a style-recalc itself, and thus likely a layout.

[eseidel, 2013-05-22 05:12:55]

These "after this phase, do this task" bits of code are littered throughout
rendering, and always feel wrong.  We seem to need more rendering phase-buckets
that we can sort these things into instead of each of them being some ad hoc
delayed timer.

[tkent, 2013-05-22 05:29:40]

I add a FIXME comment, and will land this ...

[commit-bot: I haz the power, 2013-05-22 05:29:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tkent@chromium.org/14248006/35001

[commit-bot: I haz the power, 2013-05-22 06:48:04]

Change committed as 150870