Add a test to verify finding fragment id in non UTF8 document #24412
Conversation
wpt-pr-bot
requested review from
annevk,
domenic,
foolip,
jdm,
jgraham,
zcorpan and
zqzhang
| <div id="塯" style="position:absolute; top:100px;"></div> | ||
| <div style="height:200em;"></div> | ||
| <script> | ||
| var steps = [{ |
There was a problem hiding this comment.
It looks like this style is copied from sibling tests which have more than one step. With just one step, I find it quite hard to follow with all the indirection.
Can you try flattening it into a simple single-page test, be something like this?
setup({ single_test: true });
assert_equals(location.hash, "", "Page must be loaded with no hash");
location.hash = '%89g';
setTimeout(() => {
assert_equals( document.scrollingElement.scrollTop, 0 );
done();
}, 1);I think that's all the test is doing, but I haven't tested my suggestion.
There was a problem hiding this comment.
Thanks! I tested it and it works, so I pushed that new version. Initially I thought perhaps we will add more subtests so I kept the multiple tests approach, but I guess we can introduce it later if needed.
rwlbuis
force-pushed
the
scroll_frag_non_utf8
branch
from
8c3870d
to
3aaafb8
Compare
This is related to https://html.spec.whatwg.org/multipage/browsing-the-web.html#scroll-to-fragid:the-indicated-part-of-the-document-6. See also whatwg/html#2902. In general this comes from my fix in https://bugs.webkit.org/show_bug.cgi?id=83930, i.e. make scroll frag id behavior according to spec in WebKit. I think the change is a bit older but WebKit did not get around to fixing it. The fix works but we found out the test anchor-frames-gbk.html would have a different behavior (since it uses a non UTF8 document), hence this test which should replace it (anchor-frames-gbk.html is used in chromium too btw). |
| <div style="height:200em;"></div> | ||
| <script> | ||
| setup({ single_test: true }); | ||
| assert_equals(location.hash, "", "Page must be loaded with no hash"); |
There was a problem hiding this comment.
Maybe also assert that document.characterSet is GBK?
| <body> | ||
| <div></div> | ||
| <div id="塯" style="position:absolute; top:100px;"></div> | ||
| <div style="height:200em;"></div> |
There was a problem hiding this comment.
Don't you want 200vh or some such?
There was a problem hiding this comment.
Sure, done. BTW scroll-frag-percent-encoded.html uses 200em.
| setup({ single_test: true }); | ||
| assert_equals(location.hash, "", "Page must be loaded with no hash"); | ||
| location.hash = '%89g'; | ||
| setTimeout(() => { |
There was a problem hiding this comment.
I'm surprised this passes the lint check. Using an async_test wrapper and t.step_timeout seems slightly preferable?
There was a problem hiding this comment.
Not sure if a lint checker can catch this. Anyway, fixed.
There was a problem hiding this comment.
As far as I know unless this directory is in lint.ignore (I guess it might be given your other comment) the literal string setTimeout will trip it.
There was a problem hiding this comment.
It's because lint.ignore has SET TIMEOUT: html/browsers/browsing-the-web/scroll-to-fragid/*.
rwlbuis
force-pushed
the
scroll_frag_non_utf8
branch
from
3aaafb8
to
1c0259c
Compare
No description provided.