Issue 18298016: updateStyleForNodeIfNeeded should be Shadow DOM aware

Issue 18298016: updateStyleForNodeIfNeeded should be Shadow DOM aware (Closed)

Created:
7 years, 5 months ago by esprehn

Modified:
7 years, 5 months ago

Reviewers:
eseidel, tonyg, ojan

CC:
blink-reviews, aandrey+blink_chromium.org, dglazkov+blink, Rik, eae+blinkwatch, adamk+blink_chromium.org

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

updateStyleForNodeIfNeeded should be Shadow DOM aware updateStyleForNodeIfNeeded was not traversing through shadow hosts and was being used as a fast path for Element::offsetWidth which meant that a style change on your host or any of their ancestors would not get applied in some cases if requiresLayoutToDetermineWidth was false. This was a regression caused by http://src.chromium.org/viewvc/blink?revision=153074&view=revision R=tonyg@chromium.org,eseidel@chromium.org,ojan@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154203

Patch Set 1 #

Patch Set 2 : Add a test #

Patch Set 3 : Var names #

Created: 7 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+34 lines, -2 lines)			Patch
A	LayoutTests/fast/dom/shadow/offsetWidth-host-style-change.html	View	1	1 chunk	+21 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/shadow/offsetWidth-host-style-change-expected.txt	View	1	1 chunk	+11 lines, -0 lines	0 comments	Download
M	Source/core/dom/Document.cpp	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

ojan

lgtm On Sun, Jul 14, 2013 at 10:00 PM, <esprehn@chromium.org> wrote: > Reviewers: tonyg, eseidel, ...

7 years, 5 months ago (2013-07-15 05:24:32 UTC) #2

lgtm


On Sun, Jul 14, 2013 at 10:00 PM, <esprehn@chromium.org> wrote:

> Reviewers: tonyg, eseidel, ojan,
>
> Description:
> updateStyleForNodeIfNeeded should be Shadow DOM aware
>
> updateStyleForNodeIfNeeded was not traversing through shadow hosts and was
> being
> used
> as a fast path for Element::offsetWidth which meant that a style change on
> your
> host or any
> of their ancestors would not get applied in some cases if
> requiresLayoutToDetermineWidth
> was false.
>
> This was a regression caused by
>
http://src.chromium.org/**viewvc/blink?revision=153074&**view=revision<http:/...
>
> R=tonyg@chromium.org,eseidel@**chromium.org <eseidel@chromium.org>,
> ojan@chromium.org
>
> Please review this at
https://codereview.chromium.**org/18298016/<https://codereview.chromium.org/1...
>
> SVN Base:
svn://svn.chromium.org/blink/**trunk<http://svn.chromium.org/blink/trunk>
>
> Affected files:
>   A LayoutTests/fast/dom/shadow/**offsetWidth-host-style-change-**
> expected.txt
>   A LayoutTests/fast/dom/shadow/**offsetWidth-host-style-change.**html
>   M Source/core/dom/Document.cpp
>
>
> Index: LayoutTests/fast/dom/shadow/**offsetWidth-host-style-change-**
> expected.txt
> diff --git a/LayoutTests/fast/dom/shadow/**offsetWidth-host-style-change-*
> *expected.txt b/LayoutTests/fast/dom/shadow/**
> offsetWidth-host-style-change-**expected.txt
> new file mode 100644
> index 000000000000000000000000000000**0000000000..**
> 67a5303e59dcb13d2a713de2651906**063d9e228b
> --- /dev/null
> +++ b/LayoutTests/fast/dom/shadow/**offsetWidth-host-style-change-**
> expected.txt
> @@ -0,0 +1,11 @@
> +offsetWidth of a fixed width element should cause a style recalc if host
> styles are invalid
> +
> +On success, you will see a series of "PASS" messages, followed by "TEST
> COMPLETE".
> +
> +
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +PASS div.offsetWidth is 100
> +PASS div.offsetWidth is 0
> +
> Index: LayoutTests/fast/dom/shadow/**offsetWidth-host-style-change.**html
> diff --git a/LayoutTests/fast/dom/shadow/**offsetWidth-host-style-change.*
> *html b/LayoutTests/fast/dom/shadow/**offsetWidth-host-style-change.**html
> new file mode 100644
> index 000000000000000000000000000000**0000000000..**
> d9a3d276032c042eb591ebd1e749ab**1b9ca956ef
> --- /dev/null
> +++ b/LayoutTests/fast/dom/shadow/**offsetWidth-host-style-change.**html
> @@ -0,0 +1,21 @@
> +<!DOCTYPE html>
> +
> +<script src="../../js/resources/js-**test-pre.js"></script>
> +
> +<div id="host"></div>
> +
> +<script>
> +description('offsetWidth of a fixed width element should cause a style
> recalc if host styles are invalid');
> +
> +onload = function() {
> +    host = document.getElementById('host'**);
> +    shadowRoot = host.webkitCreateShadowRoot();
> +    div = shadowRoot.appendChild(**document.createElement('div'))**;
> +    div.style.width = '100px';
> +    shouldBe('div.offsetWidth', '100');
> +    host.style.display = 'none';
> +    shouldBe('div.offsetWidth', '0');
> +};
> +</script>
> +
> +<script src="../../js/resources/js-**test-post.js"></script>
> \ No newline at end of file
> Index: Source/core/dom/Document.cpp
> diff --git a/Source/core/dom/Document.cpp b/Source/core/dom/Document.cpp
> index 95f472d66f5410d5b3092bf470ae64**2040180b90..**
> e2c88836b5212c8da0c1e612c69d5d**d79b0989e1 100644
> --- a/Source/core/dom/Document.cpp
> +++ b/Source/core/dom/Document.cpp
> @@ -1698,7 +1698,7 @@ void Document::**updateStyleForNodeIfNeeded(**Node*
> node)
>          return;
>
>      bool needsStyleRecalc = hasPendingForcedStyleRecalc();
> -    for (Node* n = node; n && !needsStyleRecalc; n = n->parentNode())
> +    for (Node* n = node; n && !needsStyleRecalc; n =
> n->parentOrShadowHostNode())
>          needsStyleRecalc = n->needsStyleRecalc();
>      if (needsStyleRecalc)
>          updateStyleIfNeeded();
>
>
>

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/esprehn@chromium.org/18298016/1001

7 years, 5 months ago (2013-07-15 05:31:02 UTC) #3

commit-bot: I haz the power

Failed to trigger a try job on linux_layout_rel HTTP Error 400: Bad Request

7 years, 5 months ago (2013-07-15 05:48:17 UTC) #4

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/esprehn@chromium.org/18298016/12001

7 years, 5 months ago (2013-07-15 05:48:31 UTC) #5

tonyg

7 years, 5 months ago (2013-07-15 15:43:20 UTC) #7

Message was sent while issue was closed.

On 2013/07/15 07:08:10, I haz the power (commit-bot) wrote:
> Change committed as 154203

lgtm -- thanks for the fix!

Expand Messages | Collapse Messages