Remove atom.isElementSelected

RESOLVED FIXED in Firefox 58

Status

defect
P2
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: ato, Assigned: ato)

Tracking

(Blocks 2 bugs, {pi-marionette-server})

Version 3
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

3 years ago
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

3 years ago
Blocks: 984682
Assignee

Updated

3 years ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
Assignee

Comment 1

3 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 14

3 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

3 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

3 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

3 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+
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

3 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.
(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

3 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

3 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

2 years ago
Priority: -- → P1
Assignee

Updated

2 years ago
Priority: P1 → P2
What are we waiting on in this bug?
Assignee

Comment 35

2 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.
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)
Assignee

Comment 38

2 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)
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

2 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)
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

2 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

2 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

2 years ago
Attachment #8811703 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8811704 - Attachment is obsolete: true
Comment hidden (mozreview-request)
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

2 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

2 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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/70b93f9bfa0c
https://hg.mozilla.org/mozilla-central/rev/5a914f741768
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee

Updated

Last year
Duplicate of this bug: 1279205
You need to log in before you can comment on or make changes to this bug.