Correct ElementResponseRect and WindowResponseRect types

RESOLVED FIXED in Firefox 57

Status

Testing
geckodriver
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: ato, Assigned: ato)

Tracking

Version 3
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

6 months ago
The field types for ElementResponseRect and WindowResponseRect needs
to be adjusted so that we use the correct data types according to
CSS.

A DOMRect (https://drafts.fxtf.org/geometry-1/#domrect) uses
‘unrestricted doubles’ for its x/y/width/height fields, which is
equivalent to an f64 with a finite check.  Fortunately the ‘Get
Element Rect’ command only returns data and does not have a
corresponding setter.

For WindowProxy
(https://drafts.csswg.org/cssom-view/#extensions-to-the-window-inter
face) the screenX/screenY/outerWidth/outerHeight fields are
platform-independent longs (meaning the bitness of the system
is not taken into account).  This is defined in WebIDL as “a
signed integer type that has values in the range [−2147483648,
2147483647]”.  This can be represented with an i32 internally.

Its corresponding setter, ‘Set Window Rect’ accepts all the same
fields but the WebDriver specification says that the input type
should be a JSON Number with the following restrictions:

> If width or height is neither null nor a Number from 0 to 264 − 1, return error with error code invalid argument.
> 
> If x or y is neither null nor a Number from −(263) to 263 − 1, return error with error code invalid argument.

(The specification uses 64-bit floats, but I’ve submitted
https://github.com/w3c/webdriver/pull/1084 to fix it to use 32-bit.)

This means that WebDriver imposes a further restriction on the
input of width/height, where we should use i32 as the internal data
type but also perform a non-negativity check on input because a
window’s size presumably cannot be negative.

Finally, all values need to be coerced from integer to float
(for ElementResponseRect) and from floats to integers (for
WindowResponseRect) because we want to be generous in what we
accept, but conservative in what we return.
(Assignee)

Updated

6 months ago
Blocks: 1391605
(Assignee)

Updated

6 months ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Blocks: 1397780

Comment 4

6 months ago
mozreview-review
Comment on attachment 8906260 [details]
Bug 1396866 - Relax WindowRectResponse typing.

https://reviewboard.mozilla.org/r/178012/#review183196

::: testing/geckodriver/src/marionette.rs:807
(Diff revision 1)
>                               ErrorStatus::UnknownError,
> -                             "Failed to find y field").as_f64(),
> +                             "Failed to find y field").as_i64(),
>                      ErrorStatus::UnknownError,
> -                    "Failed to interpret y as float");
> +                    "Failed to interpret y as integer");
>  
> -                let rect = WindowRectResponse { x, y, width, height };
> +                let rect = WindowRectResponse {

Are we happy to let this overflow on an invalid response from marionette?
Attachment #8906260 - Flags: review?(james) → review+

Comment 5

6 months ago
mozreview-review
Comment on attachment 8906259 [details]
Bug 1396866 - Restore window state on maximizing, minimizing, and fullscreening.

https://reviewboard.mozilla.org/r/178010/#review183782

::: testing/web-platform/tests/webdriver/tests/contexts/maximize_window.py:44
(Diff revision 1)
> +    """TODO"""
> +
> +
> +def test_handle_prompt_accept(new_session):
> +    """
> +    3. Handle any user prompts and return its value if it is an error.

I am not a big fan of copy pasting the spec here as it means any changes will require us to have to update all this text where ideally we can just look at the test.
Attachment #8906259 - Flags: review?(dburns) → review+

Comment 6

6 months ago
mozreview-review
Comment on attachment 8906261 [details]
Bug 1396866 - Disable test_maximize_when_resized_to_max_size.

https://reviewboard.mozilla.org/r/178014/#review183784

::: testing/web-platform/tests/webdriver/tests/contexts/maximize_window.py:253
(Diff revision 1)
>      assert_success(second_response)
>      assert session.window.size == max_size
>  
>  
> +"""
> +TODO(ato): Implicit session start does not use configuration passed on

Can't we have this as a expected failure?
Attachment #8906261 - Flags: review?(dburns) → review+
(Assignee)

Comment 7

6 months ago
mozreview-review-reply
Comment on attachment 8906259 [details]
Bug 1396866 - Restore window state on maximizing, minimizing, and fullscreening.

https://reviewboard.mozilla.org/r/178010/#review183782

> I am not a big fan of copy pasting the spec here as it means any changes will require us to have to update all this text where ideally we can just look at the test.

I thought jugglinmike had discussed this in some discussion, somewhere, that this was the preferred style.  If you don’t like it you should raise a discussion on the WG mailing list as it is now the predominant style in the wdspec tests.

If we decide not to do this, we should also do away with “step N” comments.
(Assignee)

Comment 8

6 months ago
mozreview-review-reply
Comment on attachment 8906260 [details]
Bug 1396866 - Relax WindowRectResponse typing.

https://reviewboard.mozilla.org/r/178012/#review183196

> Are we happy to let this overflow on an invalid response from marionette?

Yes, these come directly from DOM so I’m happy to trust what that gives us.  If it turns out to be a problem we can revisit this.
(Assignee)

Comment 9

6 months ago
mozreview-review-reply
Comment on attachment 8906261 [details]
Bug 1396866 - Disable test_maximize_when_resized_to_max_size.

https://reviewboard.mozilla.org/r/178014/#review183784

> Can't we have this as a expected failure?

No because it causes a failure if marked as unexpected-failure, and an error otherwise because of https://bugzil.la/1398459.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

6 months ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dcadd5c09851
Restore window state on maximizing, minimizing, and fullscreening. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/793de7b5f147
Relax WindowRectResponse typing. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/1463cc055cf9
Disable test_maximize_when_resized_to_max_size. r=automatedtester
(Assignee)

Updated

6 months ago
No longer blocks: 1397780
https://hg.mozilla.org/mozilla-central/rev/dcadd5c09851
https://hg.mozilla.org/mozilla-central/rev/793de7b5f147
https://hg.mozilla.org/mozilla-central/rev/1463cc055cf9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.