Factorize the checkboxes and the switches into 1 class

RESOLVED FIXED

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jlorenzo, Assigned: jlorenzo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

4 years ago
That was discussed in bug 1182522. It turns out the work has a bigger scope than the bug was implying. Let's put the preliminary work here.
Assignee

Updated

4 years ago
Assignee: nobody → jlorenzo
Assignee

Updated

4 years ago
No longer blocks: 1182522
Assignee

Comment 2

4 years ago
Comment on attachment 8657783 [details] [review]
[gaia] JohanLorenzo:bug-1202388 > mozilla-b2g:master

There are 2 commits in this PR: the first one is for the Gaia switches/checkboxes. The second one is for their HTML equivalents.

To find every single instance, I did:
1. In PyCharm, "Find usage" of is_custom_element_selected
2. git grep 'wrappedJSObject.checked'
3. git grep 'is_selected()'

As you can see, I changed the remaining toggle_. I'd say, this is the most-error prone part of the PR. I might have misunderstood if we needed to be enabled or not at some point. I started an adhoc job[1], to make sure I didn't break anything

[1] http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/883/
Attachment #8657783 - Flags: review?(npark)
Attachment #8657783 - Flags: review?(martijn.martijn)
Assignee

Comment 3

4 years ago
Comment on attachment 8657783 [details] [review]
[gaia] JohanLorenzo:bug-1202388 > mozilla-b2g:master

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #2)
> To find every single instance, I did:
And it turns out I forgot to search for 'expected.element_enabled', which hides 17 other places where we can factorize. I'll create a 3rd commit.
Attachment #8657783 - Flags: review?(npark)
Attachment #8657783 - Flags: review?(martijn.martijn)
Assignee

Comment 4

4 years ago
It turns out to be longer than I expected. In the Settings app we use HTML elements but in a different way: the checkbox is in the DOM but is not visible to the user, you have to tap the label. I introduced this type of Binary component in a 3rd commit.

I cleaned out the 3 commits. You should be able to review 1 commit without taking a look at the others.

Before asking you guys on review, let's check I didn't break anything: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/884/console
Assignee

Comment 5

4 years ago
Comment on attachment 8657783 [details] [review]
[gaia] JohanLorenzo:bug-1202388 > mozilla-b2g:master

The adhoc job showed some errors that are not related to the changes I made:
* The spark tests ran against a non-spark build
* The accessibility tests are failing, but I didn't change the a11y methods
* some punctual error related to other open bugs

One failure is related to the changes, though: test_clock_create_new_alarm.py. I tried to repro locally and I couldn't. I think I'm facing an intermittent failure. I started [1], to make sure of that. Until I find the cause of the failure, I think we're good for a first round of review.

[1] http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/885/console
Attachment #8657783 - Flags: review?(npark)
Attachment #8657783 - Flags: review?(martijn.martijn)
Assignee

Comment 6

4 years ago
That was passing locally because I was not on the right branch. The test is permafailing on Jenkins. I'll look into it.
Assignee

Comment 7

4 years ago
I pushed a fix. Let's see how intermittent this test is: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/887/console
Comment on attachment 8657783 [details] [review]
[gaia] JohanLorenzo:bug-1202388 > mozilla-b2g:master

I really like this patch!
I won't be complaining about any naming any more.

I'm just wondering, I see lots of stuff like this:
+    def enable_data_alert(self):
-        self.marionette.find_element(*self._data_alert_switch_locator).tap()
+        self._data_alert_switch.enable()

And then in the test you use:
ftu_step3.enable_data_alert()

Wouldn't it be better to just use self.data_alert_switch.enable() in the test?
Comment on attachment 8657783 [details] [review]
[gaia] JohanLorenzo:bug-1202388 > mozilla-b2g:master

It looks great!
There is one issue with InvisibleHtmlBinaryControl you need to fix, it doesn't have an is_checked property.
For the rest only a few nits.

I wonder if GaiaBinaryControl/HtmlBinaryControl/InvisibleHtmlBinaryControl
could just be combined into a BinaryControl.
Then we could just use that all over the place and when something would then be switched over from input type=checkbox to gaia-switch, no tests of ours would get broken.
But I guess that's impossible because of InvisibleHtmlBinaryControl which needs an extra locator to tap on the label.
Attachment #8657783 - Flags: review?(martijn.martijn) → review+
Comment on attachment 8657783 [details] [review]
[gaia] JohanLorenzo:bug-1202388 > mozilla-b2g:master

I like this change too, it might break some of the methods in my current PR, but it should be easy to remedy.  Let's put it in and see what happens.
Attachment #8657783 - Flags: review?(npark) → review+
Summary: Factorize the checkboxes and the swiches into 1 class → Factorize the checkboxes and the switches into 1 class
Johan, I noticed that immediately after the switch is toggled, the switch is temporarily greyed out, and its attribute named 'disabled' is set to true.  When in this state, it's visible, but tapping it won't do anything.  

So I do a check like the following:
Wait(self.marionette).until(lambda m: self.marionette.find_element(
*self._cell_data_enabled_input_locator).get_attribute('disabled') == 'false')

Perhaps if this is universal, we could incorporate this into your PR?
Assignee

Updated

4 years ago
Blocks: 1203456
Assignee

Comment 12

4 years ago
Before the merge, I renamed the 'FormControl' class to 'Widget'

(In reply to Martijn Wargers [:mwargers] (QA) from comment #8) 
> Wouldn't it be better to just use self.data_alert_switch.enable() in the
> test?
A more detailed comment in Github[1], in order to not break the law of Demeter and for consistency, it's better to no use self._a_switch.enable directly in a test.


(In reply to Martijn Wargers [:mwargers] (QA) from comment #9)
> There is one issue with InvisibleHtmlBinaryControl you need to fix, it
> doesn't have an is_checked property.
The is_checked property is inherited from HtmlBinaryControl. No changes to do.

> For the rest only a few nits.
Fixed. Details in the PR.

> I wonder if GaiaBinaryControl/HtmlBinaryControl/InvisibleHtmlBinaryControl
> could just be combined into a BinaryControl.
I'm not sure how we could effectively know if we have to tap it "the gaia way" or the other. Worst case scenario, if one panel changes its switches, we have 1 line to change for each switch.


(In reply to No-Jun Park [:njpark] from comment #11)
> So I do a check like the following:
> Wait(self.marionette).until(lambda m: self.marionette.find_element(
> *self._cell_data_enabled_input_locator).get_attribute('disabled') == 'false')
Like discussed on IRC, I'm not 100% sure that el.get_attribute('disabled') and el.is_enabled are exactly the same behavior. They don't call the same marionette method, but if so, we do the wait at [2].


[1] https://github.com/mozilla-b2g/gaia/pull/31718#discussion_r39138742
[2] https://github.com/mozilla-b2g/gaia/pull/31718/files#diff-39a884f07bb9900df9e98c34ae425e20R30
(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #12)
> Like discussed on IRC, I'm not 100% sure that el.get_attribute('disabled')
> and el.is_enabled are exactly the same behavior. They don't call the same
> marionette method, but if so, we do the wait at [2].

I think you're right. I guess this doesn't work for custom elements.
If Marionette does something like this (and there is indication it does):
https://code.google.com/p/selenium/source/browse/javascript/atoms/dom.js?r=d5fbbd43adecb829134167017c66722dcc06b491#318
Then is_enabled only works for certain form elements.
Blocks: 1202831
Assignee

Comment 14

4 years ago
Landed in master at: https://github.com/mozilla-b2g/gaia/commit/e1fc5d6a83925ed4fac547a7fe4bacd3dd44ede2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.