Fix broken AttachContext from r152289

Fix broken AttachContext from r152289

Implemented changes requested by esprehn as a result of https://chromiumcodereview.appspot.com/16599003

Changes made:

1. The style was overwritten due to the subtle difference between webkit and blink, now it's ok.
2. Now the context is properly propagated to the element's children and to Shadow DOM elements.
3. Now the context is properly set on the lazyReattach method.

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

[esprehn, 2013-06-13 18:20:42]

In the future please make much smaller changes. The style reuse and the hover
fix should have been separate patches so we could have caught this stuff sooner.


Also when uploading a patch it needs a one line description as the subject, not
"Patch". I fixed yours.

https://chromiumcodereview.appspot.com/16951003/diff/1/Source/core/dom/Contai...
File Source/core/dom/ContainerNode.h (right):

https://chromiumcodereview.appspot.com/16951003/diff/1/Source/core/dom/Contai...
Source/core/dom/ContainerNode.h:203: childrenContext.resolvedStyle = 0;
You should put this inside the constructor to just not copy the style and remove
all the comments.

https://chromiumcodereview.appspot.com/16951003/diff/1/Source/core/dom/Contai...
Source/core/dom/ContainerNode.h:234: AttachContext childrenContext(context);
Please remove all the comments here, this should be a one liner to initialize
the context.

https://chromiumcodereview.appspot.com/16951003/diff/1/Source/core/dom/Elemen...
File Source/core/dom/Element.cpp (right):

https://chromiumcodereview.appspot.com/16951003/diff/1/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:1322: shadow->detach();
You don't correctly handle ShadowRoots, you need to pass the context down
through shadow->detach and ->attach. Otherwise a :hover around a thing with a
shadow will fail the same as my test case.

[stavila, 2013-06-13 21:10:05]

https://chromiumcodereview.appspot.com/16951003/diff/1/Source/core/dom/Contai...
File Source/core/dom/ContainerNode.h (right):

https://chromiumcodereview.appspot.com/16951003/diff/1/Source/core/dom/Contai...
Source/core/dom/ContainerNode.h:203: childrenContext.resolvedStyle = 0;
On 2013/06/13 18:20:42, esprehn wrote:
> You should put this inside the constructor to just not copy the style and
remove
> all the comments.
You mean the copy constructor should set by default the resolvedStyle member to
0? Or are you saying you would like another parameter to the constructor,
telling it to not copy the style?

If you prefer changing the default behaviour, please keep in mind that the
normal copy constructor is used in the Node::reattach method, so in that case
the style would have to be manually copied after using the copy constructor,
which seems kinda wrong.

What do you think?

[esprehn, 2013-06-13 21:54:06]

On 2013/06/13 21:10:05, stavila wrote:
> ...
> 
> If you prefer changing the default behaviour, please keep in mind that the
> normal copy constructor is used in the Node::reattach method, so in that case
> the style would have to be manually copied after using the copy constructor,
> which seems kinda wrong.
> 
> What do you think?

I think you should make reattach take a RenderStyle instead of an AttachContext.
The copy constructor should not copy the resolvedStyle.

[stavila, 2013-06-13 22:40:47]

On 2013/06/13 21:54:06, esprehn wrote:
> On 2013/06/13 21:10:05, stavila wrote:
> > ...
> > 
> > If you prefer changing the default behaviour, please keep in mind that the
> > normal copy constructor is used in the Node::reattach method, so in that
case
> > the style would have to be manually copied after using the copy constructor,
> > which seems kinda wrong.
> > 
> > What do you think?
> 
> I think you should make reattach take a RenderStyle instead of an
AttachContext.
> The copy constructor should not copy the resolvedStyle.

Are you sure about that? That sort of defeats the entire purpose of having a
context, which was to be able in the future to add more attach/detach settings
without adding any more parameters to existing methods. Also, I think it would
be pretty messy to have attach and detach take a context and for reattach to
only take the style, don't you think?

[esprehn, 2013-06-13 23:09:36]

On 2013/06/13 22:40:47, stavila wrote:
> On 2013/06/13 21:54:06, esprehn wrote:
> > On 2013/06/13 21:10:05, stavila wrote:
> > > ...
> > > 
> > > If you prefer changing the default behaviour, please keep in mind that the
> > > normal copy constructor is used in the Node::reattach method, so in that
> case
> > > the style would have to be manually copied after using the copy
constructor,
> > > which seems kinda wrong.
> > > 
> > > What do you think?
> > 
> > I think you should make reattach take a RenderStyle instead of an
> AttachContext.
> > The copy constructor should not copy the resolvedStyle.
> 
> Are you sure about that? That sort of defeats the entire purpose of having a
> context, which was to be able in the future to add more attach/detach settings
> without adding any more parameters to existing methods. Also, I think it would
> be pretty messy to have attach and detach take a context and for reattach to
> only take the style, don't you think?

If you want to do that I think you want to fix lazyReattachifAttached to take an
AttachContext too then. I don't want us to litter the code with duplicate
explainer comments, your API should be clear enough you don't need to do that.
If you want you can just remove them for now and we can sort this out later.

Please upload a new patch with that and the Shadow DOM fixes.

[stavila, 2013-06-14 12:03:07]

Please review latest patch.

[stavila, 2013-06-14 14:56:47]

On 2013/06/14 12:03:07, stavila wrote:
> Please review latest patch.

DON'T COMMIT YET. There's something I want to check first.

[esprehn, 2013-06-14 22:25:27]

LGTM with a nit. What do you need to check?

https://codereview.chromium.org/16951003/diff/9001/Source/core/dom/shadow/Ele...
File Source/core/dom/shadow/ElementShadow.h (right):

https://codereview.chromium.org/16951003/diff/9001/Source/core/dom/shadow/Ele...
Source/core/dom/shadow/ElementShadow.h:62: void detach(const
Node::AttachContext& = Node::AttachContext());
I don't think you need the default values. We should never call this without
passing the host's attach context.

[stavila, 2013-06-16 10:16:25]

THE PROBLEM:
	- this patch breakes a test (fast/css/hover-update.html). Basically, that test
has an element (let's call it element A) that calls a js function when hovered.
Inside the js function, the element's display is set to none. As a result of
this, the element disappears and some other element (let's call it element B)
(which was initially lower in the page) ends up under the mouse pointer. The
purpose of this test is to ensure that element B also gets the hovered state.
After applying this patch, this doesn't happen until another mouseMove event is
triggered.

THE REASON FOR THE PROBLEM:
	- in the test mentioned above, element A also has a child element (let's call
it element A2). As part of the reattach procedure (due to the change of display
type), the AttachContext was not passed down to element A's children, so A2 did
not "know" in its detach() method that this is actually a reattach procedure
(context.performingReattach was false). As such, it would notify the document
that the hovered node is detaching, by calling Document::hoveredNodeDetached.
This in turn ends up calling EventHandler::scheduleHoverStateUpdate which
basically does just what it says, it schedules a new hit testing procedure (with
a timeout of 0 ms) which will start the entire process again and thus move
element B to its hovered stated.
	- this patch as you probably know passes the context down to the children. As
such, element A2 now is aware of the fact that this is a reattach procedure, and
it no longer notifies the document that the hovered element detached. As a
result, no additional hit-testing is started and element B will not have it's
hovered state triggered until another mouseMove event arrives.

THE IMMEDIATE SOLUTION:
	- the immediate solution I thought of was to detect the moment when something
like that happens (for instance, after recomputing the style of all elements
needing recomputing, check if the currently hovered element still has a
renderer) and manually trigger a hover state update by calling
EventHandler::scheduleHoverStateUpdate. This fixes the problem and correctly
moves element B to its hovered state, as expected.

WHY THE IMMEDIATE SOLUTION DOESN'T ENTIRELY WORK:
	- the solution described above fixes the problem in the hover-update.html test.
However, imagine a box that sets its display to none inside its :hover style
(fast/css/hover-display-block-none.html). This means that, in contrast to the
situation presented above, after the :hover state of the element is no longer
active, the element's display returns to its normal state (say block for
instance). So, by applying the solution above, this would happen: the box's
display would be set to none, it would be detached, it will not be attached
again (because no renderer is required for display:none), the solution described
above will detect that and trigger another hover state update. This will bring
our box back to the normal state, set its display to block and re-create its
renderer. The problem is that this all happens without the user seeing anything.
To the user it would seem like hovering that box does nothing, because the box
is not actually repainted in its hovered state. What we would want in this case
is, as the mouse is moved around over the box, to obtain that on/off flicker.

PROPOSED SOLUTION:
	- the solution I came up with (and which works for all the cases I could think
of) is basically the same solution as the "immediate" solution described above,
except that the hover state update should not be scheduled immediatelly as it
was (with a timeout of 0ms), but after a small period of say 50ms, which would
result in the flicker you would normally expect to see when setting the :hover
style of an element to change its display from block to none.


Does that sound good to you?

[stavila, 2013-06-17 08:48:39]

I found a better way than using the 50ms timeout, there's already a method that
does that, EventHandler::dispatchFakeMouseMoveEventSoon(). I'll upload the new
patch and you can look over it.

[stavila, 2013-06-17 12:41:33]

Uploaded final patch, please look over it and let me know if I can go ahead and
commit it.

[esprehn, 2013-06-17 18:43:40]

This looks fine, please fix comments before landing.

https://chromiumcodereview.appspot.com/16951003/diff/34001/Source/core/dom/Co...
File Source/core/dom/ContainerNode.cpp (right):

https://chromiumcodereview.appspot.com/16951003/diff/34001/Source/core/dom/Co...
Source/core/dom/ContainerNode.cpp:904: if (!over && !renderer()) {
This seems wrong, now if I call setHovered(false) on an element that was never
hovered to begin with it'll schedule a style recalc?

https://chromiumcodereview.appspot.com/16951003/diff/34001/Source/core/dom/Do...
File Source/core/dom/Document.cpp (right):

https://chromiumcodereview.appspot.com/16951003/diff/34001/Source/core/dom/Do...
Source/core/dom/Document.cpp:5062: for (Node* oldHoveredCurr =
oldHoverNode.get(); oldHoveredCurr; oldHoveredCurr =
oldHoveredCurr->parentNode()) {
Remove "curr", please don't add abbreviations. "node" is probably fine,

[stavila, 2013-06-17 18:50:25]

https://chromiumcodereview.appspot.com/16951003/diff/34001/Source/core/dom/Co...
File Source/core/dom/ContainerNode.cpp (right):

https://chromiumcodereview.appspot.com/16951003/diff/34001/Source/core/dom/Co...
Source/core/dom/ContainerNode.cpp:904: if (!over && !renderer()) {
On 2013/06/17 18:43:40, esprehn wrote:
> This seems wrong, now if I call setHovered(false) on an element that was never
> hovered to begin with it'll schedule a style recalc?
Only if that element has no renderer, which should only happen in special cases,
such as this.

https://chromiumcodereview.appspot.com/16951003/diff/34001/Source/core/dom/Do...
File Source/core/dom/Document.cpp (right):

https://chromiumcodereview.appspot.com/16951003/diff/34001/Source/core/dom/Do...
Source/core/dom/Document.cpp:5062: for (Node* oldHoveredCurr =
oldHoverNode.get(); oldHoveredCurr; oldHoveredCurr =
oldHoveredCurr->parentNode()) {
On 2013/06/17 18:43:40, esprehn wrote:
> Remove "curr", please don't add abbreviations. "node" is probably fine,

OK.

[commit-bot: I haz the power, 2013-06-17 19:54:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stavila@adobe.com/16951003/39002

[commit-bot: I haz the power, 2013-06-17 20:47:22]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-06-17 21:10:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stavila@adobe.com/16951003/39002

[esprehn, 2013-06-17 22:00:27]

https://chromiumcodereview.appspot.com/16951003/diff/39002/Source/core/dom/Do...
File Source/core/dom/Document.cpp (right):

https://chromiumcodereview.appspot.com/16951003/diff/39002/Source/core/dom/Do...
Source/core/dom/Document.cpp:1780:
frame()->eventHandler()->dispatchFakeMouseMoveEventSoon();
Hmm, there's a number of other places where we might call Document::recalcStyle
(in this file, and perhaps elsewhere). I don't think this is right to check just
here. Did you mean to check inside Document::recalcStyle at the end?

[stavila, 2013-06-18 08:54:00]

https://chromiumcodereview.appspot.com/16951003/diff/39002/Source/core/dom/Do...
File Source/core/dom/Document.cpp (right):

https://chromiumcodereview.appspot.com/16951003/diff/39002/Source/core/dom/Do...
Source/core/dom/Document.cpp:1780:
frame()->eventHandler()->dispatchFakeMouseMoveEventSoon();
On 2013/06/17 22:00:27, esprehn wrote:
> Hmm, there's a number of other places where we might call
Document::recalcStyle
> (in this file, and perhaps elsewhere). I don't think this is right to check
just
> here. Did you mean to check inside Document::recalcStyle at the end?

Yes, I know Document::recalcStyle is called in multiple places. The most
'correct' place would have been to add that code at the end of the
Document::updateHoverActiveState, after calling Document::updateStyleIfNeeded.
However, this only needs to happen if style updating is 'needed', so I moved
inside Document::updateStyleIfNeeded, after we decided that we do need to
recalcStyle. While it is true that moving it to recalcStyle would not change the
behaviour (everything would still work properly), it would generate unnecessary
work for the CPU, as the current implementation (inside updateStyleIfNeeded)
gets called less often and fully covers the hover situation.

What do you think?

[commit-bot: I haz the power, 2013-06-18 18:49:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stavila@adobe.com/16951003/63001

[commit-bot: I haz the power, 2013-06-18 20:33:30]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-06-18 20:41:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stavila@adobe.com/16951003/63001

[commit-bot: I haz the power, 2013-06-18 21:42:36]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[esprehn, 2013-06-18 21:55:11]

All bots consistently show input-file-re-render.html being broken, it might be
you. :)

[stavila, 2013-06-18 21:58:32]

On 2013/06/18 21:55:11, esprehn wrote:
> All bots consistently show input-file-re-render.html being broken, it might be
> you. :)

I'll check it tomorrow but I don't think it's related.

[commit-bot: I haz the power, 2013-06-18 22:04:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stavila@adobe.com/16951003/63001

[commit-bot: I haz the power, 2013-06-18 23:41:02]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-06-19 08:55:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stavila@adobe.com/16951003/16002

[commit-bot: I haz the power, 2013-06-19 11:59:02]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[Tommy Widenflycht, 2013-06-20 11:56:22]

On 2013/06/18 21:58:32, stavila wrote:
> On 2013/06/18 21:55:11, esprehn wrote:
> > All bots consistently show input-file-re-render.html being broken, it might
be
> > you. :)
> 
> I'll check it tomorrow but I don't think it's related.

Yup, it's your patch alright unfortunately.  I tested on linux release w. static
linking as the trybot.

[stavila, 2013-06-20 14:16:29]

Ok, I found the problem. This is what happens in that test:

1. Two files are "dragged"
2. The mouse is moved over the 'select file' control
3. Mouse up (which drops the files onto the 'select file' control)
4. The 'select file' control is detached (display: none)
5. A layout is forced
6. The 'select file' control is attached (display: inline-block)

WITHOUT the patch, the control is reattached and it has the normal style, not
the hovered style (although the mouse pointer is on top of it). WITH the patch,
the control is reattached and it has the hover style, as it should. Since this
is a png test, this fails because on windows and linux, the 'select file'
control is slightly different when hovered (on linux a shade appears around it,
on windows the background colour changes). No idea why it only failed on
release, I tested it locally (on windows) and it failed on both debug and
release, as normal. Tommy as I understood from my talk with him on irc also got
failures on both debug & release, on linux. 

There are multiple solutions to this problem:
- update the expected png files to contain the hovered status of the 'select
file' control - who can do that?
- modify the test, as this is not really a hover test, by moving the mouse
pointer away from it before starting the detach/layout/attach procedure - I can
do that, it's the simplest solution.

What do you think, Elliott?

[esprehn, 2013-06-20 18:22:23]

On 2013/06/20 14:16:29, stavila wrote:
> Ok, I found the problem. This is what happens in that test:
> 
> 1. Two files are "dragged"
> 2. The mouse is moved over the 'select file' control
> 3. Mouse up (which drops the files onto the 'select file' control)
> 4. The 'select file' control is detached (display: none)
> 5. A layout is forced
> 6. The 'select file' control is attached (display: inline-block)
> 
> WITHOUT the patch, the control is reattached and it has the normal style, not
> the hovered style (although the mouse pointer is on top of it). WITH the
patch,
> the control is reattached and it has the hover style, as it should. Since this
> is a png test, this fails because on windows and linux, the 'select file'
> control is slightly different when hovered (on linux a shade appears around
it,
> on windows the background colour changes). No idea why it only failed on
> release, I tested it locally (on windows) and it failed on both debug and
> release, as normal. Tommy as I understood from my talk with him on irc also
got
> failures on both debug & release, on linux. 
> 
> There are multiple solutions to this problem:
> - update the expected png files to contain the hovered status of the 'select
> file' control - who can do that?
> - modify the test, as this is not really a hover test, by moving the mouse
> pointer away from it before starting the detach/layout/attach procedure - I
can
> do that, it's the simplest solution.
> 
> What do you think, Elliott?

I'm okay with either. Moving the mouse away (provided you add a comment) seems
fine since I think you already added a test for the :hover bug you fixed. You
could also mark the test as Failing in TestExpectations, land this patch, and
then rebaseline the test from the bots. That's the standard rebaseline procedure
for pixel tests.

[commit-bot: I haz the power, 2013-06-20 19:03:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stavila@adobe.com/16951003/106001

[commit-bot: I haz the power, 2013-06-20 20:59:05]

Change committed as 152822

[ojan, 2013-06-23 17:58:11]

https://chromiumcodereview.appspot.com/16951003/diff/106001/Source/core/dom/D...
File Source/core/dom/Document.cpp (right):

https://chromiumcodereview.appspot.com/16951003/diff/106001/Source/core/dom/D...
Source/core/dom/Document.cpp:1770:
frame()->eventHandler()->dispatchFakeMouseMoveEventSoon();
To my surprised, firing the fake mouse move events (not just updating the hover
state) seems to match gecko. Mind adding an explicit test case for that as well,
i.e. put the mouse over an element, then move the element and see that the
mouseout event fires)? I could imagine someone coming in to this code later and
deciding to just update hover state without firing the mouse events.

[stavila, 2013-06-26 10:09:51]

On 2013/06/23 17:58:11, ojan wrote:
>
https://chromiumcodereview.appspot.com/16951003/diff/106001/Source/core/dom/D...
> File Source/core/dom/Document.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/16951003/diff/106001/Source/core/dom/D...
> Source/core/dom/Document.cpp:1770:
> frame()->eventHandler()->dispatchFakeMouseMoveEventSoon();
> To my surprised, firing the fake mouse move events (not just updating the
hover
> state) seems to match gecko. Mind adding an explicit test case for that as
well,
> i.e. put the mouse over an element, then move the element and see that the
> mouseout event fires)? I could imagine someone coming in to this code later
and
> deciding to just update hover state without firing the mouse events.

I believe I already covered that in the hover-update.html test by checking the
status of element 'a'.