Closed Bug 1275273 Opened 8 years ago Closed 7 years ago

Remove atom.isElementSelected

Categories

(Remote Protocol :: Marionette, defect, P2)

Version 3
defect

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.
Blocks: 984682
Assignee: nobody → ato
Status: NEW → ASSIGNED
Going to do this bug once bug 1272653 lands on central.
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 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+
Attachment #8811702 - Attachment is obsolete: true
Attachment #8810870 - Flags: review?(dburns) → review+
Before I'm going to review this I'm waiting for feedback on bug 1277090.
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.
(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 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 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)
Priority: -- → P1
Priority: P1 → P2
What are we waiting on in this bug?
(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.
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).
Setting n-i so we can get this cleared. If you can't do it let me know.
Flags: needinfo?(ato)
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)
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
(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)
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.
(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.
This blocks WebDriver conformance as well, since using Selenium atoms are not conformant.
Blocks: webdriver
Attachment #8811703 - Attachment is obsolete: true
Attachment #8811704 - Attachment is obsolete: true
Attachment #8810870 - Flags: review?(mjzffr) → 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+
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.
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 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.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: