Click's scroll into view is too aggressive on the Select wrapper

RESOLVED WONTFIX

Status

Testing
Marionette
--
major
RESOLVED WONTFIX
5 years ago
3 years ago

People

(Reporter: zac, Assigned: automatedtester)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Created attachment 734547 [details]
screenshot

The 'scroll into view' in the marionette click method (which I believe was changed last week) is operating too aggressively on the System app select wrapper html.

When the select box item (actually a <li> tag) is off screen and clicked, Marionette pushes the whole System app up off screen. The device needs a reboot after this.

If the select/<li> element is within the viewport at the time then the code works fine.

Gaia-ui-tests alarm clock test to run to replicate this:
TestClockSetAlarmRepeat.test_clock_set_alarm_repeat


(NB the FTU test does not replicate the bug without modification, I've provided the screenshot to show it's not specific to the Clock app).
(Reporter)

Comment 1

5 years ago
Pardon,
BuildID: 20130407230204 / v1train
Marionette: 0.5.23

Sorry I don't have the FTU screenshot anymore but it depicts the exact same symptoms.
(Assignee)

Updated

5 years ago
Assignee: nobody → dburns
(Assignee)

Comment 2

5 years ago
I broke it, I will fix it
Thanks for filing; just noting the current behaviour can also be seen when running test_keyboard.py (https://github.com/mozilla/gaia-ui-tests/blob/master/gaiatest/tests/test_keyboard.py)
Blocks: 801898
It seems like we should only attempt to scroll elements which are inside a user-scrollable container (i.e., one with scrollbars).  Otherwise, we can end up scrolling things that were never intended to be scrolled, and end up with Gaia in a bad state.

I'm not sure how easy it would be to detect this, though.
(Assignee)

Comment 5

5 years ago
I have started looking to see if I can port the Selenium scrolling code, its extremely unweildy but it would appear that it is really needed...
(Assignee)

Comment 6

5 years ago
right, I think the crux of the issue is more than checking for scroll bars. there are some CSS elements that prevent users from scrolling yet scrollIntoView overrides that behaviour. I have a plan
(Reporter)

Comment 7

5 years ago
Hey David, we had some work start on this but it seems to have stalled.

We're still suffering from it and I believe it affects desktop a bit worse too. Can this be re-prioritised?
(Assignee)

Comment 8

5 years ago
back on this now... I seem to have deleted my previous patch... sad times
(In reply to David Burns :automatedtester from comment #8)
> back on this now... I seem to have deleted my previous patch... sad times

Is this separate from the work needed for bug 865232 and its dependencies, and, if so, which do you anticipate we'll see first?  Thanks!
(Assignee)

Comment 10

4 years ago
(In reply to Stephen Donner [:stephend] from comment #9)
> Is this separate from the work needed for bug 865232 and its dependencies,
> and, if so, which do you anticipate we'll see first?  Thanks!

Separate and bug 865232 will be done first. I have my Proof of concept working I just need to try make test cases for them, which is a lot, to make sure that we aren't too aggressive or too lenient
Duplicate of this bug: 914982
(Reporter)

Comment 12

3 years ago
This problem just disappeared at some point.

It could still exist with the right combination of elements but we'll file a fresh bug if we see it again.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.