Update an Element's ancestor chain at the end of its activation

Update an Element's ancestor chain at the end of its activation

In the course of the an Element being activated, there is a possiblity
of the Element loosing its renderer (display: none being set on :active).
At the end of the activation, the element and its ancestor chain need to
be updated, so that the styles get reapplied. The current code skips the
last part if the active element did not have a renderer.

BUG=139660
TEST=fast/css/clear-activechain-list.html

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

[spartha, 2014-06-12 14:06:06]

The CQ bit was checked by sudarshan.p@samsung.com

[spartha, 2014-06-12 14:06:08]

The CQ bit was unchecked by sudarshan.p@samsung.com

[spartha, 2014-06-13 09:54:14]

PTAL!

[Inactive, 2014-06-13 13:09:25]

On 2014/06/13 11:34:34, spartha wrote:

Please add a "TEST=fast/css/clear-activechain-list.html" line to your CL
description, right after the BUG= line.

[Inactive, 2014-06-13 13:15:33]

https://codereview.chromium.org/317733004/diff/80001/LayoutTests/fast/css/cle...
File LayoutTests/fast/css/clear-activechain-list.html (right):

https://codereview.chromium.org/317733004/diff/80001/LayoutTests/fast/css/cle...
LayoutTests/fast/css/clear-activechain-list.html:21: <pre
id="description"></div>
Why are those 2 <pre>...</pre> needed? AFAIK, the js-test.js script is adding
those for you.

https://codereview.chromium.org/317733004/diff/80001/LayoutTests/fast/css/cle...
LayoutTests/fast/css/clear-activechain-list.html:37: description("Chain of
active elements should be cleared, even if style :active sets display: none on
the current active element");
This can probably be outside the if(window.testRunner).

https://codereview.chromium.org/317733004/diff/80001/LayoutTests/fast/css/cle...
LayoutTests/fast/css/clear-activechain-list.html:39: testRunner.dumpAsText();
This is the default.

[spartha, 2014-06-13 14:28:14]

Thanks!

https://codereview.chromium.org/317733004/diff/80001/LayoutTests/fast/css/cle...
File LayoutTests/fast/css/clear-activechain-list.html (right):

https://codereview.chromium.org/317733004/diff/80001/LayoutTests/fast/css/cle...
LayoutTests/fast/css/clear-activechain-list.html:21: <pre
id="description"></div>
On 2014/06/13 13:15:33, Chris Dumez wrote:
> Why are those 2 <pre>...</pre> needed? AFAIK, the js-test.js script is adding
> those for you.

You are right, js-test.js adds these for us (adds a div). This I picked from a
similar test and had not paid attention. But on checking, the construct is
intentional to avoid adding the additional div. The <style> screws the test if
we don't.

https://codereview.chromium.org/317733004/diff/80001/LayoutTests/fast/css/cle...
LayoutTests/fast/css/clear-activechain-list.html:37: description("Chain of
active elements should be cleared, even if style :active sets display: none on
the current active element");
On 2014/06/13 13:15:33, Chris Dumez wrote:
> This can probably be outside the if(window.testRunner).

Done.

https://codereview.chromium.org/317733004/diff/80001/LayoutTests/fast/css/cle...
LayoutTests/fast/css/clear-activechain-list.html:39: testRunner.dumpAsText();
On 2014/06/13 13:15:33, Chris Dumez wrote:
> This is the default.

Done.

[esprehn, 2014-06-17 08:37:20]

https://codereview.chromium.org/317733004/diff/100001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/317733004/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5461: for (Node* node = oldActiveElement; node;
node = node->parentNode()) {
This is not correct for Shadow DOM. You need to use
NodeRenderingTraversal::parent(). You should also just be able to modify the
loop above to use that. That also doesn't seem quite right though since you
could change the distribution and break the parentNode() chain. Perhaps
::detach() should clear the bits? What does Firefox do here?

[spartha, 2014-06-17 15:07:00]

https://codereview.chromium.org/317733004/diff/100001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/317733004/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5461: for (Node* node = oldActiveElement; node;
node = node->parentNode()) {
On 2014/06/17 08:37:20, esprehn wrote:
> This is not correct for Shadow DOM. You need to use
> NodeRenderingTraversal::parent(). You should also just be able to modify the
> loop above to use that. That also doesn't seem quite right though since you
> could change the distribution and break the parentNode() chain. Perhaps
> ::detach() should clear the bits? What does Firefox do here?
I tried this content http://jsbin.com/fasoyuqi on firefox nightly and Chrome
with my patch. As you have correctly pointed out, using parentNode() fails to
restore the style for the shadow DOM elements. The same content on Firefox
nightly is inconclusive as the shadow DOM implementation is still not very
mature. As far as the behaviour without shadow DOM, Firefox restores the non
active state for all the active chain elements at the end of active state.

Thanks for the insight...

[spartha, 2014-06-18 12:40:10]

https://codereview.chromium.org/317733004/diff/100001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/317733004/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5461: for (Node* node = oldActiveElement; node;
node = node->parentNode()) {
On 2014/06/17 08:37:20, esprehn wrote:
> This is not correct for Shadow DOM. You need to use
> NodeRenderingTraversal::parent(). You should also just be able to modify the
> loop above to use that. That also doesn't seem quite right though since you
> could change the distribution and break the parentNode() chain. Perhaps
> ::detach() should clear the bits? What does Firefox do here?
I have uploaded a patch using NodeRenderingTraversal, PTAL. I quite did not get
the scenario where chaning the distribution could be a problem.

[spartha, 2014-06-23 13:47:29]

On 2014/06/18 12:40:10, spartha wrote:
>
https://codereview.chromium.org/317733004/diff/100001/Source/core/dom/Documen...
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/317733004/diff/100001/Source/core/dom/Documen...
> Source/core/dom/Document.cpp:5461: for (Node* node = oldActiveElement; node;
> node = node->parentNode()) {
> On 2014/06/17 08:37:20, esprehn wrote:
> > This is not correct for Shadow DOM. You need to use
> > NodeRenderingTraversal::parent(). You should also just be able to modify the
> > loop above to use that. That also doesn't seem quite right though since you
> > could change the distribution and break the parentNode() chain. Perhaps
> > ::detach() should clear the bits? What does Firefox do here?
> I have uploaded a patch using NodeRenderingTraversal, PTAL. I quite did not
get
> the scenario where chaning the distribution could be a problem.

Ping for Review!

[esprehn, 2014-06-25 07:47:17]

This looks correct now, but can you add a shadow dom test using
createShadowRoot()?

[spartha, 2014-06-26 06:04:18]

On 2014/06/25 at 07:47:17, esprehn wrote:
> This looks correct now, but can you add a shadow dom test using
createShadowRoot()?

Done

[esprehn, 2014-06-26 06:09:35]

lgtm

[spartha, 2014-06-26 06:14:18]

The CQ bit was checked by sudarshan.p@samsung.com

[commit-bot: I haz the power, 2014-06-26 06:14:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/317733004/140001

[spartha, 2014-06-26 06:22:45]

The CQ bit was unchecked by sudarshan.p@samsung.com

[spartha, 2014-06-26 09:58:09]

The CQ bit was checked by sudarshan.p@samsung.com

[commit-bot: I haz the power, 2014-06-26 09:59:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/317733004/160001

[commit-bot: I haz the power, 2014-06-26 11:31:45]

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/1...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[commit-bot: I haz the power, 2014-06-26 11:39:51]

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

[commit-bot: I haz the power, 2014-06-26 11:39:52]

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

[spartha, 2014-06-26 19:01:36]

https://codereview.chromium.org/317733004/diff/160001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (left):

https://codereview.chromium.org/317733004/diff/160001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5447: ASSERT(!curr->node()->isTextNode());
@morrita, the assert was added in one of your patches. I haven't figured why
this is added. The assert is hit with blink tests. Could you remember why this
was added, and is it still relevant?

[Hajime Morrita, 2014-06-26 19:37:28]

On 2014/06/26 19:01:36, spartha wrote:
>
https://codereview.chromium.org/317733004/diff/160001/Source/core/dom/Documen...
> File Source/core/dom/Document.cpp (left):
> 
>
https://codereview.chromium.org/317733004/diff/160001/Source/core/dom/Documen...
> Source/core/dom/Document.cpp:5447: ASSERT(!curr->node()->isTextNode());
> @morrita, the assert was added in one of your patches. I haven't figured why
> this is added. The assert is hit with blink tests. Could you remember why this
> was added, and is it still relevant?

It replaced if () criteria and I think it still doesn make sense.
The iteration starting point, |oldActiveElement|, is eleemnt. So its parent
shouldn't be able to Text node.
In my understanding,

 - Text nodes are on the leaf of rendering tree and it cannot be a parent of
any.
 - The starting point of the iteration is an Element, not Text.

This is what the ASSERT() says.
If it fails, something I haven't considered happens.

[spartha, 2014-06-27 05:56:15]

On 2014/06/26 19:37:28, morrita wrote:
> On 2014/06/26 19:01:36, spartha wrote:
> >
>
https://codereview.chromium.org/317733004/diff/160001/Source/core/dom/Documen...
> > File Source/core/dom/Document.cpp (left):
> > 
> >
>
https://codereview.chromium.org/317733004/diff/160001/Source/core/dom/Documen...
> > Source/core/dom/Document.cpp:5447: ASSERT(!curr->node()->isTextNode());
> > @morrita, the assert was added in one of your patches. I haven't figured why
> > this is added. The assert is hit with blink tests. Could you remember why
this
> > was added, and is it still relevant?
> 
> It replaced if () criteria and I think it still doesn make sense.
> The iteration starting point, |oldActiveElement|, is eleemnt. So its parent
> shouldn't be able to Text node.
> In my understanding,
> 
>  - Text nodes are on the leaf of rendering tree and it cannot be a parent of
> any.
>  - The starting point of the iteration is an Element, not Text.
> 
> This is what the ASSERT() says.
> If it fails, something I haven't considered happens.

It wasn't failing earlier. It is now, with my patch. There is a similar loop in
the "else" part, unlike here uses an explicit check for the textNode. I see no
difference,  or am I missing something?

[Hajime Morrita, 2014-06-27 17:39:24]

On 2014/06/27 05:56:15, spartha wrote:
> On 2014/06/26 19:37:28, morrita wrote:
> > On 2014/06/26 19:01:36, spartha wrote:
> > >
> >
>
https://codereview.chromium.org/317733004/diff/160001/Source/core/dom/Documen...
> > > File Source/core/dom/Document.cpp (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/317733004/diff/160001/Source/core/dom/Documen...
> > > Source/core/dom/Document.cpp:5447: ASSERT(!curr->node()->isTextNode());
> > > @morrita, the assert was added in one of your patches. I haven't figured
why
> > > this is added. The assert is hit with blink tests. Could you remember why
> this
> > > was added, and is it still relevant?
> > 
> > It replaced if () criteria and I think it still doesn make sense.
> > The iteration starting point, |oldActiveElement|, is eleemnt. So its parent
> > shouldn't be able to Text node.
> > In my understanding,
> > 
> >  - Text nodes are on the leaf of rendering tree and it cannot be a parent of
> > any.
> >  - The starting point of the iteration is an Element, not Text.
> > 
> > This is what the ASSERT() says.
> > If it fails, something I haven't considered happens.
> 

> It wasn't failing earlier. It is now, with my patch. There is a similar loop
in
> the "else" part, unlike here uses an explicit check for the textNode. I see no
> difference,  or am I missing something?
My guess is that the another code path is just a leftover I overlooked at that
time.
Sorry for the confusion :-(

[spartha, 2014-06-27 18:33:30]

On 2014/06/27 17:39:24, morrita wrote:
> On 2014/06/27 05:56:15, spartha wrote:
> > On 2014/06/26 19:37:28, morrita wrote:
> > > On 2014/06/26 19:01:36, spartha wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/317733004/diff/160001/Source/core/dom/Documen...
> > > > File Source/core/dom/Document.cpp (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/317733004/diff/160001/Source/core/dom/Documen...
> > > > Source/core/dom/Document.cpp:5447: ASSERT(!curr->node()->isTextNode());
> > > > @morrita, the assert was added in one of your patches. I haven't figured
> why
> > > > this is added. The assert is hit with blink tests. Could you remember
why
> > this
> > > > was added, and is it still relevant?
> > > 
> > > It replaced if () criteria and I think it still doesn make sense.
> > > The iteration starting point, |oldActiveElement|, is eleemnt. So its
parent
> > > shouldn't be able to Text node.
> > > In my understanding,
> > > 
> > >  - Text nodes are on the leaf of rendering tree and it cannot be a parent
of
> > > any.
> > >  - The starting point of the iteration is an Element, not Text.
> > > 
> > > This is what the ASSERT() says.
> > > If it fails, something I haven't considered happens.
> > 
> 
> > It wasn't failing earlier. It is now, with my patch. There is a similar loop
> in
> > the "else" part, unlike here uses an explicit check for the textNode. I see
no
> > difference,  or am I missing something?
> My guess is that the another code path is just a leftover I overlooked at that
> time.
> Sorry for the confusion :-(
Thanks for the review ... This needs further investigation, as to why it is
asserting let me check

[spartha, 2014-06-30 15:03:50]

The CQ bit was checked by sudarshan.p@samsung.com

[commit-bot: I haz the power, 2014-06-30 15:04:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/317733004/220001

[commit-bot: I haz the power, 2014-06-30 16:54:39]

Change committed as 177215