Node::scroll_area should check if the element is potentially scrollable #10947

dlrobertson · 2016-04-30T19:56:57Z

Node::scroll_area should check if the element is potentially_scrollable for the second condition of line 607

relevant specs:
scrollWidth: Step 5
scrollHeight: Step 5
scrollLeft: Step 7
scrollTop: Step 7
potentially scrollable

broesamle · 2016-05-02T10:15:03Z

I'd like to work on this one.

dlrobertson · 2016-05-03T23:49:54Z

Awesome! Feel free to ping me if you have any questions!

broesamle · 2016-05-04T20:20:59Z

To me /web-platform-tests/cssom-view/scrollingElement.html is a relevant test here or, at least is a good starting point.

For me both subtests I see a fail as expected but with a strange message in the window: `URL.createObjectURL is not a function.

Any further suggestions regarding tests?

jdm · 2016-05-04T20:42:01Z

We don't implement the createObjectURL API yet; that's part of one of the GSoC projects that were accepted for Servo this year.

dlrobertson · 2016-05-04T20:48:05Z

As of this moment Element::ScrollHeight and Element::ScrollWidth are the only places Node::scroll_area is used. If I'm not mistaken neither of these functions are used in test-wpt. The most relevant tests I'm aware of are /css-tests/cssom-view-1_dev/scrollWidthHeight*. You can run them with something like the following.

./mach test-css tests/wpt/css-tests/cssom-view-1_dev/html/scrollWidthHeight*

The outcome of these tests shouldn't change since none of the elements have overflow-(x|y): visible. Perhaps a test should be added for an element with overflow-(x|y): visible?

broesamle · 2016-05-04T22:19:14Z

yes, that was the other candidate... but I need quirk mode and a body element.

It will need the visibility (→pot. srollable), and a different result from the window vs the scrolling area.

Ill try to write sth that would be sensitive to the change...

Martin Br:osamle m-broe.de

-------- Ursprüngliche Nachricht --------
Von: Dan Robertson notifications@github.com
Datum: 04.05.2016 22:48 (GMT+01:00)
An: servo/servo servo@noreply.github.com
Cc: "Martin Br:osamle" info@m-broe.de,Comment comment@noreply.github.com
Betreff: Re: [servo/servo] Node::scroll_area should check if the element is potentially scrollable (#10947)

As of this moment Element::ScrollHeight and Element::ScrollWidth are the only places Node::scroll_area is used. Iif I'm not mistaken neither of these functions are used in test-wpt. The most relevant tests I'm aware of are /css-tests/cssom-view-1_dev/scrollWidthHeight*. You can run them with something like the following.

./mach test-css tests/wpt/css-tests/cssom-view-1_dev/html/scrollWidthHeight*
The outcome of these tests shouldn't change since none of the elements have overflow-(x|y): visible. Perhapse a test should be added for an element with overflow-(x|y): visible?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub

broesamle · 2016-05-12T13:41:17Z

when adding the new test and running ./mach test-css --manifest-update SKIP_TESTS I get a huge difference in 'tests/wpt/metadata-css/MANIFEST.json.'
(despite it is based on a recent version 4d13f9f)

785199 vs 1208311 lines!!

difference starting with items that certainly do not belong to my new test file:

   "items": {
      "reftest": {
        "compositing-1_dev/html/background-blend-mode-gradient-image.htm": [
          {
            "path": "compositing-1_dev/html/background-blend-mode-gradient-image.htm",
            "references": [
              [
                "/compositing-1_dev/html/reference/background-blend-mode-gradient-image-ref.htm",
                "=="
              ]
            ],
            "url": "/compositing-1_dev/html/background-blend-mode-gradient-image.htm"
          }
        ],
        "compositing-1_dev/html/mix-blend-mode-animation.htm": [
          {
            "path": "compositing-1_dev/html/mix-blend-mode-animation.htm",
            "references": [
              [
                "/compositing-1_dev/html/reference/mix-blend-mode-animation-ref.htm",
. . .    
. . .

jdm · 2016-05-12T14:02:32Z

That's a known issue. However, we currently cannot add new tests to the css-tests directory; we only mirror tests from upstream so it would be overwritten. It should be added to tests/wpt/web-platform-tests/cssom-view instead.

broesamle · 2016-05-12T17:15:27Z

maybe I am missing something but this tiny test should pass?!

<!doctype html>
<html>
<script src="/resources/testharness.js" type="text/javascript"></script>
<script src="/resources/testharnessreport.js" type="text/javascript"></script>
<body id="thebody">
</body>
<script>
test(function() {
    document.body.style.overflow = "hidden";
    assert_equals(document.body.style.overflow, "hidden", "Could not set document.body.style.overflow to `hidden`.");
}, "Ensure that style.overflow can be set properly");
</script>
</html>

it tells me: expected "hidden" but got "hidden hidden"

broesamle · 2016-05-12T17:17:02Z

Why would it double the hidden in the overflow property?

jdm · 2016-05-12T17:37:48Z

Probably because of a bug in our CSS serialization code in cssstyledeclaration.rs .

broesamle · 2016-05-12T21:38:17Z

To me it looks like #9307 is addressing the issue. At least it is related.
It would make sense to fix the style setting issue first, since testing for this one would be difficult without it.
Another small example demonstrating the issue:

<!doctype html>
<html>
<body id="thebody">
</body>
<script>
    console.log("A "+document.body.style.overflow);
    document.body.style.overflow = "hidden";
    console.log("B "+document.body.style.overflow);
    console.log("C "+document.body.getAttribute('style'));
    document.body.setAttribute('style','height:30px');
    console.log("D "+document.body.getAttribute('style'));
</script>
</html>

Output is:

A 
B hidden hidden
C null
D height:30px

jdm · 2016-05-25T15:36:08Z

Now that #11377 merged, is the output improved?

broesamle · 2016-05-25T19:35:52Z

thats what i hope too, ill check tomorrow.

But I think it is more related to the test i have in mind.

Martin Br:osamle m-broe.de

-------- Ursprüngliche Nachricht --------
Von: Josh Matthews notifications@github.com
Datum: 25.05.2016 17:45 (GMT+01:00)
An: servo/servo servo@noreply.github.com
Cc: "Martin Br:osamle" info@m-broe.de,Comment comment@noreply.github.com
Betreff: Re: [servo/servo] Node::scroll_area should check if the element is potentially scrollable (#10947)

Now that #11377 merged, is the output improved?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub

broesamle · 2016-05-26T18:58:32Z

With a freshly build version of this morning I still get the style attribute doubled, as before. :-/
This attribute issue looks like a separate thing. I did check for other 'suspisious' issues in this respect but did not find any. Shall I open one?

Fixing this (#10947) in itself should only need half a line of code change but its hard to come up with a test for it as long as setting/getting the style attrib does not work properly.

jdm · 2016-05-26T18:59:28Z

Yes, please do file a separate issue for the attribute one.

broesamle · 2016-05-26T19:21:53Z

here it is #11448

broesamle · 2016-05-28T20:48:16Z

Having learned what shorthands are I could now avoid them in the testing.
Does it make sense to create a PR and review the test already?
broesamle@364f808

broesamle · 2016-06-09T15:44:09Z

currently I am struggeling with my natural english of not and or:

how is the following phrase in the spec are meant?

The element’s used value of the overflow-x or overflow-y properties is not visible.

a) both of them have to be visible in order to make this true

b) it is enough if one is visible
(in the sense of: EITHER overflow-x is not visible OR overflow-Y is not visible(OR both are not))

a) overflow-x==visible

jdm · 2016-06-09T15:52:19Z

The sentence is considered true if:

the used value for overflow-x is not visible OR
the used value for overflow-y is not visible

broesamle · 2016-06-09T15:55:41Z

thank you for the quick reply ... then I have to figure out something else to be wrong -- which is good to know
:-)

broesamle · 2016-06-09T20:09:59Z

during adding the additional condition I find that potentially_scrollable always returns true on the example HTML file, which in Firefox provokes the two different behaviours defined in the specs for scrollHeight.

Since I now scrambled the logic gatters of my brain back and forth quite a few times I suggest making a PR for the test, review it and then continue on fixing the bug in the servo code. This would ensure that I do not co-evolve a double-negate test and error logic.

jdm · 2016-06-09T21:14:50Z

That sounds like a sensible choice :)


        Tests for on body element in quiks mode.

see servo#10947 Node::scroll_area should check if the element is potentially scrollable Three tests use `document.scrollingElement` which was not supported at the time of writing. They are, hence, expected to FAIL. The last expected FAIL should be fixed by servo#10947.


        Tests for scroll_area on body element in quirks mode.

see servo#10947 Node::scroll_area should check if the element is potentially scrollable Three tests use `document.scrollingElement` which was not supported at the time of writing. They are, hence, expected to FAIL. The last expected FAIL should be fixed by servo#10947.


        Tests for scroll_area on body element in quirks mode.

see servo#10947 Node::scroll_area should check if the element is potentially scrollable Three tests use `document.scrollingElement` which was not supported at the time of writing. They are, hence, expected to FAIL. The last expected FAIL should be fixed by servo#10947.


        Tests for scroll_area on body element in quirks mode.

see servo#10947 Node::scroll_area should check if the element is potentially scrollable Three tests use `document.scrollingElement` which was not supported at the time of writing. They are, hence, expected to FAIL. The last expected FAIL should be fixed by servo#10947. Edited some comments, message texts.


        Auto merge of #11699 - broesamle:issue10947_Test-body-scroll-quirksA,…

… r=izgzhen Tests for scroll_area on body element in quirks mode.  ---  - [X ] `./mach build -d` does not report any errors - [ X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [X ] There are tests for these changes OR - [ ] These changes do not require tests because _____ see #10947 Node::scroll_area should check if the element is potentially scrollable Three tests use `document.scrollingElement` which was not supported at the time of writing. They are, hence, expected to FAIL. The last expected FAIL should be fixed by #10947.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11699)

broesamle · 2016-06-29T19:31:49Z

It looks like overflow_x_is_visible and/or overflow_y_is_visible are not carrying the right values.

Any hints for logging the overflow properties are welcome.

I tried ./target/debug/servo -Z dump-flow-tree ...

should I find something like overflow=Visible instead pf loads of overflow=Overflow here; or somewhere else?

When running ./target/debug/servo -Z dump-display-list ... on the same file I get a 404 error in the servo window!

izgzhen · 2016-06-30T01:00:42Z

@broesamle Do you mean element::overflow_x_is_visible? I experimented with it a bit and looks like it is truly problematic. Also, doc.is_fully_active in element::scroll is not working as I intended neither.

jdm added the A-content/dom label Apr 30, 2016

KiChjang added the C-assigned label May 2, 2016

broesamle mentioned this issue May 26, 2016

body.style.overflow set/get doubles attribute's value #11448

Closed

broesamle mentioned this issue Jun 10, 2016

Tests for scroll_area on body element in quirks mode. #11699

Merged

0 of 2 tasks complete

servo / servo

Node::scroll_area should check if the element is potentially scrollable #10947

Node::scroll_area should check if the element is potentially scrollable #10947

dlrobertson commented Apr 30, 2016

broesamle commented May 2, 2016

dlrobertson commented May 3, 2016

broesamle commented May 4, 2016

jdm commented May 4, 2016

dlrobertson commented May 4, 2016 •

edited

broesamle commented May 4, 2016

broesamle commented May 12, 2016

jdm commented May 12, 2016

broesamle commented May 12, 2016 •

edited

broesamle commented May 12, 2016

jdm commented May 12, 2016

broesamle commented May 12, 2016

jdm commented May 25, 2016 •

edited

broesamle commented May 25, 2016

broesamle commented May 26, 2016 •

edited

jdm commented May 26, 2016

broesamle commented May 26, 2016

broesamle commented May 28, 2016

broesamle commented Jun 9, 2016

jdm commented Jun 9, 2016 •

edited

broesamle commented Jun 9, 2016

broesamle commented Jun 9, 2016

jdm commented Jun 9, 2016

broesamle commented Jun 29, 2016

izgzhen commented Jun 30, 2016

servo / servo

Join GitHub today

Node::scroll_area should check if the element is potentially scrollable #10947

Node::scroll_area should check if the element is potentially scrollable #10947

Comments

dlrobertson commented Apr 30, 2016

broesamle commented May 2, 2016

dlrobertson commented May 3, 2016

broesamle commented May 4, 2016

jdm commented May 4, 2016

dlrobertson commented May 4, 2016 • edited

broesamle commented May 4, 2016

broesamle commented May 12, 2016

jdm commented May 12, 2016

broesamle commented May 12, 2016 • edited

broesamle commented May 12, 2016

jdm commented May 12, 2016

broesamle commented May 12, 2016

jdm commented May 25, 2016 • edited

broesamle commented May 25, 2016

broesamle commented May 26, 2016 • edited

jdm commented May 26, 2016

broesamle commented May 26, 2016

broesamle commented May 28, 2016

broesamle commented Jun 9, 2016

jdm commented Jun 9, 2016 • edited

broesamle commented Jun 9, 2016

broesamle commented Jun 9, 2016

jdm commented Jun 9, 2016

broesamle commented Jun 29, 2016

izgzhen commented Jun 30, 2016

dlrobertson commented May 4, 2016 •

edited

broesamle commented May 12, 2016 •

edited

jdm commented May 25, 2016 •

edited

broesamle commented May 26, 2016 •

edited

jdm commented Jun 9, 2016 •

edited