Closed Bug 1255955 Opened 8 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: