Skip to content

ci: update remote browsers in saucelabs and browserstack #24468

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

devversion
Copy link
Member

@devversion devversion commented Feb 23, 2022

See individual commits.

TL;DR: Updates us to the latest available Safari for iOS and macOS. Adjusts tests as needed, and removed some test filter that disabled certain tests in Firefox that actually were hiding an underlying issue in cdk/testing.

Updates the remote browsers in SL and BS. We also updated
Chromium and Firefox recently through `dev-infra`.
Updates the CDK table flex-layout sticky to work with Safari v15. The
test starts failing because the `position: sticky` does not work as
expected for the custom elements without an explicit `display`. Updating
the fixture to actually align columns in a row horizontally fixes this
and makes sense, given the name of the fixture being about flex-based table.
@devversion devversion force-pushed the ci/update-remote-browser-tests branch from 1caae0b to e3b6fc9 Compare February 23, 2022 18:44
…on disabled elements

Currently the CDK harness implementation for `TestBed` tries to mimic user interactions,
like `click`. These helpers can then be called by harness authors or consumers.

Currently there is some inconistency across harness environments where `testElement.click()`
on disabled buttons, inputs etc. causes `click` events to be still emitted in `TestBed` for
Chromium-based browsers, while this is not the case for `TestBed` in `Firefox` and `Safari`,
neither in e2e-based clicks (like protractor or webdriver).

This seems to be inconsistency caused by browsers in the ecosystem. There are bug reports
in Firefox where they explicitly kept emitting `click` when explicitly dispatched programmatically
(seems reasonable to me). Chromium has a bug report where users complain about events being
prevented, even though dispatched programmatically.

https://bugzilla.mozilla.org/show_bug.cgi?id=329509.
https://bugs.chromium.org/p/chromium/issues/detail?id=1115661

In either way though, we should make this consistent with an actual user interaction (like with e2e environments)
and just skip the click event when the underlying element is disabled (we assume the `disabled` property is a sufficient
indicator for this). Note that the mousedown, mouseup event sequence continues to be emulated/synthesized.
…earch

Some tests seem to use `:focus` instead of `document.activeElement` for test
assertions. This seems to work unreliably in Safari as sometimes the `:focus`
query returns `null`, unless `document.activeElement` is requested before (which
seems to indicate that `document.activeElement` does something under-the-hood
to determine the active element, while `querySelector(':focus')` does not have this.

The details on why `activeElement` is better in our case are a little hypothetical
but doesn't seem worth investigating a lot since in all other places in the repo
we haven't used this pattern for checking the active document element.
@devversion devversion force-pushed the ci/update-remote-browser-tests branch from e1d09b3 to 765b407 Compare February 23, 2022 19:23
@devversion devversion added the target: patch This PR is targeted for the next patch release label Feb 23, 2022
@devversion devversion marked this pull request as ready for review February 23, 2022 19:46
@devversion devversion requested review from a team and jelbourn as code owners February 23, 2022 19:46
@devversion
Copy link
Member Author

It would be interesting seeing how many tests this breaks inside g3 (looking at the cdk/testing fix)

@andrewseguin andrewseguin added the action: merge The PR is ready for merge by the caretaker label Feb 23, 2022
@andrewseguin andrewseguin merged commit cf79308 into angular:master Feb 24, 2022
andrewseguin pushed a commit that referenced this pull request Feb 24, 2022
* ci: update remote browsers in saucelabs and browserstack

Updates the remote browsers in SL and BS. We also updated
Chromium and Firefox recently through `dev-infra`.

* test: update cdk table sticky test to work with Safari v15

Updates the CDK table flex-layout sticky to work with Safari v15. The
test starts failing because the `position: sticky` does not work as
expected for the custom elements without an explicit `display`. Updating
the fixture to actually align columns in a row horizontally fixes this
and makes sense, given the name of the fixture being about flex-based table.

* fix(cdk/testing): harness click events inconsistently causing clicks on disabled elements

Currently the CDK harness implementation for `TestBed` tries to mimic user interactions,
like `click`. These helpers can then be called by harness authors or consumers.

Currently there is some inconistency across harness environments where `testElement.click()`
on disabled buttons, inputs etc. causes `click` events to be still emitted in `TestBed` for
Chromium-based browsers, while this is not the case for `TestBed` in `Firefox` and `Safari`,
neither in e2e-based clicks (like protractor or webdriver).

This seems to be inconsistency caused by browsers in the ecosystem. There are bug reports
in Firefox where they explicitly kept emitting `click` when explicitly dispatched programmatically
(seems reasonable to me). Chromium has a bug report where users complain about events being
prevented, even though dispatched programmatically.

https://bugzilla.mozilla.org/show_bug.cgi?id=329509.
https://bugs.chromium.org/p/chromium/issues/detail?id=1115661

In either way though, we should make this consistent with an actual user interaction (like with e2e environments)
and just skip the click event when the underlying element is disabled (we assume the `disabled` property is a sufficient
indicator for this). Note that the mousedown, mouseup event sequence continues to be emulated/synthesized.

* test: use `document.activeElement` instead of pseudo `:focus` query search

Some tests seem to use `:focus` instead of `document.activeElement` for test
assertions. This seems to work unreliably in Safari as sometimes the `:focus`
query returns `null`, unless `document.activeElement` is requested before (which
seems to indicate that `document.activeElement` does something under-the-hood
to determine the active element, while `querySelector(':focus')` does not have this.

The details on why `activeElement` is better in our case are a little hypothetical
but doesn't seem worth investigating a lot since in all other places in the repo
we haven't used this pattern for checking the active document element.

(cherry picked from commit cf79308)
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants