Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 49

Status

Testing
General
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: Titus Fortner, Assigned: ato)

Tracking

(Blocks: 1 bug, {ateam-marionette-server})

Trunk
mozilla51
ateam-marionette-server
Points:
---

Firefox Tracking Flags

(firefox49 fixed, firefox50 fixed, firefox51 fixed)

Details

MozReview Requests

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

Attachments

(15 attachments, 2 obsolete attachments)

86.13 KB, image/png
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
(Reporter)

Description

a year ago
Created attachment 8729775 [details]
multiple select list and browse button

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
(Assignee)

Updated

a year ago
Blocks: 721859
Keywords: ateam-marionette-server
(Assignee)

Updated

a year ago
Summary: Multiple Select tag → Wrong element is clicked when requested element is out of view in <select> element
(Assignee)

Updated

a year ago
OS: Unspecified → All
Hardware: Unspecified → All
Version: 45 Branch → Trunk
(Assignee)

Updated

a year ago
Assignee: nobody → ato
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 1

a year ago
Created attachment 8778356 [details]
Bug 1255955 - Rename checkbox test to not conflict with select tests;

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)
(Assignee)

Comment 2

a year ago
Created attachment 8778357 [details]
Bug 1255955 - Scroll element into view when not visible;

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/
(Assignee)

Comment 3

a year ago
Created attachment 8778358 [details]
Bug 1255955 - Recalculate visibility after scrolling;

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/
(Assignee)

Comment 4

a year ago
Created attachment 8778359 [details]
Bug 1255955 - Perform click checks as part of promise;

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/
(Assignee)

Comment 5

a year ago
Created attachment 8778360 [details]
Bug 1255955 - Rename a11y functions check* to assert*;

"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/
(Assignee)

Comment 6

a year ago
Created attachment 8778361 [details]
Bug 1255955 - Fix event.parseModifiers_ to not use module argument name;

Review commit: https://reviewboard.mozilla.org/r/69676/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69676/
(Assignee)

Comment 7

a year ago
Created attachment 8778362 [details]
Bug 1255955 - Fix event.sendMouseEvent to work in privileged space;

Also fixes assumptions about permitted mouse events.

Review commit: https://reviewboard.mozilla.org/r/69678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69678/
(Assignee)

Comment 8

a year ago
Created attachment 8778363 [details]
Bug 1255955 - Use correct argument in event.synthesizeMouse;

Review commit: https://reviewboard.mozilla.org/r/69680/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69680/
(Assignee)

Comment 9

a year ago
Created attachment 8778364 [details]
Bug 1255955 - Add function to dispatch events to event library;

Review commit: https://reviewboard.mozilla.org/r/69682/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69682/
(Assignee)

Comment 10

a year ago
Created attachment 8778365 [details]
Bug 1255955 - Add shorthands for generating common DOM events;

Review commit: https://reviewboard.mozilla.org/r/69684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69684/
(Assignee)

Comment 11

a year ago
Created attachment 8778366 [details]
Bug 1255955 - Add support for interacting with <select> elements;

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 8778363 [details]
Bug 1255955 - Use correct argument in event.synthesizeMouse;

https://reviewboard.mozilla.org/r/69680/#review66888
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+
(Assignee)

Updated

a year ago
Duplicate of this bug: 1293622
(Assignee)

Comment 27

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 41

a year ago
mozreview-review-reply
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 42

a year ago
mozreview-review
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 43

a year ago
mozreview-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 44

a year ago
mozreview-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 45

a year ago
mozreview-review
Comment on attachment 8778358 [details]
Bug 1255955 - Recalculate visibility after scrolling;

https://reviewboard.mozilla.org/r/69670/#review67638

Comment 46

a year ago
mozreview-review
Comment on attachment 8778358 [details]
Bug 1255955 - Recalculate visibility after scrolling;

https://reviewboard.mozilla.org/r/69670/#review67640
Attachment #8778358 - Flags: review- → review+

Comment 47

a year ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8779328 - Attachment is obsolete: true
(Assignee)

Updated

11 months ago
Attachment #8779329 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 73

11 months ago
mozreview-review
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 74

11 months ago
mozreview-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 75

11 months ago
mozreview-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+

Comment 76

11 months ago
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

Comment 77

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/68a158b365c8
https://hg.mozilla.org/mozilla-central/rev/ccb7b3eed99b
https://hg.mozilla.org/mozilla-central/rev/b73525918482
https://hg.mozilla.org/mozilla-central/rev/04e81c5855ce
https://hg.mozilla.org/mozilla-central/rev/136dce46f6e3
https://hg.mozilla.org/mozilla-central/rev/a7cfbd664c93
https://hg.mozilla.org/mozilla-central/rev/1638c7c1614d
https://hg.mozilla.org/mozilla-central/rev/b7eed459e6d7
https://hg.mozilla.org/mozilla-central/rev/9ed13f873a2a
https://hg.mozilla.org/mozilla-central/rev/8ef4a877d2be
https://hg.mozilla.org/mozilla-central/rev/8e8aa1f9c27b
https://hg.mozilla.org/mozilla-central/rev/8426aadbf3db
https://hg.mozilla.org/mozilla-central/rev/2b2e61f6c66b
https://hg.mozilla.org/mozilla-central/rev/da03fe82ba9d
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 78

11 months ago
These are test-only changes.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]

Comment 79

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/2f7f2aa1c730
https://hg.mozilla.org/releases/mozilla-aurora/rev/decf2949ae6c
https://hg.mozilla.org/releases/mozilla-aurora/rev/f1da984b8731
https://hg.mozilla.org/releases/mozilla-aurora/rev/fc55305fb268
https://hg.mozilla.org/releases/mozilla-aurora/rev/6b1c452bd581
https://hg.mozilla.org/releases/mozilla-aurora/rev/4371df1f298a
https://hg.mozilla.org/releases/mozilla-aurora/rev/b8d43d25038b
https://hg.mozilla.org/releases/mozilla-aurora/rev/5a591a1a98cd
https://hg.mozilla.org/releases/mozilla-aurora/rev/0e8d2fe2a205
https://hg.mozilla.org/releases/mozilla-aurora/rev/c06be43a960f
https://hg.mozilla.org/releases/mozilla-aurora/rev/30edafac6164
https://hg.mozilla.org/releases/mozilla-aurora/rev/72f83f7e308c
https://hg.mozilla.org/releases/mozilla-aurora/rev/393b9382b1fc
https://hg.mozilla.org/releases/mozilla-aurora/rev/f4aa3c3af595
status-firefox50: --- → fixed
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-beta]

Comment 80

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/690edd23c8c6
https://hg.mozilla.org/releases/mozilla-beta/rev/833ee9a9a942
https://hg.mozilla.org/releases/mozilla-beta/rev/556eca85f7d7
https://hg.mozilla.org/releases/mozilla-beta/rev/07a530ae86fd
https://hg.mozilla.org/releases/mozilla-beta/rev/7ae6f4a74bdb
https://hg.mozilla.org/releases/mozilla-beta/rev/004286546df5
https://hg.mozilla.org/releases/mozilla-beta/rev/9180c0406c2c
https://hg.mozilla.org/releases/mozilla-beta/rev/019699b3498f
https://hg.mozilla.org/releases/mozilla-beta/rev/22b6159c39ec
https://hg.mozilla.org/releases/mozilla-beta/rev/dd9969584975
https://hg.mozilla.org/releases/mozilla-beta/rev/a6cb9a93d51e
https://hg.mozilla.org/releases/mozilla-beta/rev/11781f2941a9
https://hg.mozilla.org/releases/mozilla-beta/rev/846649708880
https://hg.mozilla.org/releases/mozilla-beta/rev/1779d707fb17
status-firefox49: --- → fixed
Whiteboard: [checkin-needed-beta]
(Assignee)

Updated

9 months ago
Duplicate of this bug: 1255957
(Assignee)

Updated

9 months ago
Duplicate of this bug: 1314671
You need to log in before you can comment on or make changes to this bug.