simple_scroll_to_fragment.html is now a perma-failure

[jdm]

This regressed sometime on November 13, because there were no failures reported prior to that date, and since then there are >60 failures every day. This wasn't noticed because it's marked as intermittent at #14606.

[jdm]

This regression was caused by #19127. cc @tigercosmos

[jdm]

See ./mach test-wpt tests/wpt/mozilla/tests/mozilla/simple_scroll_to_fragment.html to reproduce.

[tigercosmos]

I will check for it tomorrow.

[tigercosmos]

This is what I saw on Firefox

simple_scroll_to_fragment.html

simple_scroll_to_fragment_ref.html

Looks like it is the test problem?
@jdm How do you think?

[jdm]

It does seem like the test was incorrect, but the change #19127 made no green appear at all.

[tigercosmos]

The test want to test if two show as the same, but we can see they are not equal.
Or the red area(scroll bar maybe) will not be counted in testing?

[jdm]

You are correct that the test does not pass in Firefox. However in Servo we have a layout bug that makes the test pass (which we should fix in another issue). #19127 made the test not pass in Servo, in a way that is very different from the way that it does not pass in Firefox. Does that make sense?

[tigercosmos]

I see. very weird.
Assume the wrong is right...

[tigercosmos]

what #19127 did is function scrollTop

simple_scroll_to_fragment.html has a line:

 document.getElementById("scroller").scrollTop = 100;

I would add a new test to check out whether scrollTop will return 100.
If yes, that would be layout problem; if not, it is really caused by #19127

[tigercosmos]

From

servo/components/script/dom/element.rs

Lines 366 to 376 in b2b51d3

	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() ||
	self.overflow_y_is_hidden()
	}
	fn has_overflow(&self) -> bool {
	self.ScrollHeight() > self.ClientHeight() ||
	self.ScrollWidth() > self.ClientWidth()
	}

Change to this will pass the test.

    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() ||
        self.overflow_y_is_hidden() ||
        self.overflow_x_is_scroll() ||
        self.overflow_y_is_scroll()
    }

    fn has_overflow(&self) -> bool {
        self.ScrollHeight() >= self.ClientHeight() ||
        self.ScrollWidth() >= self.ClientWidth()
    }

Although I have find a workaround for this issue, I think the most important reason is from #19280

[tigercosmos]

@highfive assign me

[highfive]

Hey @tigercosmos! Thanks for your interest in working on this issue. It's now assigned to you!

[tigercosmos]

test for this web-platform-tests/wpt#8409

[tigercosmos]

this one should be continue after #19280 done and web-platform-tests/wpt#8409 done.

[mrobinson]

Here's a related cssom-view issue: w3c/csswg-drafts#1526

[tigercosmos]

@highfive: unassigned me.
Cannot solve this in short term.

[mrobinson]

I wonder if it makes sense to back out the original PR for the moment, since it breaks scrolling from script?

[tigercosmos]

The origin PR did fix many tests. This is the only case that failed. Beside, this issue is caused by #19280 more precisely.

[tigercosmos]

In fact, I think padding, margin, overflow all have some incomplete logic, and will cause scrolling wrong, too. I believe #19466, #19477 are related to this assuming.

[mrobinson]

@tigercosmos In that case, perhaps the best thing to do here is to remove the code for "Step 10" for SetScrollTop and SetScrollLeft in order to allow scrolling from script to work, but also continuing to have the tests pass. What do you think?

[tigercosmos]

scrolling from script

Do you mean code in simple_scroll_to_fragment.html?

In my opinion, "step 10" is followed by the SPEC to implement, and I thought this part is correct.
The key makes SetScrollTop wrong, which is in the test, is caused by scrollHeight getter, which influences "step 10".
So, removing the code in "step 10" doesn't solve the issue, and we should fix scrollHeight getter first. However, padding, margin, overflow are incomplete currently, makes fixing scrollHeight more difficult.

In other word, if we remove code, we just avoid the problem, not fix the problem.

[mrobinson]

@tigercosmos Another alternative is to remove all the new steps from SetScrollTop. In addition to the issue you mention, w3c/csswg-drafts#1526 also causes scrolling from script to fail for nodes with overflow:scroll.

[tigercosmos]

@mrobinson I agree with you, that overflow:scroll might be a potential reason why this test failed, too. So now we have more than one factors to influence this test.

Rather than removing the code, I would prefer do the following.
(We can remove code now, but we still need to fix them in the future.)

Some work we need to do:

• fix scrollHeight, which influences has_overflow()
• fix has_scrolling_box() in step 10. Should consider not only hidden due to w3c/csswg-drafts#1526

servo/components/script/dom/element.rs

Lines 1963 to 1969 in 7a87337

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

[mrobinson]

@tigercosmos Since you say it is going to take a while to fix the issue, I think it makes sense to remove the code now in order to ensure that Servo can scroll from script. I can open a PR for this.

[tigercosmos]

Sure. Once the test pass, this issue will close.
I will open a new issue to mention that some work above still need to do.