New issue
Advanced search Search tips

Issue 768406 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

AffectedByHover not always properly set on display:contents

Reported by r...@opera.com, Sep 25 2017

Issue description

If we recalculate the computed style of a display:contents element, and the ComputedStyle doesn't change, but the AffectedBy* bits changed because other selectors are matched, we don't store the new ComputedStyle which means we don't store a ComputedStyle with the AffectedBy* flags correctly set.

See attached demo.

 
hover.html
229 bytes View Download

Comment 1 by shend@chromium.org, Sep 25 2017

Labels: Hotlist-Interop Update-Monthly
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5b37246c8f89310cb5f8d94c385627b6b596a0c6

commit 5b37246c8f89310cb5f8d94c385627b6b596a0c6
Author: Rune Lillesveen <rune@opera.com>
Date: Tue Sep 26 04:43:44 2017

Store new ComputedStyle object if style did not change.

The comparison of ComputedStyle does not take additional flags into
account. For instance, the AffectedBy* flags used for updating pseudo
classes like :hover. We used to call SetStyleInternal, but this was
removed because the previous comment said it was because of style
sharing which is now removed.

The display:contents case (768406) never worked because the code path
for StoreNonLayoutObjectComputedStyle() was always skipped when
computed style compared to be equal.

Bug:  768406 ,  767832 
Change-Id: Iac4708e3cd3a6451d99c1bb2bb69efb74289b8eb
Reviewed-on: https://chromium-review.googlesource.com/681755
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: Rune Lillesveen <rune@opera.com>
Cr-Commit-Position: refs/heads/master@{#504285}
[add] https://crrev.com/5b37246c8f89310cb5f8d94c385627b6b596a0c6/third_party/WebKit/LayoutTests/fast/dynamic/hover-after-affected-by-change.html
[modify] https://crrev.com/5b37246c8f89310cb5f8d94c385627b6b596a0c6/third_party/WebKit/Source/core/dom/Element.cpp

Comment 3 by r...@opera.com, Sep 26 2017

Status: Fixed (was: Started)
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ac19ac76519aaa0e4ffd228629d4488d5efea79

commit 8ac19ac76519aaa0e4ffd228629d4488d5efea79
Author: Marc Treib <treib@chromium.org>
Date: Tue Sep 26 08:18:11 2017

Revert "Store new ComputedStyle object if style did not change."

This reverts commit 5b37246c8f89310cb5f8d94c385627b6b596a0c6.

Reason for revert: hover-after-affected-by-change.html consistently fails on WebKit Linux Trusty Leak: https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linux%20Trusty%20Leak

Original change's description:
> Store new ComputedStyle object if style did not change.
> 
> The comparison of ComputedStyle does not take additional flags into
> account. For instance, the AffectedBy* flags used for updating pseudo
> classes like :hover. We used to call SetStyleInternal, but this was
> removed because the previous comment said it was because of style
> sharing which is now removed.
> 
> The display:contents case (768406) never worked because the code path
> for StoreNonLayoutObjectComputedStyle() was always skipped when
> computed style compared to be equal.
> 
> Bug:  768406 ,  767832 
> Change-Id: Iac4708e3cd3a6451d99c1bb2bb69efb74289b8eb
> Reviewed-on: https://chromium-review.googlesource.com/681755
> Reviewed-by: nainar <nainar@chromium.org>
> Commit-Queue: Rune Lillesveen <rune@opera.com>
> Cr-Commit-Position: refs/heads/master@{#504285}

TBR=rune@opera.com,nainar@chromium.org

Change-Id: I34a54b694bb719b278426f9a76076fa77cd9e82f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  768406 ,  767832 
Reviewed-on: https://chromium-review.googlesource.com/684275
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504313}
[delete] https://crrev.com/a3f3699bedf7166984c5c4664f673eecb7486aaa/third_party/WebKit/LayoutTests/fast/dynamic/hover-after-affected-by-change.html
[modify] https://crrev.com/8ac19ac76519aaa0e4ffd228629d4488d5efea79/third_party/WebKit/Source/core/dom/Element.cpp

Comment 5 by r...@opera.com, Sep 26 2017

Status: Started (was: Fixed)
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9f0c480da9cb188c82e16e6c6751875a60a114cf

commit 9f0c480da9cb188c82e16e6c6751875a60a114cf
Author: Rune Lillesveen <rune@opera.com>
Date: Wed Sep 27 02:59:59 2017

Reland "Store new ComputedStyle object if style did not change."

This is a reland of https://chromium-review.googlesource.com/681755

The comparison of ComputedStyle does not take additional flags into
account. For instance, the AffectedBy* flags used for updating pseudo
classes like :hover. We used to call SetStyleInternal, but this was
removed because the previous comment said it was because of style
sharing which is now removed.

The display:contents case (768406) never worked because the code path
for StoreNonLayoutObjectComputedStyle() was always skipped when
computed style compared to be equal.

TBR=nainar@chromium.org,treib@chromium.org

Bug:  768406 ,  767832 ,  768790 
Change-Id: Iee509e43e5fcc29d04944655d3d966ea61cc2adf
Reviewed-on: https://chromium-review.googlesource.com/684186
Commit-Queue: Rune Lillesveen <rune@opera.com>
Reviewed-by: Rune Lillesveen <rune@opera.com>
Cr-Commit-Position: refs/heads/master@{#504561}
[add] https://crrev.com/9f0c480da9cb188c82e16e6c6751875a60a114cf/third_party/WebKit/LayoutTests/fast/dynamic/hover-after-affected-by-change.html
[modify] https://crrev.com/9f0c480da9cb188c82e16e6c6751875a60a114cf/third_party/WebKit/Source/core/dom/Element.cpp

Comment 7 by r...@opera.com, Sep 27 2017

Status: Fixed (was: Started)
Cc: futhark@chromium.org
Owner: ----
Status: Available (was: Fixed)
Re-opened due to revert:

https://crrev.com/a1bff011ab3fc8f42feb1d27f796cabfc2eef7da
Labels: -Update-Monthly
Status: Started (was: Available)
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/394e62d9a30231f2d4a226e5e2f585ec36397b81

commit 394e62d9a30231f2d4a226e5e2f585ec36397b81
Author: Rune Lillesveen <futhark@chromium.org>
Date: Fri Jan 19 10:14:49 2018

Use new ComputedStyle instance for kNoChange.

Even if the computed style doesn't change, AffectedBy* flags may have
changed, which means we need to set the current ComputedStyle to the new
instance. We can however skip any invalidation diffs.

This is a re-land of the revert of this functionality which did not make
us recover from the memory regression reported in 771294.

Bug:  768406 ,  771294 
Change-Id: Icc7eeee8a982eed53243d52d04d45e313d79a10d
Reviewed-on: https://chromium-review.googlesource.com/870391
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530468}
[add] https://crrev.com/394e62d9a30231f2d4a226e5e2f585ec36397b81/third_party/WebKit/LayoutTests/external/wpt/css/selectors/hover-002-manual.html
[add] https://crrev.com/394e62d9a30231f2d4a226e5e2f585ec36397b81/third_party/WebKit/LayoutTests/external/wpt_automation/css/selectors/hover-002-manual-automation.js
[modify] https://crrev.com/394e62d9a30231f2d4a226e5e2f585ec36397b81/third_party/WebKit/LayoutTests/resources/testharnessreport.js
[modify] https://crrev.com/394e62d9a30231f2d4a226e5e2f585ec36397b81/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/394e62d9a30231f2d4a226e5e2f585ec36397b81/third_party/WebKit/Source/core/layout/LayoutObject.cpp
[modify] https://crrev.com/394e62d9a30231f2d4a226e5e2f585ec36397b81/third_party/WebKit/Source/core/style/ComputedStyle.h

Status: Fixed (was: Started)

Sign in to add a comment