Implement GetElementRect webdriver command: #8623 #9708

dlrobertson · 2016-02-20T02:22:23Z

Implement the webdriver Get Element Rect command. Originally I wrote out the algorithm for Step 7 and then I found GetBoundingClientRect, and i thought it was probably best to use it instead.

As always, feedback is very welcomed!
#8623

highfive · 2016-02-20T02:22:28Z

Warning

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

KiChjang · 2016-02-20T02:41:00Z

components/webdriver_server/lib.rs

@@ -816,7 +834,7 @@ impl WebDriverHandler<ServoExtensionRoute> for Handler {
                    ServoExtensionCommand::SetPrefs(ref x) => self.handle_set_prefs(x),
                    ServoExtensionCommand::ResetPrefs(ref x) => self.handle_reset_prefs(x),
                }
-            }
+            },


The comma here isn't necessary when the match-arm uses braces.

jdm · 2016-02-20T03:09:25Z

If we fixed #9651, GetBoundingClientRect would become inappropriate for this, I believe.

dlrobertson · 2016-02-20T04:00:03Z

If we fixed #9651, GetBoundingClientRect would become inappropriate for this, I believe.

Ah, I see... So would it be better to go back to the written out algorithm?

jdm · 2016-02-20T17:12:43Z

I think so, yes.

dlrobertson · 2016-02-20T20:30:22Z

As stated in #9709, I think I'm wrong on my implementation of find_element_by_element_id. Step 3 in the spec mentions that the element should be found by UUID, so I think I should be using find_node_by_unique_id

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions.

components/script/webdriver_handlers.rs, line 227 [r2] (raw file):
webdriver::ElementRectResponse uses f64, so I just make the conversion here.

components/webdriver_server/lib.rs, line 837 [r1] (raw file):
Fixed. Thanks!

Comments from the review on Reviewable.io

KiChjang · 2016-02-21T18:18:25Z

components/script/webdriver_handlers.rs

+                    let mut y: i32 = 0;
+                    // Step 2
+                    let mut offset_parent = html_elem.GetOffsetParent();
+                    while offset_parent.is_some() {


This can be while let Some(element) = offset_parent {.

KiChjang · 2016-02-21T18:19:28Z

components/script/webdriver_handlers.rs

+                    // Step 2
+                    let mut offset_parent = html_elem.GetOffsetParent();
+                    while offset_parent.is_some() {
+                        offset_parent = match offset_parent.unwrap().downcast::<HTMLElement>() {


offset_parent = offset_parent.downcast::<HTMLElement>().map(|elem| {

dlrobertson · 2016-02-21T19:10:21Z

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions.

components/script/webdriver_handlers.rs, line 210 [r2] (raw file):
Much cleaner 😄 i don't know why i didn't do that.

components/script/webdriver_handlers.rs, line 211 [r2] (raw file):
Nice!

Comments from the review on Reviewable.io

dlrobertson · 2016-02-21T20:10:37Z

Review status: 0 of 4 files reviewed at latest revision, 5 unresolved discussions.

components/script/webdriver_handlers.rs, line 40 [r4] (raw file):
After looking at this it seemed like this should use find. So I changed this in a separate commit

Comments from the review on Reviewable.io

KiChjang · 2016-02-21T20:20:25Z

components/script/webdriver_handlers.rs

+                            x += elem.OffsetLeft();
+                            y += elem.OffsetTop();
+                            elem.GetOffsetParent()
+                        }).unwrap_or(None);


Why is an unwrap_or still necessary?

dlrobertson · 2016-02-21T20:38:10Z

Review status: 0 of 4 files reviewed at latest revision, 6 unresolved discussions.

components/script/webdriver_handlers.rs, line 209 [r4] (raw file):
Great question! Map will return an Option<T> and GetOffsetParent will return a Option<Root<Element>>, so unless I'm mistaken object.downcast<T>().map(...) should return an Option<Option<HTMLElement>> while offset_parent is an Option<HTMLElement>, so I used unwrap_or to unwrap the inner Option. I'll double check to see if it compiles without it, but I don't think it will.

Also if you know a better way around this let me know!

Comments from the review on Reviewable.io

Ms2ger · 2016-02-21T21:43:47Z

components/msg/webdriver_msg.rs

@@ -52,6 +53,14 @@ pub enum WebDriverFrameId {
    Parent
 }

+#[derive(Clone, Copy, Deserialize, Serialize)]
+pub struct WebDriverElementRect {


Why not a Rect from Euclid?

Great point, didn't see that it impl'd Serialize. I'll make the change in the update.

Ms2ger · 2016-02-21T21:44:48Z

components/script/webdriver_handlers.rs

+    page.document().upcast::<Node>().traverse_preorder().find(|x| match x.downcast::<Element>() {
+        Some(elem) => elem.Id() == &*element_id,
+        None => false
+    }).map(|x| Root::from_ref(x.downcast::<Element>().unwrap()))


GetElementById should be a lot faster.

Good point, but I think I'm wrong here. See Step 3 of the spec. I think I'm supposed to find by UUID or Node::unique_id.

Ms2ger · 2016-02-21T21:49:31Z

components/script/webdriver_handlers.rs

+                    while let Some(element) = offset_parent {
+                        offset_parent = element.downcast::<HTMLElement>().and_then(|elem| {
+                            x += elem.OffsetLeft();
+                            y += elem.OffsetTop();


Using and_then with a callback that mutates closed-over state us poor practice

Is there documentation for this?

Is this the case for all iterator adapters or just the Option<T> methods that take a FnOnce or just and_then?

Ideally I'd like to create a OffsetParentIterator struct, impl Iterator, and use fold, but this seems to be the only case where this would be used. Without an iterator as far as I know we're pretty much stuck to a while loop and mutating x and y. Let me know if I'm missing anything. Thanks for the comments!

KiChjang · 2016-02-24T13:26:23Z

components/script/webdriver_handlers.rs

+            }
+        },
+        None => Err(())
+    }).unwrap()


This function doesn't look like it has a return value. Perhaps you're missing a semi-colon here?

KiChjang · 2016-02-24T13:27:45Z

components/script/webdriver_handlers.rs

+                Some(html_elem) => {
+                    // Step 1
+                    let mut x: i32 = 0;
+                    let mut y: i32 = 0;


nit: Instead of specifying the type of x and y, specify the type of the 0 constant instead (i.e. use 0i32 instead of 0).

KiChjang · 2016-02-24T13:51:22Z

components/script/webdriver_handlers.rs

@@ -22,6 +22,9 @@ use dom::htmlinputelement::HTMLInputElement;
 use dom::htmloptionelement::HTMLOptionElement;
 use dom::node::Node;
 use dom::window::ScriptHelpers;
+use euclid::point::Point2D;
+use euclid::rect::Rect;
+use euclid::size::Size2D;


Are these imports necessary?

Wait, sorry, nevermind.

KiChjang · 2016-02-24T13:52:14Z

components/webdriver_server/lib.rs

@@ -540,6 +541,24 @@ impl Handler {
        }
    }

+    fn handle_element_rect(&self, element: &WebElement) -> WebDriverResult<WebDriverResponse> {


Is there a spec link for this?

Good point. The update has the spec listed

How about this function?

KiChjang · 2016-02-24T16:37:05Z

components/script/webdriver_handlers.rs

+            // https://w3c.github.io/webdriver/webdriver-spec.html#dfn-calculate-the-absolute-position
+            match elem.downcast::<HTMLElement>() {
+                Some(html_elem) => {
+                    // Step 1


This will definitely confuse people as to which spec this step is following. I think this should only link to the absolute position spec.


        Implement GetElementRect webdriver command

Implement the webdriver Get Element Rect command


        find_node_by_unique_id should use find

find_node_by_unique_id should use find instead of for-loop

KiChjang · 2016-02-24T19:53:16Z

@bors-servo r+

Thanks for working on this!

bors-servo · 2016-02-24T19:53:17Z

📌 Commit cad3115 has been approved by KiChjang

bors-servo · 2016-02-25T00:19:03Z

⌛ Testing commit cad3115 with merge e5f3c5b...


        Auto merge of #9708 - danlrobertson:i8623, r=KiChjang

Implement GetElementRect webdriver command: #8623 Implement the webdriver Get Element Rect command. Originally I wrote out the algorithm for [Step 7](https://w3c.github.io/webdriver/webdriver-spec.html#dfn-calculate-the-absolute-position) and then I found `GetBoundingClientRect`, and i thought it was probably best to use it instead. As always, feedback is very welcomed!  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9708)

bors-servo · 2016-02-25T01:16:58Z

☀️ 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 20, 2016

KiChjang reviewed Feb 20, 2016
View changes

dlrobertson force-pushed the dlrobertson:i8623 branch from 80fb8e0 to 65d7483 Feb 20, 2016

dlrobertson force-pushed the dlrobertson:i8623 branch from 65d7483 to 25967ab Feb 20, 2016

KiChjang reviewed Feb 21, 2016
View changes

dlrobertson force-pushed the dlrobertson:i8623 branch from 25967ab to 80aeb2f Feb 21, 2016

KiChjang reviewed Feb 21, 2016
View changes

components/script/webdriver_handlers.rs Outdated

x += elem.OffsetLeft();

y += elem.OffsetTop();

elem.GetOffsetParent()

}).unwrap_or(None);

This comment has been minimized.

Sign in to view

Copy link

KiChjang Feb 21, 2016
Member

Why is an unwrap_or still necessary?

dlrobertson force-pushed the dlrobertson:i8623 branch from f421d16 to 568c505 Feb 21, 2016

Ms2ger reviewed Feb 21, 2016
View changes

dlrobertson force-pushed the dlrobertson:i8623 branch from 568c505 to 0bb9143 Feb 22, 2016

jdm assigned KiChjang Feb 24, 2016

KiChjang reviewed Feb 24, 2016
View changes

components/script/webdriver_handlers.rs Outdated

}

},

None => Err(())

}).unwrap()

This comment has been minimized.

Sign in to view

Copy link

KiChjang Feb 24, 2016
Member

This function doesn't look like it has a return value. Perhaps you're missing a semi-colon here?

KiChjang reviewed Feb 24, 2016
View changes

dlrobertson force-pushed the dlrobertson:i8623 branch from db9a24b to 99bfd76 Feb 24, 2016

KiChjang reviewed Feb 24, 2016
View changes

dlrobertson force-pushed the dlrobertson:i8623 branch 2 times, most recently from 8d2320a to a640955 Feb 24, 2016

KiChjang added the S-needs-code-changes label Feb 24, 2016

KiChjang removed the S-awaiting-review label Feb 24, 2016

dlrobertson force-pushed the dlrobertson:i8623 branch from a640955 to 826469a Feb 24, 2016

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

KiChjang reviewed Feb 24, 2016
View changes

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

dlrobertson added 2 commits Feb 20, 2016

Implement GetElementRect webdriver command

83b2388

Implement the webdriver Get Element Rect command

find_node_by_unique_id should use find

Loading status checks…

cad3115

find_node_by_unique_id should use find instead of for-loop

dlrobertson force-pushed the dlrobertson:i8623 branch from 826469a to cad3115 Feb 24, 2016

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

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

bors-servo merged commit cad3115 into servo:master Feb 25, 2016
2 checks passed

2 checks passed

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

homu Test successful
Details

dlrobertson deleted the dlrobertson:i8623 branch Feb 25, 2016

servo / servo

Implement GetElementRect webdriver command: #8623 #9708

Implement GetElementRect webdriver command: #8623 #9708

dlrobertson commented Feb 20, 2016

highfive commented Feb 20, 2016

This comment has been minimized.

jdm commented Feb 20, 2016

dlrobertson commented Feb 20, 2016

jdm commented Feb 20, 2016

dlrobertson commented Feb 20, 2016

This comment has been minimized.

This comment has been minimized.

dlrobertson commented Feb 21, 2016

dlrobertson commented Feb 21, 2016

This comment has been minimized.

dlrobertson commented Feb 21, 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.

KiChjang commented Feb 24, 2016

bors-servo commented Feb 24, 2016

bors-servo commented Feb 25, 2016

bors-servo commented Feb 25, 2016

servo / servo

Join GitHub today

Implement GetElementRect webdriver command: #8623 #9708

Implement GetElementRect webdriver command: #8623 #9708

Conversation

dlrobertson commented Feb 20, 2016

highfive commented Feb 20, 2016

This comment has been minimized.

jdm commented Feb 20, 2016

dlrobertson commented Feb 20, 2016

jdm commented Feb 20, 2016

dlrobertson commented Feb 20, 2016

This comment has been minimized.

This comment has been minimized.

dlrobertson commented Feb 21, 2016

dlrobertson commented Feb 21, 2016

This comment has been minimized.

dlrobertson commented Feb 21, 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.

KiChjang commented Feb 24, 2016

bors-servo commented Feb 24, 2016

bors-servo commented Feb 25, 2016

bors-servo commented Feb 25, 2016