Addresses Issue #1716. Indicated part of the document. #9632

peterjoel · 2016-02-14T14:33:19Z

highfive · 2016-02-14T14:33:25Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @mbrubeck (or someone else) soon.

highfive · 2016-02-14T14:33:26Z

Warning

This pull request adds a file without the .ini file extension to tests/wpt/metadata. Please consider removing it!

jdm · 2016-02-14T14:55:30Z

We can ignore the warning from @highfive due to servo/highfive#59.

peterjoel · 2016-02-14T14:57:49Z

Actually @jdm, the highfive warning has reminded me that I didn't add all the tests in the manifest.json. I'll address that now.

peterjoel · 2016-02-14T16:17:43Z

#1716

KiChjang · 2016-02-15T00:35:37Z

components/script/dom/document.rs

-                    .find(|node| check_anchor(&node))
-                    .map(Root::upcast)
-        })
+        if fragid == "" {


Step annotations here would be nice.

KiChjang · 2016-02-15T00:39:46Z

components/script/dom/document.rs

+            String::from_utf8(percent_decode(fragid.as_bytes())).ok()
+                .and_then(|decoded_fragid| self.get_element_by_id(&Atom::from(&*decoded_fragid)))
+                .or_else(|| self.get_anchor_by_name(fragid))
+                .or_else(|| if fragid.to_lowercase() == "top" {


I think or would suffice here.

@KiChjang I don't follow where you mean it could be used.

Instead of or_else, or can be used.

KiChjang · 2016-02-15T01:06:51Z

components/script/dom/document.rs

+            self.GetDocumentElement()
+        } else {
+            use url::percent_encoding::percent_decode;
+            // 3. Let fragid bytes be the result of percent-decoding fragid.


// Step 3 would be enough, we don't need to quote the entire step from the spec.

On that note, where is step 1 and 2?

Step 2 is directly above, on line 520.
Step 1 is about getting the fragid from the url, but the value is already obtained elsewhere and passed in as an argument. I stuck that step above the function declaration to document that it arises elsewhere, since it wouldn't be very helpful at the actual point where it happens (in another file).

In any case, the step annotations still needs to be changed.

KiChjang · 2016-02-15T01:22:16Z

components/script/dom/document.rs

+                // 7. If fragid is an ASCII case-insensitive match for the string top, then the
+                //    indicated part of the document is the top of the document; stop the algorithm
+                //    here.
+                .or_else(|| if fragid.to_lowercase() == "top" {


or_else can be changed into or.

@KiChjang Lazily evaluating those expressions seems better, since quite often the values will never be needed. As written, at most one Some value will be evaluated, and we don't unnecessarily search the document tree for an anchor if we already found an id match. This function probably isn't performance-critical, but I don't think it alters too much in readability either.

I can of course change it if you think it's best.

Hmm, true, never thought about it in the aspect of eager vs lazy evaluation!

KiChjang · 2016-02-15T01:57:51Z

components/script/dom/document.rs

+        let doc_node = self.upcast::<Node>();
+        doc_node.traverse_preorder()
+                .filter_map(Root::downcast)
+                .find(|node| check_anchor(&node))


Would .find(check_anchor) work here?

To be honest, this was existing code that I extracted into its own function, and I didn't look too hard at the details.

A quick attempt tells me that this isn't a straightforward change. I don't fully understand the type-conversions going on there, but I can't see this code getting shorter by going down that path.

KiChjang · 2016-02-15T03:23:02Z

components/script/dom/document.rs

+    }
+
+    fn get_anchor_by_name(&self, name: &str ) -> Option<Root<Element>> {
+        let check_anchor = |node: &HTMLAnchorElement| {


nit: This could be fn check_anchor(node: &HTMLAnchorElement) instead.

Needs to be a closure, or else it can't access name.

KiChjang · 2016-02-15T04:00:10Z

components/script/dom/document.rs

-                    .map(Root::upcast)
-        })
+        // Step 2
+        if fragid == "" {


if fragid.is_empty() {

KiChjang · 2016-02-15T04:00:33Z

components/script/dom/document.rs

-                    .find(|node| check_anchor(&node))
-                    .map(Root::upcast)
-        })
+        // Step 2


We really should leave a note here detailing why Step 1 isn't necessary.

// Step 1 is not handled here; the fragid is already obtained by the calling function

That ok? I think including the name of the file/function that obtains the fragid isn't a good idea, for maintainability reasons. It would be in danger of going out of date if it the other code changed without affecting this location.

Yes, that looks perfect!

KiChjang · 2016-02-15T04:01:29Z

components/script/dom/document.rs

+        if fragid == "" {
+            self.GetDocumentElement()
+        } else {
+            use url::percent_encoding::percent_decode;


Move this to the top of the file.

KiChjang · 2016-02-15T04:02:20Z

components/script/dom/document.rs

+        }
+    }
+
+    fn get_anchor_by_name(&self, name: &str ) -> Option<Root<Element>> {


nit: extra space after &str

KiChjang · 2016-02-15T04:45:56Z

@bors-servo r+

Thanks for working on this!

bors-servo · 2016-02-15T04:45:57Z

📌 Commit 17b51c5 has been approved by KiChjang

peterjoel · 2016-02-15T22:11:23Z

Tests are failing because scrollTop is not yet implemented (#9650) and I can't see another obvious way to detect scrolling - for example getClientRects() does not take scroll position into account yet.

@KiChjang Should I disable the test or should this issue be blocked on #9650?

jdm · 2016-02-15T22:51:32Z

No need to disable the tests, but we can mark that they are expected to fail so we'll be notified when they start passing in the future :)

That being said, I'm not clear on why getClientRects or getBoundingClientRect should be affected by the scroll position; I don't see any language in https://drafts.csswg.org/cssom-view/#dom-element-getclientrects that suggests that.

peterjoel · 2016-02-15T22:58:23Z

@jdm I can't see it in the spec either. However, in 3 browsers that I checked (Firefox, Chrome and Safari), all of them have values that change as you scroll. The value of the rectangle's top is (and I'm not sure why 8) 8 - element.scrollTop.


        #1716 The indicated part of the document.

Interactive test for fragid resolution. Added HTML tests for scrolling to fragid Applied algorithm from whatwg spec https://html.spec.whatwg.org/multipage/#the-indicated-part-of-the-document Changes following code review

KiChjang · 2016-02-17T07:37:19Z

@bors-servo r+

bors-servo · 2016-02-17T07:37:20Z

📌 Commit e39e59e has been approved by KiChjang

bors-servo · 2016-02-17T08:17:23Z

⌛ Testing commit e39e59e with merge 6eb2e8a...


        Auto merge of #9632 - peterjoel:issue_1716, r=KiChjang

Addresses Issue #1716. Indicated part of the document. Fixes #1716.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9632)

bors-servo · 2016-02-17T09:05:25Z

💔 Test failed - linux-rel

KiChjang · 2016-02-17T09:08:58Z

@bors-servo retry

#9120

bors-servo · 2016-02-17T09:53:22Z

⌛ Testing commit e39e59e with merge 8d63eff...


        Auto merge of #9632 - peterjoel:issue_1716, r=KiChjang

Addresses Issue #1716. Indicated part of the document. Fixes #1716.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9632)

bors-servo · 2016-02-17T10:41:16Z

💔 Test failed - linux-rel

KiChjang · 2016-02-17T10:50:13Z

@bors-servo retry

#9120

bors-servo · 2016-02-17T11:36:31Z

⚡ Previous build results for android, gonk, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel...

bors-servo · 2016-02-17T12:24:14Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Feb 14, 2016

jdm mentioned this pull request Feb 14, 2016

Ignore changes to MANIFEST.json in nonini_wpt_meta servo/highfive#59

Closed

peterjoel force-pushed the peterjoel:issue_1716 branch from 56b21c6 to f8c5cce Feb 14, 2016

peterjoel changed the title ~~Issue 1716~~ Addresses Issue #1716. Indicated part of the document. Feb 14, 2016

KiChjang reviewed Feb 15, 2016
View changes

components/script/dom/document.rs Outdated

.find(|node| check_anchor(&node))

.map(Root::upcast)

})

if fragid == "" {

This comment has been minimized.

Sign in to view

KiChjang Feb 15, 2016
Member

Step annotations here would be nice.

KiChjang reviewed Feb 15, 2016
View changes

jdm assigned KiChjang Feb 15, 2016

KiChjang added S-needs-code-changes and removed S-awaiting-review labels Feb 15, 2016

KiChjang reviewed Feb 15, 2016
View changes

peterjoel force-pushed the peterjoel:issue_1716 branch from f3b6e3a to 11debf2 Feb 15, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 15, 2016

KiChjang reviewed Feb 15, 2016
View changes

components/script/dom/document.rs Outdated

.map(Root::upcast)

})

// Step 2

if fragid == "" {

This comment has been minimized.

Sign in to view

KiChjang Feb 15, 2016
Member

if fragid.is_empty() {

KiChjang reviewed Feb 15, 2016
View changes

components/script/dom/document.rs Outdated

if fragid == "" {

self.GetDocumentElement()

} else {

use url::percent_encoding::percent_decode;

This comment has been minimized.

Sign in to view

KiChjang Feb 15, 2016
Member

Move this to the top of the file.

KiChjang reviewed Feb 15, 2016
View changes

components/script/dom/document.rs Outdated

}

}

fn get_anchor_by_name(&self, name: &str ) -> Option<Root<Element>> {

This comment has been minimized.

Sign in to view

KiChjang Feb 15, 2016
Member

nit: extra space after &str

peterjoel force-pushed the peterjoel:issue_1716 branch from bc59a40 to 17b51c5 Feb 15, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 15, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Feb 16, 2016

peterjoel force-pushed the peterjoel:issue_1716 branch from 8edcadc to d74b665 Feb 16, 2016

peterjoel force-pushed the peterjoel:issue_1716 branch from db1c42e to e39e59e Feb 17, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 17, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 17, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Feb 17, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 17, 2016

bors-servo merged commit e39e59e into servo:master Feb 17, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

peterjoel deleted the peterjoel:issue_1716 branch Feb 20, 2016

servo / servo

Join GitHub today

Addresses Issue #1716. Indicated part of the document. #9632

Addresses Issue #1716. Indicated part of the document. #9632

Conversation

peterjoel commented Feb 14, 2016

highfive commented Feb 14, 2016

highfive commented Feb 14, 2016

jdm commented Feb 14, 2016

peterjoel commented Feb 14, 2016

peterjoel commented Feb 14, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Feb 15, 2016

bors-servo commented Feb 15, 2016

peterjoel commented Feb 15, 2016

jdm commented Feb 15, 2016

peterjoel commented Feb 15, 2016

KiChjang commented Feb 17, 2016

bors-servo commented Feb 17, 2016

bors-servo commented Feb 17, 2016

bors-servo commented Feb 17, 2016

KiChjang commented Feb 17, 2016

bors-servo commented Feb 17, 2016

bors-servo commented Feb 17, 2016

KiChjang commented Feb 17, 2016

bors-servo commented Feb 17, 2016

bors-servo commented Feb 17, 2016