Closed
Bug 1396866
Opened 7 years ago
Closed 7 years ago
Correct ElementResponseRect and WindowResponseRect types
Categories
(Testing :: geckodriver, enhancement)
Tracking
(firefox57 fixed)
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: ato, Assigned: ato)
References
Details
Attachments
(3 files)
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•7 years 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) |
Comment 4•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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
Comment 14•7 years ago
|
||
bugherder |
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
Closed: 7 years 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.
Description
•