Closed
Bug 1275273
Opened 8 years ago
Closed 7 years ago
Remove atom.isElementSelected
Categories
(Remote Protocol :: Marionette, defect, P2)
Tracking
(firefox58 fixed)
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: ato, Assigned: ato)
References
(Blocks 2 open bugs)
Details
(Keywords: pi-marionette-server)
Attachments
(2 files, 3 obsolete files)
Bug 1272653 made us non-dependant on atom.getElementAttribute from testing/marionette/atom.js but forgot to remove it. This is a follow-up bug to do so.
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 1•8 years ago
|
||
Going to do this bug once bug 1272653 lands on central.
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 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8811702 [details]
fixup! Bug 1275273 - Remove getElementAttribute atom;
https://reviewboard.mozilla.org/r/93722/#review93798
Can you merge this with the commit that it fixes up
Attachment #8811702 -
Flags: review?(dburns) → review-
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8810871 [details]
Bug 1275273 - Make WebDriver:IsElementSelected conform to spec.
https://reviewboard.mozilla.org/r/93168/#review93802
The code is correct but the commit message is wrong. It's not `is Element Displayed` it's `is Element Selected`.
Attachment #8810871 -
Flags: review?(dburns) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8811702 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 24•8 years ago
|
||
mozreview-review |
Comment on attachment 8810870 [details]
Bug 1275273 - Remove isElementSelected atom.
https://reviewboard.mozilla.org/r/93166/#review93850
Attachment #8810870 -
Flags: review?(dburns) → review+
Comment 25•8 years ago
|
||
Before I'm going to review this I'm waiting for feedback on bug 1277090.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•8 years ago
|
||
I suspect I’m going to have to rework the changes to the Firefox UI tests here so that they are backwards compatible. Perhaps we could introduce a temporary (deprecated) Marionette.get_conflated_attribute function or something similar in the Firefox UI harness.
Comment 31•8 years ago
|
||
(In reply to Andreas Tolfsen ‹:ato› from comment #30)
> I suspect I’m going to have to rework the changes to the Firefox UI tests
> here so that they are backwards compatible. Perhaps we could introduce a
> temporary (deprecated) Marionette.get_conflated_attribute function or
> something similar in the Firefox UI harness.
Is this bug targeted to uplift to Aurora? If not I would propose we do the fix on bug 1277090 and get this uplifted for 52. Then it will be part of the next ESR and we can get rid of the workaround in 55.0. Then we land this patch on central only - in case it is too risky to uplift.
Comment 32•8 years ago
|
||
mozreview-review |
Comment on attachment 8811703 [details]
Bug 1275273 - Make puppeteer framework aware of properties and attributes;
https://reviewboard.mozilla.org/r/93724/#review95316
Attachment #8811703 -
Flags: review?(hskupin)
Comment 33•8 years ago
|
||
mozreview-review |
Comment on attachment 8811704 [details]
Bug 1275273 - Fix attribute vs. property conflation in Firefox UI tests;
https://reviewboard.mozilla.org/r/93726/#review95318
Attachment #8811704 -
Flags: review?(hskupin)
Assignee | ||
Updated•8 years ago
|
Priority: -- → P1
Assignee | ||
Updated•8 years ago
|
Blocks: selenium-atoms
Updated•8 years ago
|
Priority: P1 → P2
Comment 34•8 years ago
|
||
What are we waiting on in this bug?
Assignee | ||
Comment 35•8 years ago
|
||
(In reply to David Burns :automatedtester from comment #34)
> What are we waiting on in this bug?
I can’t remember exactly, but I suspect it had Firefox UI test failures.
I can try to rebase and it and give it another try run.
Comment 36•8 years ago
|
||
Yes, there was the problem with change from getAttribute to getProperty. But that got fixed for the last ESR. So nothing should basically stop us to land this in 55.0 (not in 54.0 please due to update tests from 51.0).
Comment 37•7 years ago
|
||
Setting n-i so we can get this cleared. If you can't do it let me know.
Flags: needinfo?(ato)
Assignee | ||
Comment 38•7 years ago
|
||
This depends on some patches in https://bugzilla.mozilla.org/show_bug.cgi?id=1321516 because it changes code in the same area. I have a couple of higher-priority bugs on my hand right at the moment, but this is certainly on my backlog.
Depends on: 1321516
Flags: needinfo?(ato)
Comment 39•7 years ago
|
||
The bug stalled a bit over the last months. So a couple of changes happened meanwhile.
Also the removal of the atom.getElementAttribute method will be done with my patch on bug 1375660 now. But what I cannot find is a bug about removing atom.isElementSelected. Given that this bug seems to have a sane implementation, lets update its summary, and Andreas might find the time to check how that will work with our current code base.
Andreas, given that you are assigned, will you continue on this bug?
Flags: needinfo?(ato)
Summary: Remove atom.getElementAttribute → Remove atom.isElementSelected
Assignee | ||
Comment 40•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #39)
> Also the removal of the atom.getElementAttribute method will be
> done with my patch on bug 1375660 now.
Getting rid of atom.getElementAttribute helps here, if I read my own
notes above correctly.
> Andreas, given that you are assigned, will you continue on this
> bug?
As I said, I will get back to it.
Flags: needinfo?(ato)
Comment 41•7 years ago
|
||
If you don't have the time to work on this bug yet, you can unassign yourself and we could see if we can open up this bug as mentored bug. We shouldn't hold off bugs from getting fixed by keeping ourselves assigned.
Assignee | ||
Comment 42•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #41)
> If you don't have the time to work on this bug yet, you can
> unassign yourself and we could see if we can open up this bug as
> mentored bug. We shouldn't hold off bugs from getting fixed by
> keeping ourselves assigned.
I have already written the patches, so it should just be a matter
of rebasing them and re-running the tests. If I don’t have the
capacity to do that I doubt doing a mentored bug will help.
Assignee | ||
Comment 43•7 years ago
|
||
This blocks WebDriver conformance as well, since using Selenium
atoms are not conformant.
Blocks: webdriver
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8811703 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8811704 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment 47•7 years ago
|
||
mozreview-review |
Comment on attachment 8810870 [details]
Bug 1275273 - Remove isElementSelected atom.
https://reviewboard.mozilla.org/r/93166/#review193574
Attachment #8810870 -
Flags: review?(mjzffr) → review+
Comment 48•7 years ago
|
||
mozreview-review |
Comment on attachment 8810871 [details]
Bug 1275273 - Make WebDriver:IsElementSelected conform to spec.
https://reviewboard.mozilla.org/r/93168/#review193584
::: testing/marionette/test_element.js:64
(Diff revision 10)
> + option.checked = true;
> + option.selected = false;
> + ok(!element.isSelected(option));
> +
> + // anything else should not be selected
> + for (let typ of [undefined, null, "foo", true, [], {}]) {
Could add `domEl` to this list.
Attachment #8810871 -
Flags: review?(mjzffr) → review+
Assignee | ||
Comment 49•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8810871 [details]
Bug 1275273 - Make WebDriver:IsElementSelected conform to spec.
https://reviewboard.mozilla.org/r/93168/#review193584
> Could add `domEl` to this list.
Good idea! Fixed.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 52•7 years ago
|
||
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70b93f9bfa0c
Remove isElementSelected atom. r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a914f741768
Make WebDriver:IsElementSelected conform to spec. r=maja_zf
Comment 53•7 years ago
|
||
mozreview-review |
Comment on attachment 8810870 [details]
Bug 1275273 - Remove isElementSelected atom.
https://reviewboard.mozilla.org/r/93166/#review193654
Just a sidenote... this commit should have been the second one in the list, given that we now have busted tests.
Comment 54•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/70b93f9bfa0c
https://hg.mozilla.org/mozilla-central/rev/5a914f741768
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•