David Burns :automatedtester Reporter
	Description • 9 years ago

size and location have been deprecated and are due for removal. We need to update the python tests to use HTMLElement.rect instead

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 1 • 9 years ago

/r/7207 - Bug 1154525: Make HTMLElement#location and #size use #rect internally

Pull down this commit:

hg pull -r d0f18163b3bcf7e3a9d5698953e244b718013258 https://reviewboard-hg.mozilla.org/gecko/

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 2 • 9 years ago

dburns: I'm sorry I misread this bug as making them use rect internally, but I only realised after I'd pushed my patch.  Refiled your original bug as bug 1155746.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 3 • 9 years ago

Comment on attachment 8594010 [details]
MozReview Request: bz://1154525/ato

/r/7207 - Bug 1154525: Make HTMLElement#location and #size use #rect internally

Pull down this commit:

hg pull -r d0f18163b3bcf7e3a9d5698953e244b718013258 https://reviewboard-hg.mozilla.org/gecko/

	Chris Manchester (limited bugmail, email directly)
	Comment 4 • 9 years ago

Comment on attachment 8594010 [details]
MozReview Request: bz://1154525/ato

https://reviewboard.mozilla.org/r/7205/#review6145

::: testing/marionette/driver/marionette_driver/marionette.py:153
(Diff revision 1)
> +        return {"x": rect.x, "y": rect.y}

In the listener's getElementLocation I see:

    let el = elementManager.getKnownElement(msg.json.id, curFrame);
    let rect = el.getBoundingClientRect();

    let location = {};
    location.x = rect.left;
    location.y = rect.top;
    
but getElementRect has:

    let el = elementManager.getKnownElement(id, curFrame);
    let clientRect = el.getBoundingClientRect();
    return {
        x: clientRect.x + curFrame.pageXOffset,
        y: clientRect.y  + curFrame.pageYOffset,
        width: clientRect.width,
        height: clientRect.height
    };

from documentation of getBoundingClientRect, this means location used to be relative to the viewport but is now relative to the document. Is that intended?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 5 • 9 years ago

https://reviewboard.mozilla.org/r/7205/#review6195

> In the listener's getElementLocation I see:
> 
>     let el = elementManager.getKnownElement(msg.json.id, curFrame);
>     let rect = el.getBoundingClientRect();
> 
>     let location = {};
>     location.x = rect.left;
>     location.y = rect.top;
>     
> but getElementRect has:
> 
>     let el = elementManager.getKnownElement(id, curFrame);
>     let clientRect = el.getBoundingClientRect();
>     return {
>         x: clientRect.x + curFrame.pageXOffset,
>         y: clientRect.y  + curFrame.pageYOffset,
>         width: clientRect.width,
>         height: clientRect.height
>     };
> 
> from documentation of getBoundingClientRect, this means location used to be relative to the viewport but is now relative to the document. Is that intended?

Well spotted, sir!  I didn't catch this.

Although the [specification isn't entirely clear on this](https://w3c.github.io/webdriver/webdriver-spec.html#elementrect-getelementrect) I think `getElementLocation` has been wrong all along.  An element's position should be relative to the document, and not the viewport.

I also think this is what Selenium does, but that's also not quite clear from the API documentation.

The question then is if we have test code which relies on this for calculations.  I'll do a full try run to verify.

Thanks again for the thorough review!

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 6 • 9 years ago

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7875e6a70828

	Chris Manchester (limited bugmail, email directly)
	Comment 7 • 9 years ago

Comment on attachment 8594010 [details]
MozReview Request: bz://1154525/ato

https://reviewboard.mozilla.org/r/7205/#review6217

::: testing/marionette/driver/marionette_driver/marionette.py:158
(Diff revision 1)
> +        

Whitespace added.

	Chris Manchester (limited bugmail, email directly)
	Comment 8 • 9 years ago

Comment on attachment 8594010 [details]
MozReview Request: bz://1154525/ato

https://reviewboard.mozilla.org/r/7205/#review6219

Ship It!

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 9 • 9 years ago

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f1ca5412ad1

	Pulsebot
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/768d5fb8b227

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/768d5fb8b227