Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scroll, SetScrollTop, SetScrollLeft in `element.rs` #19127

Merged
merged 1 commit into from Nov 13, 2017
+157 −5
Merged

scroll, SetScrollTop, SetScrollLeft in `element.rs` #19127

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Loading status checks…

scroll, SetScrollTop, SetScrollLeft in `dom-element-scroll` with test

  • Loading branch information
@tigercosmos
tigercosmos committed Nov 11, 2017
commit 9965f7e2cc0f722acfee5f0710b74a84d1112b54
51 components/script/dom/element.rs
View file
Open in desktop
@@ -363,6 +363,19 @@ impl Element {
!self.overflow_y_is_visible()
}

// https://drafts.csswg.org/cssom-view/#scrolling-box
fn has_scrolling_box(&self) -> bool {
// TODO: scrolling mechanism, such as scrollbar (We don't have scrollbar yet)
// self.has_scrolling_mechanism()
self.overflow_x_is_hidden() ||

This comment has been minimized.

Sign in to view
@emilio

emilio Nov 11, 2017

Member

Hmm, there's a big condition that is missing here, which is:

it overflows its content area and the used value of the overflow-x or overflow-y property is hidden.

So, is there something missing here? If not, where does it get handled? If nowhere, should it? Or should at least be a TODO here?

This comment has been minimized.

Sign in to view
@tigercosmos

tigercosmos Nov 11, 2017 •
edited

Author Collaborator

In my thought, when overflow-x or overflow-y property is hidden, it must it overflows its content area.
Because, when it is not hidden, it cannot overflow anymore.
So in my opinion, overflows its content area is describing used value of the overflow-x or overflow-y property is hidden

This comment has been minimized.

Sign in to view
@emilio

emilio Nov 11, 2017

Member

That's not true, an element can definitely overflow its content area with overflow other than hidden.

See for example:

<div style="width: 100px; height: 100px;">
    <div>Something that takes more than 100px</div>
</div>

But in general the spec doesn't make much sense to me, it seems to me that the element would have a scroll box when the content overflows and the overflow property is not hidden... I cannot make any sense of the spec logic here, maybe @jdm or @SimonSapin can double-check me?

This comment has been minimized.

Sign in to view
@emilio

emilio Nov 11, 2017

Member

Let me take a closer look at the test and other implementations...

This comment has been minimized.

Sign in to view
@emilio

emilio Nov 11, 2017

Member

Ok, I see the intent now, and the "element overflows its content area" bit is pretty important.

For example, in your test, even if overflow is hidden, there's no scrolling box when #scrollables width and height is less than #section1s size.

A test should probably be added for this, and this is basically the has_overflow check if I understand correctly. So you can put it here, I think.

This comment has been minimized.

Sign in to view
@tigercosmos

tigercosmos Nov 11, 2017 •
edited

Author Collaborator

I thought element overflows its content area and has overflow is equal?
The fourth test has tested has_overflow.
@emilio I am not sure if I got your points.

This comment has been minimized.

Sign in to view
@tigercosmos

tigercosmos Nov 12, 2017

Author Collaborator

@emilio Shall I change anything?
self.overflow_y_is_hidden()
}

fn has_overflow(&self) -> bool {

This comment has been minimized.

Sign in to view
@emilio

emilio Nov 11, 2017

Member

This needs a spec link or something? I cannot find its definition anywhere though...

This comment has been minimized.

Sign in to view
@tigercosmos

tigercosmos Nov 11, 2017

Author Collaborator

The document only say or the element has no overflow, and don't give any url to explain it. But I thought the overflow happen when scroll length is longer than client's.

This comment has been minimized.

Sign in to view
@tigercosmos

tigercosmos Nov 11, 2017 •
edited

Author Collaborator

And in the new test which I created, there is tests for spec it.
self.ScrollHeight() > self.ClientHeight() ||
self.ScrollWidth() > self.ClientWidth()
}

// used value of overflow-x is "visible"
fn overflow_x_is_visible(&self) -> bool {
let window = window_from_node(self);
@@ -376,6 +389,20 @@ impl Element {
let overflow_pair = window.overflow_query(self.upcast::<Node>().to_trusted_node_address());
overflow_pair.y == overflow_y::computed_value::T::visible
}

// used value of overflow-x is "hidden"
fn overflow_x_is_hidden(&self) -> bool {
let window = window_from_node(self);
let overflow_pair = window.overflow_query(self.upcast::<Node>().to_trusted_node_address());
overflow_pair.x == overflow_x::computed_value::T::hidden
}

// used value of overflow-y is "hidden"
fn overflow_y_is_hidden(&self) -> bool {
let window = window_from_node(self);
let overflow_pair = window.overflow_query(self.upcast::<Node>().to_trusted_node_address());
overflow_pair.y == overflow_y::computed_value::T::hidden
}
}

#[allow(unsafe_code)]
@@ -1471,7 +1498,13 @@ impl Element {
return;
}

// Step 10 (TODO)
// Step 10
if !self.has_css_layout_box() ||
!self.has_scrolling_box() ||
!self.has_overflow()
{
return;
}

// Step 11
win.scroll_node(node, x, y, behavior);
@@ -1927,7 +1960,13 @@ impl ElementMethods for Element {
return;
}

// Step 10 (TODO)
// Step 10
if !self.has_css_layout_box() ||
!self.has_scrolling_box() ||
!self.has_overflow()
{
return;
}

// Step 11
win.scroll_node(node, self.ScrollLeft(), y, behavior);
@@ -2020,7 +2059,13 @@ impl ElementMethods for Element {
return;
}

// Step 10 (TODO)
// Step 10
if !self.has_css_layout_box() ||
!self.has_scrolling_box() ||
!self.has_overflow()
{
return;
}

// Step 11
win.scroll_node(node, x, self.ScrollTop(), behavior);
2 tests/wpt/metadata/css/css-ui/text-overflow-021.html.ini
View file
Open in desktop

This file was deleted.

10 tests/wpt/mozilla/meta/MANIFEST.json
View file
Open in desktop
{}
]
],
"css/cssom-view/dom-element-scroll.html": [
[
"/_mozilla/css/cssom-view/dom-element-scroll.html",
{}
]
],
"css/empty-keyframes.html": [
[
"/_mozilla/css/empty-keyframes.html",
"16a4dd68da41156fbdd139b4a56547f94ad4dbe7",
"support"
],
"css/cssom-view/dom-element-scroll.html": [
"247b85d5988878a7b27bc9f0f7b817085725c038",
"testharness"
],
"css/data_img_a.html": [
"323bf50369f98ed02acccdd3a5824e38d3f353bf",
"reftest"
99 tests/wpt/mozilla/tests/css/cssom-view/dom-element-scroll.html
View file
Open in desktop
@@ -0,0 +1,99 @@
<!doctype html>
<meta charset="utf-8">
<title>dom-element-scroll tests</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<style>
#section1 {
width: 300px;
height: 500px;
top: 16px;
left: 16px;
border: inset gray 3px;
background: white;
}

#scrollable {
width: 400px;
height: 700px;
background: linear-gradient(135deg, red, blue);
}

#section2 {
width: 300px;
height: 500px;
top: 16px;
left: 16px;
border: inset gray 3px;
background: white;
}

#unscrollable {
width: 200px;
height: 300px;
background: linear-gradient(135deg, red, blue);
}
</style>
<section id="section1">
<div id="scrollable"></div>
</section>
<section id="section2">
<div id="unscrollable"></div>
</section>
<script>
var section1 = document.getElementById("section1");
var section2 = document.getElementById("section2");

test(function () {
// let it be "hidden" to have scrolling box
section1.style.overflow = "hidden";

section1.scroll(50, 60);
assert_equals(section1.scrollLeft, 50, "changed scrollLeft should be 50");
assert_equals(section1.scrollTop, 60, "changed scrollTop should be 60");

section1.scroll(0, 0); // back to the origin
}, "Element test for having scrolling box");

test(function () {
section1.scroll(10, 20);
assert_equals(section1.scrollLeft, 10, "changed scrollLeft should be 10");
assert_equals(section1.scrollTop, 20, "changed scrollTop should be 20");

section1.scroll(0, 0); // back to the origin
}, "Element test for having overflow");

test(function () {
// make it not "hidden" to not have scrolling box
section1.style.overflow = "visible";

section1.scroll(50, 0);
assert_equals(section1.scrollLeft, 0, "changed scrollLeft should be 0");
assert_equals(section1.scrollTop, 0, "changed scrollTop should be 0");

section1.scroll(0, 60);
assert_equals(section1.scrollLeft, 0, "changed scrollLeft should be 0");
assert_equals(section1.scrollTop, 0, "changed scrollTop should be 0");

section1.scroll(50, 60);
assert_equals(section1.scrollLeft, 0, "changed scrollLeft should be 0");
assert_equals(section1.scrollTop, 0, "changed scrollTop should be 0");

section1.scroll(0, 0); // back to the origin
}, "Element test for not having scrolling box");

test(function () {
section2.scroll(0, 20);
assert_equals(section2.scrollLeft, 0, "changed scrollLeft should be 0");
assert_equals(section2.scrollTop, 0, "changed scrollTop should be 0");

section2.scroll(10, 0);
assert_equals(section2.scrollLeft, 0, "changed scrollLeft should be 0");
assert_equals(section2.scrollTop, 0, "changed scrollTop should be 0");

section2.scroll(10, 20);
assert_equals(section2.scrollLeft, 0, "changed scrollLeft should be 0");
assert_equals(section2.scrollTop, 0, "changed scrollTop should be 0");
}, "Element test for not having overflow");

</script>
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.