Closed Bug 1255955 Opened 9 years ago Closed 8 years ago

Wrong element is clicked when requested element is out of view in <select> element

Categories

(Testing :: General, defect)

Product:
Testing
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox49 fixed, firefox50 fixed, firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: titus.fortner, Assigned: ato)

References

(Blocks 1 open bug)

Details

(Keywords: pi-marionette-server)

Attachments

(15 files, 2 obsolete files)

multiple select list and browse button
86.13 KB, image/png
Details
Bug 1255955 - Rename checkbox test to not conflict with select tests;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1255955 - Scroll element into view when not visible;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1255955 - Recalculate visibility after scrolling;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1255955 - Perform click checks as part of promise;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1255955 - Rename a11y functions check* to assert*;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1255955 - Fix event.parseModifiers_ to not use module argument name;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1255955 - Fix event.sendMouseEvent to work in privileged space;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1255955 - Use correct argument in event.synthesizeMouse;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1255955 - Add function to dispatch events to event library;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1255955 - Add shorthands for generating common DOM events;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1255955 - Add support for interacting with <select> elements;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1255955 - Document clickElement and calculateCentreCoords;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1255955 - Simplify element.clickElement complexity;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1255955 -Run element enabled check before accessibility checks;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.116 Safari/537.36 Steps to reproduce: This test: https://github.com/watir/watirspec/blob/master/select_list_spec.rb#L268 with this html: https://github.com/watir/watirspec/blob/master/html/forms_with_input_elements.html#L48 There are 5 options in the list, but only 4 are displayed. To select 'Sweden' a user needs to scroll to it. Marionette is clicking the element below it. Debug info: https://gist.github.com/titusfortner/701d4e84f4b31c73f171 Actual results: The browse button below the multiple select list was clicked Expected results: Sweden should have been selected from the multiple select list
Blocks: webdriver
Keywords: ateam-marionette-server
Summary: Multiple Select tag → Wrong element is clicked when requested element is out of view in <select> element
OS: Unspecified → All
Hardware: Unspecified → All
Version: 45 Branch → Trunk
Assignee: nobody → ato
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Review commit: https://reviewboard.mozilla.org/r/69666/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69666/
Attachment #8778356 - Flags: review?(dburns)
Attachment #8778357 - Flags: review?(dburns)
Attachment #8778358 - Flags: review?(dburns)
Attachment #8778359 - Flags: review?(dburns)
Attachment #8778360 - Flags: review?(dburns)
Attachment #8778361 - Flags: review?(dburns)
Attachment #8778362 - Flags: review?(dburns)
Attachment #8778363 - Flags: review?(dburns)
Attachment #8778364 - Flags: review?(dburns)
Attachment #8778365 - Flags: review?(dburns)
Attachment #8778366 - Flags: review?(dburns)
The if-condition in the specification compatible interactability check for interaction.clickElement is wrong. It should scroll an element into view when it is _not_ visible. If it is visible it does not matter. Review commit: https://reviewboard.mozilla.org/r/69668/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69668/
We want to redo the element interactability calculation after scrolling. Determining if an element is not visible by the old location would be wrong. Review commit: https://reviewboard.mozilla.org/r/69670/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69670/
Some of the element interactability- and visibility checks were previously done when interaction.clickElement was called, and not as part of the resolution of the returned promise. This could have caused a potential race condition. Review commit: https://reviewboard.mozilla.org/r/69672/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69672/
"Check" is a fine word but with functions which primary purpose is to throw an error internally we should use "assert" to make the reprecussions of using them crystal clear. Review commit: https://reviewboard.mozilla.org/r/69674/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69674/
This patch introduces support for clicking on <select> and <select multiple> elements to Marionette. As <select> elements, especially <select multiple>, are operating system level concepts usually implemented with native widget sets, this patch takes the approach of dispatching generated events. Review commit: https://reviewboard.mozilla.org/r/69686/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69686/
Comment on attachment 8778356 [details] Bug 1255955 - Rename checkbox test to not conflict with select tests; https://reviewboard.mozilla.org/r/69666/#review66866
Attachment #8778356 - Flags: review?(dburns) → review+
https://reviewboard.mozilla.org/r/69668/#review66868 ::: testing/marionette/interaction.js:95 (Diff revision 1) > let win = getWindow(el); > > let visible = false; > if (specCompat) { > visible = element.isInteractable(el); > - if (visible) { > + if (!visible) { Looking at isInteractable, it will always return `true`. On https://dxr.mozilla.org/mozilla-central/source/testing/marionette/element.js#912 `isKeyboardInteractable()` always returning `true`. I dont think we are going to enter this conditional as it is.
Comment on attachment 8778357 [details] Bug 1255955 - Scroll element into view when not visible; https://reviewboard.mozilla.org/r/69668/#review66870
Attachment #8778357 - Flags: review?(dburns) → review-
https://reviewboard.mozilla.org/r/69670/#review66872 ::: testing/marionette/interaction.js:98 (Diff revision 1) > if (specCompat) { > visible = element.isInteractable(el); > if (!visible) { > el.scrollIntoView(false); > } > + visible = element.isInteractable(el); Same issue as described in patch titled `Bug 1255955 - Scroll element into view when not visible; r?automatedtester `
Comment on attachment 8778358 [details] Bug 1255955 - Recalculate visibility after scrolling; https://reviewboard.mozilla.org/r/69670/#review66874
Attachment #8778358 - Flags: review?(dburns) → review-
Comment on attachment 8778359 [details] Bug 1255955 - Perform click checks as part of promise; https://reviewboard.mozilla.org/r/69672/#review66876
Attachment #8778359 - Flags: review?(dburns) → review+
Comment on attachment 8778360 [details] Bug 1255955 - Rename a11y functions check* to assert*; https://reviewboard.mozilla.org/r/69674/#review66878
Attachment #8778360 - Flags: review?(dburns) → review+
Comment on attachment 8778361 [details] Bug 1255955 - Fix event.parseModifiers_ to not use module argument name; https://reviewboard.mozilla.org/r/69676/#review66884
Attachment #8778361 - Flags: review?(dburns) → review+
Comment on attachment 8778362 [details] Bug 1255955 - Fix event.sendMouseEvent to work in privileged space; https://reviewboard.mozilla.org/r/69678/#review66886
Attachment #8778362 - Flags: review?(dburns) → review+
Attachment #8778363 - Flags: review?(dburns) → review+
Comment on attachment 8778364 [details] Bug 1255955 - Add function to dispatch events to event library; https://reviewboard.mozilla.org/r/69682/#review66890
Attachment #8778364 - Flags: review?(dburns) → review+
Comment on attachment 8778365 [details] Bug 1255955 - Add shorthands for generating common DOM events; https://reviewboard.mozilla.org/r/69684/#review66892
Attachment #8778365 - Flags: review?(dburns) → review+
https://reviewboard.mozilla.org/r/69686/#review66902 ::: testing/marionette/harness/marionette/tests/unit/test_select.py:29 (Diff revision 1) > + self.assertFalse(self.marionette.execute_script( > + "return arguments[0].selected", script_args=[option_element], sandbox=None), > + "<option> selected attribute not updated") > + > + > +class TestSelect(SelectTestCase): Can you add a test for `optgroup` as there is one in the selenium test suite for some reason ``` <select> <optgroup label="Group"> <option value="one">one</option> <option id="two-in-group" value="two">two</option> </optgroup> </select> ```
Comment on attachment 8778366 [details] Bug 1255955 - Add support for interacting with <select> elements; https://reviewboard.mozilla.org/r/69686/#review66904
Attachment #8778366 - Flags: review?(dburns) → review+
Comment on attachment 8778357 [details] Bug 1255955 - Scroll element into view when not visible; https://reviewboard.mozilla.org/r/69668/#review66868 > Looking at isInteractable, it will always return `true`. On https://dxr.mozilla.org/mozilla-central/source/testing/marionette/element.js#912 `isKeyboardInteractable()` always returning `true`. I dont think we are going to enter this conditional as it is. You’re right, and according to a spec patch I filed yesterday we should only check for pointer-interactability here: https://github.com/w3c/webdriver/pull/321/files#diff-ce5a67d95770dc70b1b4250c7a9348f2R4138
Comment on attachment 8778361 [details] Bug 1255955 - Fix event.parseModifiers_ to not use module argument name; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69676/diff/1-2/
Comment on attachment 8778366 [details] Bug 1255955 - Add support for interacting with <select> elements; https://reviewboard.mozilla.org/r/69686/#review66902 > Can you add a test for `optgroup` as there is one in the selenium test suite for some reason > > ``` > <select> > <optgroup label="Group"> > <option value="one">one</option> > <option id="two-in-group" value="two">two</option> > </optgroup> > </select> > ``` Good catch. Added a test.
Comment on attachment 8778357 [details] Bug 1255955 - Scroll element into view when not visible; https://reviewboard.mozilla.org/r/69668/#review67626
Attachment #8778357 - Flags: review?(dburns) → review+
Comment on attachment 8778358 [details] Bug 1255955 - Recalculate visibility after scrolling; https://reviewboard.mozilla.org/r/69670/#review67630
Attachment #8778358 - Flags: review?(dburns) → review-
Comment on attachment 8779328 [details] Bug 1255955 - Make interaction.clickElement return a promise; https://reviewboard.mozilla.org/r/70324/#review67636
Attachment #8779328 - Flags: review?(dburns) → review+
Comment on attachment 8778358 [details] Bug 1255955 - Recalculate visibility after scrolling; https://reviewboard.mozilla.org/r/69670/#review67638
Comment on attachment 8778358 [details] Bug 1255955 - Recalculate visibility after scrolling; https://reviewboard.mozilla.org/r/69670/#review67640
Attachment #8778358 - Flags: review- → review+
Comment on attachment 8779329 [details] Bug 1255955 - Check for pointer-interactability on click; https://reviewboard.mozilla.org/r/70326/#review67642
Attachment #8779329 - Flags: review?(dburns) → review+
Attachment #8779328 - Attachment is obsolete: true
Attachment #8779329 - Attachment is obsolete: true
Comment on attachment 8780854 [details] Bug 1255955 - Document clickElement and calculateCentreCoords; https://reviewboard.mozilla.org/r/71436/#review69568
Attachment #8780854 - Flags: review?(dburns) → review+
Comment on attachment 8780855 [details] Bug 1255955 - Simplify element.clickElement complexity; https://reviewboard.mozilla.org/r/71438/#review69572
Attachment #8780855 - Flags: review?(dburns) → review+
Comment on attachment 8780856 [details] Bug 1255955 -Run element enabled check before accessibility checks; https://reviewboard.mozilla.org/r/71440/#review69580
Attachment #8780856 - Flags: review?(dburns) → review+
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/68a158b365c8 Rename checkbox test to not conflict with select tests; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/ccb7b3eed99b Scroll element into view when not visible; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/b73525918482 Recalculate visibility after scrolling; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/04e81c5855ce Perform click checks as part of promise; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/136dce46f6e3 Rename a11y functions check* to assert*; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/a7cfbd664c93 Fix event.parseModifiers_ to not use module argument name; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/1638c7c1614d Fix event.sendMouseEvent to work in privileged space; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/b7eed459e6d7 Use correct argument in event.synthesizeMouse; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/9ed13f873a2a Add function to dispatch events to event library; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/8ef4a877d2be Add shorthands for generating common DOM events; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/8e8aa1f9c27b Add support for interacting with <select> elements; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/8426aadbf3db Document clickElement and calculateCentreCoords; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/2b2e61f6c66b Simplify element.clickElement complexity; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/da03fe82ba9d Run element enabled check before accessibility checks; r=automatedtester
These are test-only changes.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: