From 74fa801b03df32e0284bc6a326914e49ecea7e9c Mon Sep 17 00:00:00 2001 From: Pu Xingyu Date: Wed, 23 Nov 2016 20:37:22 +0800 Subject: [PATCH] script: Fix the scroll to top behavior When finding the indicated fragment, do not use the document element to indicate the top of the Document, and when scrolling to the frament and we do not find a element, scrolling the top if the fragment is empty or equal to "top". --- components/script/dom/document.rs | 65 ++++++++----------- tests/wpt/mozilla/meta/MANIFEST.json | 6 ++ .../tests/mozilla/scroll_top_null_target.html | 21 ++++++ 3 files changed, 54 insertions(+), 38 deletions(-) create mode 100644 tests/wpt/mozilla/tests/mozilla/scroll_top_null_target.html diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 0bf77ccf73dc..b4273233c37b 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -580,26 +580,18 @@ impl Document { /// https://html.spec.whatwg.org/multipage/#the-indicated-part-of-the-document pub fn find_fragment_node(&self, fragid: &str) -> Option> { // Step 1 is not handled here; the fragid is already obtained by the calling function - // Step 2 - if fragid.is_empty() { - self.GetDocumentElement() - } else { - // Step 3 & 4 - percent_decode(fragid.as_bytes()).decode_utf8().ok() - // Step 5 - .and_then(|decoded_fragid| self.get_element_by_id(&Atom::from(decoded_fragid))) - // Step 6 - .or_else(|| self.get_anchor_by_name(fragid)) - // Step 7 - .or_else(|| if fragid.eq_ignore_ascii_case("top") { - self.GetDocumentElement() - } else { - // Step 8 - None - }) - } + // Step 2: Simply use None to indicate the top of the document. + // Step 3 & 4 + percent_decode(fragid.as_bytes()).decode_utf8().ok() + // Step 5 + .and_then(|decoded_fragid| self.get_element_by_id(&Atom::from(decoded_fragid))) + // Step 6 + .or_else(|| self.get_anchor_by_name(fragid)) + // Step 7 & 8 } + /// Scroll to the target element, and when we do not find a target + /// and the fragment is empty or "top", scroll to the top. /// https://html.spec.whatwg.org/multipage/#scroll-to-the-fragment-identifier pub fn check_and_scroll_fragment(&self, fragment: &str) { let target = self.find_fragment_node(fragment); @@ -607,30 +599,27 @@ impl Document { // Step 1 self.set_target_element(target.r()); - let point = if fragment.is_empty() || fragment.eq_ignore_ascii_case("top") { + let point = target.r().map(|element| { + // FIXME(#8275, pcwalton): This is pretty bogus when multiple layers are involved. + // Really what needs to happen is that this needs to go through layout to ask which + // layer the element belongs to, and have it send the scroll message to the + // compositor. + let rect = element.upcast::().bounding_content_box(); + + // In order to align with element edges, we snap to unscaled pixel boundaries, since + // the paint thread currently does the same for drawing elements. This is important + // for pages that require pixel perfect scroll positioning for proper display + // (like Acid2). Since we don't have the device pixel ratio here, this might not be + // accurate, but should work as long as the ratio is a whole number. Once #8275 is + // fixed this should actually take into account the real device pixel ratio. + (rect.origin.x.to_nearest_px() as f32, rect.origin.y.to_nearest_px() as f32) + }).or_else(|| if fragment.is_empty() || fragment.eq_ignore_ascii_case("top") { // FIXME(stshine): this should be the origin of the stacking context space, // which may differ under the influence of writing mode. Some((0.0, 0.0)) } else { - target.r().map(|element| { - // FIXME(#8275, pcwalton): This is pretty bogus when multiple layers - // are involved. Really what needs to happen is that this needs to go - // through layout to ask which layer the element belongs to, and have - // it send the scroll message to the compositor. - let rect = element.upcast::().bounding_content_box(); - - // In order to align with element edges, we snap to unscaled pixel - // boundaries, since the paint thread currently does the same for - // drawing elements. This is important for pages that require pixel - // perfect scroll positioning for proper display (like Acid2). Since - // we don't have the device pixel ratio here, this might not be - // accurate, but should work as long as the ratio is a whole number. - // Once #8275 is fixed this should actually take into account the - // real device pixel ratio. - (rect.origin.x.to_nearest_px() as f32, - rect.origin.y.to_nearest_px() as f32) - }) - }; + None + }); if let Some((x, y)) = point { // Step 3 diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index a1b285c21bab..64fcd7c0e98d 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -14690,6 +14690,12 @@ "url": "/_mozilla/mozilla/scrollTo.html" } ], + "mozilla/scroll_top_null_target.html": [ + { + "path": "mozilla/scroll_top_null_target.html", + "url": "/_mozilla/mozilla/scroll_top_null_target.html" + } + ], "mozilla/send-arraybuffer.htm": [ { "path": "mozilla/send-arraybuffer.htm", diff --git a/tests/wpt/mozilla/tests/mozilla/scroll_top_null_target.html b/tests/wpt/mozilla/tests/mozilla/scroll_top_null_target.html new file mode 100644 index 000000000000..512bf34d506a --- /dev/null +++ b/tests/wpt/mozilla/tests/mozilla/scroll_top_null_target.html @@ -0,0 +1,21 @@ + + + + + + + + + + + + +