New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
NOT IN USE
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment
link

Issue 665927: cssom-view/scrollingElement.html test fails even with scrollTopLeftInterop enabled

Reported by rbyers@chromium.org, Nov 16 2016 Project Member

Issue description

Version: 56.0.2920.0
OS: Mac

What steps will reproduce the problem?
(1) Enable chrome://flags#enable-experimental-web-platform-features (in order to get ScrollTopLeftInterop enabled)
(1) Open http://w3c-test.org/cssom-view/scrollingElement.html

What is the expected result?
Expect it to pass

What happens instead?
Quirks mode case fails with:

assert_equals: expected Element node <body style="overflow: scroll;"></body> but got null
    at Test.<anonymous> (http://w3c-test.org/cssom-view/scrollingElement.html:26:5)
    at Test.step (http://w3c-test.org/resources/testharness.js:1406:25)
    at HTMLIFrameElement.<anonymous> (http://w3c-test.org/resources/testharness.js:1430:35)

This passes on Firefox.

I haven't looked in detail, but I suspect our quirks-mode implementation doesn't quite match https://drafts.csswg.org/cssom-view/#dom-document-scrollingelement
 

Comment 1 by rbyers@chromium.org, Nov 16 2016

Cc: sim...@opera.com

Comment 2 by bokan@chromium.org, Nov 17 2016

Owner: chaopeng@chromium.org
Status: Assigned (was: Untriaged)
Here's the relevant rule in Document::scrollingElement:

if (inQuirksMode()) {
      updateStyleAndLayoutTree();
      HTMLBodyElement* body = firstBodyElement();
      if (body && body->layoutObject() &&
          body->layoutObject()->hasOverflowClip())
        return nullptr;

      return body;
}

While the spec says: 

"If the HTML body element exists, and it is not potentially scrollable, return the HTML body element and abort these steps." which translates to:

if (body && !(body->layoubObject() && body->layoutObject()->hasOverflowClip()))
  return body

The body element is only "potentially scrollable" if the root element’s used value of the overflow-x or overflow-y properties is not visible. I think we need to look at the style on documentElement here.

Chao, please take a look when you have some time.

Comment 3 by bokan@chromium.org, Nov 17 2016

Cc: bokan@chromium.org

Comment 4 by dtapu...@chromium.org, May 18 2017

Q2 2017 Update: Issue is still relevant.

Comment 5 by bokan@chromium.org, Jun 8 2017

This has to do with how we check "potentially scrollable". From the spec (https://drafts.csswg.org/cssom-view/#potentially-scrollable):

An element is potentially scrollable if all of the following conditions are true:

- The element has an associated CSS layout box.
- The element is not the HTML body element, or it is and the root element’s used value of the overflow-x or overflow-y properties is not visible.
- The element’s used value of the overflow-x or overflow-y properties is not visible.

So for the body element we need to look at the hasOverflowClip of the root element (i.e. documentElement). In the test, we fail because documentElement.style.overflow becomes visible but we're still returning nullptr. We should be failing the `if`-returning nullptr and falling into the body return;

Comment 6 by dtapu...@chromium.org, Jun 8 2017

Cc: rbyers@chromium.org
So there are two bugs

1) https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.cpp?dr&l=2910 is wrong in that it doesn't check if the document is a HTML document...

2) There seems to be a recalculation issue for propagating the HasOverflowClip bit...

If I add this (dirtying the style on the document) the test passes correctly.
    quirksDoc.body.style = quirksDoc.body.style;

    assert_equals(quirksDoc.scrollingElement, quirksDoc.body);

Comment 7 by dtapu...@chromium.org, Jun 8 2017

Cc: chrishtr@chromium.org e...@chromium.org
eae@, chrishtr@ any idea why HasOverflowClip isn't updated on the body? when a parent's overflow style changes?

I think the sequence is like this:
    quirksDoc.documentElement.style.overflow = "scroll";
    quirksDoc.body.style.overflow = "scroll";
    quirksDoc.documentElement.style.overflow = "visible";
body doesn't have overflow clip here..
but if I add:
    quirksDoc.body.style = quirksDoc.body.style;
Then it does

Comment 8 by chrishtr@chromium.org, Jun 8 2017

Cc: trchen@chromium.org
I don't know, sorry. Adding trchen who might know.

Comment 9 by phistuck@gmail.com, Jun 8 2017

Sounds related to issues  541529 ,  731022  and  590818 .

Comment 10 by phistuck@chromium.org, Jun 8 2017

Labels: Hotlist-Interop

Comment 11 by rbyers@chromium.org, Jun 9 2017

Perhaps this case just isn't handled by the Document::InheritHtmlAndBodyElementStyles logic that tries to correctly propagate the style changes given the crazy rules?

Comment 12 by bugdroid1@chromium.org, Jun 9 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc202b2b1174f5732f7b9e63a6c46fd01ac2be02

commit fc202b2b1174f5732f7b9e63a6c46fd01ac2be02
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Fri Jun 09 13:45:06 2017

Change FirstBodyElement to check the document is a HTML Document.

The CSS spec specifies that the document must be a HTML Document.

Layout test asserts this:
http://w3c-test.org/cssom-view/scrollingElement.html

There is one other user of FirstBodyElement since it was first
added. The background style propagation from the HTML body
element to the HTML element. It appears before it was rewritten
it was checking that the document was a HTML document as well.
So I think this is safe to make the API match the spec.

BUG= 157855 , 665927 

Change-Id: Iafbe7c25011cdde2c89b3d839dcfb08e5bc12c3e
Reviewed-on: https://chromium-review.googlesource.com/527462
Reviewed-by: Rick Byers <rbyers@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478262}
[modify] https://crrev.com/fc202b2b1174f5732f7b9e63a6c46fd01ac2be02/third_party/WebKit/Source/core/dom/Document.cpp

Comment 13 by bokan@chromium.org, Jun 9 2017

+1 to #11, I'd guess that the style code doesn't know (at least in this case) to dirty style that's shared between <html> and <body> on the other element.

Comment 14 by dtapu...@chromium.org, Jun 12 2017

Cc: r...@opera.com
rune@ are you looking at the other style propagation issues for the documentElement?

Currently shipping the scrollTopInterop is blocked on this wpt test failure. Just wondering if I should continue looking at it or you have a fix in the works.

Comment 15 by dtapu...@chromium.org, Jun 12 2017

Cc: chaopeng@chromium.org
Labels: -Pri-3 Pri-2
Owner: r...@opera.com
As per comment #3 dirtying the style in javascript makes this test work. I'm convinced something is odd with the style recalc system.

Assinging to rune@ to investigate/delegate.

Comment 16 by r...@opera.com, Jun 12 2017

Did a quick test and my WIP change doesn't currently fix the w3c test.

Comment 17 by brajkumar@chromium.org, Jun 13 2017

Tested this issue on Ubuntu 14.04, Windows-10 and Mac OS 10.12.5 using chrome latest dev #61.0.3129.0 by following steps mentioned in the original comment. Observed assert_equals still displays fails.

Comment 18 by dtapu...@chromium.org, Jun 16 2017

rune@ are you able to debug this and understand why adjusting the style makes this work? This is a relatively stared feature for interop that we'd like to launch so I'd appreciate it if you could devote some time to it.

If not then I'll have to roll back our intervention for mousewheel which has already landed in ToT because launching the scrollTopLeft interop wouldn't happen shortly.

Comment 19 by rbyers@chromium.org, Jun 16 2017

Maybe this is actually orthogonal from shipping scrollTopLeftInterop?  There's a pre-existing problem here regardless, right?

Comment 20 by r...@opera.com, Jun 19 2017

I'll have a look

Comment 21 by r...@opera.com, Jun 19 2017

There was a spec bug for "potentially scrollable" which I've discussed with simonp@ now, and a spec PR in progress, but that's not directly relevant to the bug, I think.

Comment 22 by r...@opera.com, Jun 19 2017

Based on comment #7, I made this simple case which correctly logs the body element (enabled experimental features):

<!-- quirks -->
<body>
<script>
  document.documentElement.style.overflow = "scroll";
  document.body.style.overflow = "scroll";
  document.offsetTop;
  document.documentElement.style.overflow = "visible";
  console.log(document.scrollingElement);
</script>

so, there must be a different twist in the failing test.

Comment 23 by r...@opera.com, Jun 19 2017

I know what the problem is now, but not sure how to fix this is a pretty way.

UpdateFromStyle() on the layout object sets the HasOverflowClip() flag as part of setting ComputedStyle. That means updating this flag only happens when recalculating style. Since this flag on body depends on the computed style of html, we fail to update the flag on body when html is recalculated but body does not need to be recalculated. Forcing a body recalc does not work because if the style doesn't change, UpdateFromStyle is not called. Checking if ViewportDefiningElement() changes in InheritHtmlAndBodyElementStyles() could work, but the UpdateFromStyle() depends on the computed style being set, while the former method does not do that.

I'll figure something out.

Comment 25 by r...@opera.com, Jun 20 2017

Status: Started (was: Assigned)

Comment 26 by r...@opera.com, Jun 20 2017

Status: Fixed (was: Started)

Comment 27 by dtapu...@chromium.org, Jun 20 2017

rune@ you are awesome! Many thanks for working on this!

Comment 28 by r...@opera.com, Jun 21 2017

thanks. np.

Comment 29 by bugdroid1@chromium.org, Jun 21 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c6703ce6739d5e8725844bbed2c2e938b54e863

commit 1c6703ce6739d5e8725844bbed2c2e938b54e863
Author: Steve Kobes <skobes@chromium.org>
Date: Wed Jun 21 20:21:54 2017

Revert "Update HasOverflowClip for body after style recalc."

This reverts commit 826817e52d02 (http://crrev.com/480824).

Reason for revert: Identified as cause of crash regression in
http://crbug.com/735371.

TBR=rbyers@chromium.org,rune@opera.com,dtapuska@chromium.org
Bug:  665927 , 735371

Change-Id: I1721851eb654e14fb3a697acce1f5c6f0cc2fdcb
Reviewed-on: https://chromium-review.googlesource.com/543540
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481289}
[add] https://crrev.com/1c6703ce6739d5e8725844bbed2c2e938b54e863/third_party/WebKit/LayoutTests/external/wpt/cssom-view/HTMLBody-ScrollArea_quirksmode-expected.txt
[modify] https://crrev.com/1c6703ce6739d5e8725844bbed2c2e938b54e863/third_party/WebKit/LayoutTests/external/wpt/cssom-view/HTMLBody-ScrollArea_quirksmode.html
[add] https://crrev.com/1c6703ce6739d5e8725844bbed2c2e938b54e863/third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollingElement-expected.txt
[modify] https://crrev.com/1c6703ce6739d5e8725844bbed2c2e938b54e863/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/1c6703ce6739d5e8725844bbed2c2e938b54e863/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/1c6703ce6739d5e8725844bbed2c2e938b54e863/third_party/WebKit/Source/core/layout/LayoutBlock.cpp
[modify] https://crrev.com/1c6703ce6739d5e8725844bbed2c2e938b54e863/third_party/WebKit/Source/core/layout/LayoutBlock.h

Comment 30 by skobes@chromium.org, Jun 21 2017

Status: Assigned (was: Fixed)

Comment 31 by r...@opera.com, Jun 22 2017

Reverted due to the crash exposed in attached test case.
overflow.html
207 bytes View Download

Comment 32 by r...@opera.com, Jun 22 2017

Status: Started (was: Assigned)

Comment 33 by bugdroid1@chromium.org, Jun 22 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1edc94d19d773c1459f73cf9ed52b70cf209288e

commit 1edc94d19d773c1459f73cf9ed52b70cf209288e
Author: Rune Lillesveen <rune@opera.com>
Date: Thu Jun 22 21:59:55 2017

Update style for body when viewport defining element changes.

Whether the body box has scrolling overflow or not depends on the
computed overflow of the html element.

The HasOverflowClip flag, along with creating and removing a paint
layer on LayoutBlock is updated as part of SetStyle. However, if the
html element is recalculated, its overflow changing causing the
viewport defining element to change, but body did not need a recalc,
the overflow clip flag and paint layer is not updated for body.

This CL forces a SetStyle on the body LayoutObject to trigger the
necessary update after recalc when when the viewport defining element
changes.

This fixes scrollingElement.html in wpt/cssom-view.

Bug:  665927 
Change-Id: I146c3e976edef28074bde6531fe4c6ec65ecb090
Reviewed-on: https://chromium-review.googlesource.com/544958
Commit-Queue: Rune Lillesveen <rune@opera.com>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Rick Byers <rbyers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481682}
[delete] https://crrev.com/15399f887a0e8498253dc4496446eee83158e96a/third_party/WebKit/LayoutTests/external/wpt/cssom-view/HTMLBody-ScrollArea_quirksmode-expected.txt
[modify] https://crrev.com/1edc94d19d773c1459f73cf9ed52b70cf209288e/third_party/WebKit/LayoutTests/external/wpt/cssom-view/HTMLBody-ScrollArea_quirksmode.html
[delete] https://crrev.com/15399f887a0e8498253dc4496446eee83158e96a/third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollingElement-expected.txt
[add] https://crrev.com/1edc94d19d773c1459f73cf9ed52b70cf209288e/third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollingElement-quirks-dynamic-001-ref.html
[add] https://crrev.com/1edc94d19d773c1459f73cf9ed52b70cf209288e/third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollingElement-quirks-dynamic-001.html
[add] https://crrev.com/1edc94d19d773c1459f73cf9ed52b70cf209288e/third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollingElement-quirks-dynamic-002-ref.html
[add] https://crrev.com/1edc94d19d773c1459f73cf9ed52b70cf209288e/third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollingElement-quirks-dynamic-002.html
[modify] https://crrev.com/1edc94d19d773c1459f73cf9ed52b70cf209288e/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/1edc94d19d773c1459f73cf9ed52b70cf209288e/third_party/WebKit/Source/core/dom/Document.h

Comment 34 by r...@opera.com, Jun 22 2017

Status: Fixed (was: Started)

Sign in to add a comment