Implemented get element target algorithm #27299

avr1254 · 2020-07-16T16:09:01Z

Added check for area and anchor element

Finished issue: Implemented get target and no opener algorithm

Implemented get element target and get element noopener algorithms.

Used the algorithms in html spec to implement target and no opener algorithms.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #27253 (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because ___

highfive · 2020-07-16T16:09:11Z

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

highfive · 2020-07-16T16:09:12Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmlformelement.rs, components/script/dom/htmlanchorelement.rs
@KiChjang: components/script/dom/htmlformelement.rs, components/script/dom/htmlanchorelement.rs

highfive · 2020-07-16T16:09:14Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

avr1254 · 2020-07-16T19:52:21Z

I think the error says Incorrect License for a source file. Any idea what that means?

avr1254 · 2020-07-20T15:07:03Z

@jdm I think this looks good. Is there something I'm missing in the code?

jdm · 2020-07-21T01:49:25Z

Sorry, I got distracted from this. I'll review it tomorrow!

jdm

Good start!

components/script/dom/htmlformelement.rs

jdm · 2020-07-21T15:31:21Z

components/script/dom/htmlanchorelement.rs

@@ -581,21 +582,39 @@ pub fn follow_hyperlink(subject: &Element, hyperlink_suffix: Option<String>) {
    let source = document.browsing_context().unwrap();

    // Step 4-5: target attribute.
-    let target_attribute_value = subject.get_attribute(&ns!(), &local_name!("target"));
-
+    let subject_type = subject.get_attribute(&ns!(), &local_name!("target"));


My preference would be to extract this into a separate function that has a comment linking to https://html.spec.whatwg.org/multipage/semantics.html#get-an-element's-target and invoke that, so that this code more clearly follows the specification.

jdm · 2020-07-21T15:31:21Z

components/script/dom/htmlanchorelement.rs

+        if !subject.has_attribute(&local_name!("target")) {
+            let base = (*document).base_element();
+            match base {
+                Some(base) => {
+                    let base_html_unwrap = (*base).upcast::<HTMLElement>().upcast::<Element>();
+                    if base_html_unwrap.has_attribute(&local_name!("target")) {
+                        target_attribute_value =
+                            base_html_unwrap.get_attribute(&ns!(), &local_name!("target"));
+                    } else {
+                        target_attribute_value = None;
+                    }
+                },
+                None => target_attribute_value = None,
+            };
+        }
+    }


With a get_element_target function that returns Option<DOMString>, we could write this as:

if !(subject.is::<HTMLAreaElement>() || subject.is::<HTMLAnchorElement>() || subject.is::<HTMLFormElement>()) { return None; } if subject.has_attribute(&local_name!("target")) { return subject.get_attribute(&ns!(), &local_name!("target")); } document .base_element() .map(|base| base.upcast::<Element>()) .and_then(|element| { if element.has_attribute(&local_name!("target")) { Some(element.get_attribute(&local_name!("target"))) } else { None } })

jdm · 2020-07-21T15:31:21Z

components/script/dom/htmlanchorelement.rs

    let noopener = if let Some(link_types) = subject.get_attribute(&ns!(), &local_name!("rel")) {
        let values = link_types.Value();
-        let contains_noopener = values.contains("noopener");
-        let contains_noreferrer = values.contains("noreferrer");
-        let contains_opener = values.contains("opener");
        let target_is_blank = if let Some(name) = target_attribute_value.as_ref() {
            name.Value().to_lowercase() == "_blank"
        } else {
            false
        };
-
-        contains_noreferrer || contains_noopener || (!contains_opener && target_is_blank)
+        values.contains("noreferrer") ||
+            values.contains("noopener") ||
+            (!values.contains("opener") && target_is_blank)
    } else {
        false
    };


This would also be good to extract into a function that links to https://html.spec.whatwg.org/multipage/links.html#get-an-element's-noopener and splits the string value appropriately (https://html.spec.whatwg.org/multipage/links.html#linkTypes)

jdm · 2020-07-21T15:31:21Z

components/script/dom/htmlformelement.rs

+        let target_attribute_value;
+        let subject = submitter.form_owner().unwrap();
+        if submitter.is_submit_button() && submitter.target() != DOMString::new() {
+            target_attribute_value = action;
+        } else {
+            // the result of getting an element's target given submitter's form owner.
+            if subject
+                .upcast::<HTMLElement>()
+                .upcast::<Element>()
+                .has_attribute(&local_name!("target"))
+            {
+                target_attribute_value = subject
+                    .upcast::<HTMLElement>()
+                    .upcast::<Element>()
+                    .get_attribute(&ns!(), &local_name!("target"))
+                    .unwrap()
+                    .Value();
+            } else {
+                let base =
+                    (*document_from_node(subject.upcast::<HTMLElement>().upcast::<Element>()))
+                        .base_element();
+                match base {
+                    Some(base) => {
+                        let base_html_unwrap = (*base).upcast::<HTMLElement>().upcast::<Element>();
+                        if base_html_unwrap.has_attribute(&local_name!("target")) {
+                            target_attribute_value = (*base_html_unwrap
+                                .get_attribute(&ns!(), &local_name!("target"))
+                                .unwrap())
+                            .Value();
+                        } else {
+                            target_attribute_value = DOMString::new();
+                        }
+                    },
+                    None => target_attribute_value = DOMString::new(),
+                };
+            }
+        }
+
+        // Step 18
+        let noopener = if let Some(form_data) = subject
+            .upcast::<HTMLElement>()
+            .upcast::<Element>()
+            .get_attribute(&ns!(), &local_name!("rel"))
+        {
+            let values = form_data.Value();
+            let target_is_blank: bool =
+                target_attribute_value.to_string().to_lowercase() == "_blank".to_string();
+            values.contains("noreferrer") ||
+                values.contains("noopener") ||
+                (!values.contains("opener") && target_is_blank)
+        } else {
+            false
+        };


Reusing the functions here would be a good improvement.

jdm

Getting close!

jdm · 2020-07-22T02:33:43Z

components/script/dom/htmlanchorelement.rs

+    if (document_from_node(subject).base_element())
+        .unwrap()


We don't want to panic if there is no <base>, so let's do this:

document_from_node(subject) .base_element() .map(|base| base.upcast::<Element>()) .and_then(|element| { if element.has_attribute(&local_name!("target")) { Some(element.get_string_attribute(&local_name!("target"))) } else { None } })

I get an error code E0515 about not being able to return a value referencing function parameter 'base'

@jdm Would using a match statement instead of unwrapping the base_element() value eliminate the chance for a panic?

It would. It may also be possible to do .base_element().as_ref(), or remove the map and do let element = element.upcast::<Element>(); inside of the and_then.

jdm · 2020-07-22T02:33:43Z

components/script/dom/htmlanchorelement.rs

+            subject
+                .get_attribute(&ns!(), &local_name!("target"))
+                .unwrap()
+                .Value(),


Let's use get_string_attribute, which avoids the need to unwrap anything.

jdm · 2020-07-22T02:33:43Z

components/script/dom/htmlanchorelement.rs

+    {
+        return false;
+    }
+    let link_types = subject.get_attribute(&ns!(), &local_name!("rel")).unwrap();


To avoid panicking, let's do:

let link_types = match subject.get_attribute(&ns!(), &local_name!("rel")) { Some(rel) => rel.Value(), None => return false, };

jdm · 2020-07-22T02:33:43Z

components/script/dom/htmlanchorelement.rs

+    let target_is_blank = if let Some(name) = target_attribute_value.as_ref() {
+        name.to_lowercase() == "_blank"
+    } else {
+        false
+    };


let target_is_blank = target_attribute_value.as_ref().map_or(false, |target| target.to_lowercase() == "_blank");

jdm · 2020-07-22T02:33:43Z

components/script/dom/htmlanchorelement.rs

+    if values.contains("noreferrer") ||
+        values.contains("noopener") ||
+        (!values.contains("opener") && target_is_blank)
+    {
+        return true;
+    } else {
+        return false;
+    }


We can return the condition directly; there's no need for the explicit return true or return false here.

jdm · 2020-07-22T02:33:43Z

components/script/dom/htmlanchorelement.rs

+    let mut target_attribute_value = None;
+    if subject.is::<HTMLAreaElement>() || subject.is::<HTMLAnchorElement>() {
+        target_attribute_value = get_element_target(subject);
+    }


Let's do:

let target_attribute_value = if subject.is::<...> ... { get_element_target(subject) } else { None };

jdm · 2020-07-22T02:33:43Z

components/script/dom/htmlformelement.rs

+            target_attribute_value = Some(action);
+        } else {
+            target_attribute_value =
+                get_element_target(&subject.upcast::<HTMLElement>().upcast::<Element>());


&subject.upcast::<Element>()

jdm · 2020-07-22T02:33:43Z

components/script/dom/htmlformelement.rs

+        let target_attribute_value: Option<DOMString>;
+        let subject = submitter.form_owner().unwrap();
+        if submitter.is_submit_button() && submitter.target() != DOMString::new() {
+            target_attribute_value = Some(action);


This doesn't look right - shouldn't this be submitter.target()?

jdm · 2020-07-22T02:33:43Z

components/script/dom/htmlformelement.rs

+
+        // Step 18
+        let noopener = get_element_noopener(
+            &subject.upcast::<HTMLElement>().upcast::<Element>(),


This should beself.upcast::<Element>(), based on my reading of the spec.

Sounds good! I'll get to these in the morning.

bors-servo · 2020-07-24T17:26:46Z

💔 Test failed - status-taskcluster

jdm · 2020-07-24T17:28:55Z

tests/wpt/metadata/html/browsers/windows/opener-closed.html.ini

@@ -0,0 +1,3 @@
+[opener-closed.html]


Aha - this file should be in the auxiliary-browsing-contexts subdirectory of browsers/windows/. That's why it's not being picked up correctly.

@jdm Fixed it!

jdm · 2020-07-24T19:04:14Z

@bors-servo r+

bors-servo · 2020-07-24T19:04:15Z

📌 Commit 135fbb2 has been approved by jdm

avr1254 · 2020-07-24T19:24:25Z

@jdm Thanks for all the help and mentoring on this PR!

jdm · 2020-07-24T19:52:23Z

Thanks for sticking with it!

bors-servo · 2020-07-24T19:54:27Z

⌛ Testing commit 135fbb2 with merge 90c9907...


        Auto merge of #27299 - avr1254:master, r=jdm

Implemented get element target algorithm Added check for area and anchor element Finished issue: Implemented get target and no opener algorithm Implemented get element target and get element noopener algorithms.  Used the algorithms in html spec to implement target and no opener algorithms. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #27253 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-07-24T20:36:23Z

💔 Test failed - status-taskcluster

jdm · 2020-07-24T20:48:03Z

tests/wpt/metadata/html/browsers/windows/auxiliary-browsing-contexts/opener-closed.html.ini

+[opener-closed.html]
+  [An auxiliary browsing context should report `null` for `window.opener` when that browsing context is discarded]
+    expected: TIMEOUT


We're missing a TIMEOUT for the top-level test file, too:

[opener-closed.html] expected: TIMEOUT [An auxiliary browsing context should report `null` for `window.opener` when that browsing context is discarded] expected: TIMEOUT

Fixed it with the latest push!


        Implemented get element target and get element noopener algorithms

and refactored into functions.

jdm · 2020-07-24T21:13:32Z

@bors-servo r+

bors-servo · 2020-07-24T21:13:33Z

📌 Commit 2d5c30d has been approved by jdm

bors-servo · 2020-07-24T21:30:40Z

⌛ Testing commit 2d5c30d with merge b83433f...

bors-servo · 2020-07-24T22:34:14Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing b83433f to master...

highfive assigned SimonSapin Jul 16, 2020

highfive added the S-awaiting-review label Jul 16, 2020

jdm assigned jdm and unassigned SimonSapin Jul 16, 2020

avr1254 force-pushed the avr1254:master branch 3 times, most recently from b262242 to eac6011 Jul 16, 2020

jdm requested changes Jul 21, 2020

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 21, 2020

avr1254 force-pushed the avr1254:master branch from eac6011 to 0b86fd6 Jul 22, 2020

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 22, 2020

avr1254 force-pushed the avr1254:master branch from c9075d2 to 03e7180 Jul 22, 2020

jdm requested changes Jul 22, 2020

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 22, 2020

avr1254 force-pushed the avr1254:master branch from 03e7180 to 6a7607f Jul 22, 2020

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 22, 2020

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 22, 2020

avr1254 force-pushed the avr1254:master branch from 6a7607f to a3fdeda Jul 22, 2020

highfive removed the S-needs-code-changes label Jul 22, 2020

highfive added the S-tests-failed label Jul 24, 2020

jdm reviewed Jul 24, 2020

View changes

avr1254 force-pushed the avr1254:master branch from 37eee14 to 135fbb2 Jul 24, 2020

highfive removed the S-tests-failed label Jul 24, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 24, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 24, 2020

jdm reviewed Jul 24, 2020

View changes

Implemented get element target and get element noopener algorithms

Loading status checks…

2d5c30d

and refactored into functions.

avr1254 force-pushed the avr1254:master branch from 135fbb2 to 2d5c30d Jul 24, 2020

highfive added S-awaiting-review and removed S-tests-failed labels Jul 24, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 24, 2020

bors-servo merged commit b83433f into servo:master Jul 24, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 24, 2020

servo / servo

Join GitHub today

Implemented get element target algorithm #27299

Implemented get element target algorithm #27299

Conversation

avr1254 commented Jul 16, 2020 • edited by atouchet

highfive commented Jul 16, 2020

highfive commented Jul 16, 2020

highfive commented Jul 16, 2020

avr1254 commented Jul 16, 2020

avr1254 commented Jul 20, 2020

jdm commented Jul 21, 2020 • edited

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm left a comment

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.

bors-servo commented Jul 24, 2020

This comment has been minimized.

This comment has been minimized.

jdm commented Jul 24, 2020

bors-servo commented Jul 24, 2020

avr1254 commented Jul 24, 2020

jdm commented Jul 24, 2020

bors-servo commented Jul 24, 2020

bors-servo commented Jul 24, 2020

This comment has been minimized.

This comment has been minimized.

jdm commented Jul 24, 2020

bors-servo commented Jul 24, 2020

bors-servo commented Jul 24, 2020

bors-servo commented Jul 24, 2020

avr1254 commented Jul 16, 2020 •

edited by atouchet

jdm commented Jul 21, 2020 •

edited