Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[web] Add styling in CSS reset to remove Edge password reveal icon #38011

Merged
merged 10 commits into from
Dec 5, 2022
Merged

[web] Add styling in CSS reset to remove Edge password reveal icon #38011

merged 10 commits into from
Dec 5, 2022

Conversation

htoor3
Copy link
Contributor

@htoor3 htoor3 commented Dec 1, 2022 •
edited
Loading

Microsoft Edge includes a password reveal icon pseudoelement inside text inputs by default. To allow users to add custom icons to their password inputs and to keep behavior consistent with Chrome and Firefox, we should remove the pseudoelement in password inputs for Edge browsers.

Fixes flutter/flutter#83695

Note for tests: getComputedStyle() doesn't seem to work with this pseudoelement ::-ms-reveal in newer versions of Edge, so there doesn't appear to be a way to add a unit test to determine that this style was applied.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Dec 1, 2022
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

htoor3
htoor3 commented Dec 1, 2022
View reviewed changes
yjbanov
yjbanov reviewed Dec 1, 2022
View reviewed changes
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if LGT @nbayati

@htoor3 htoor3 requested a review from nbayati December 1, 2022 23:01
@skia-gold
Copy link

Gold has detected about 72 new digest(s) on patchset 4.
View them at https://flutter-engine-gold.skia.org/cl/github/38011

nbayati
nbayati approved these changes Dec 3, 2022
View reviewed changes
Copy link
Contributor

@nbayati nbayati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit. Otherwise, LGTM.

@skia-gold
Copy link

Gold has detected about 38 new digest(s) on patchset 10.
View them at https://flutter-engine-gold.skia.org/cl/github/38011

@htoor3 htoor3 added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 5, 2022
@auto-submit auto-submit bot merged commit 53d66aa into flutter:main Dec 5, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 5, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 5, 2022
@engine-flutter-autoroll
53d66aa55 [web] Add styling in CSS reset to remove Edge password reve…
e16ebf6 
…al icon (flutter/engine#38011)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 5, 2022
@engine-flutter-autoroll
Roll Flutter Engine from d82c8ad7ac7f to 0efb0ef40513 (2 revisions) (#…
e0a0190 
…116552)

* 53d66aa55 [web] Add styling in CSS reset to remove Edge password reveal icon (flutter/engine#38011)

* 0efb0ef40 [ImpellerC] Print the working directory that was tried when failing to open (flutter/engine#38079)
mit-mit pushed a commit to mit-mit/flutter that referenced this pull request Dec 6, 2022
@engine-flutter-autoroll @mit-mit
Roll Flutter Engine from d82c8ad7ac7f to 0efb0ef40513 (2 revisions) (f…
2eeb9ab 
…lutter#116552)

* 53d66aa55 [web] Add styling in CSS reset to remove Edge password reveal icon (flutter/engine#38011)

* 0efb0ef40 [ImpellerC] Print the working directory that was tried when failing to open (flutter/engine#38079)
shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
@engine-flutter-autoroll @shogohida
Roll Flutter Engine from d82c8ad7ac7f to 0efb0ef40513 (2 revisions) (f…
5a7dd36 
…lutter#116552)

* 53d66aa55 [web] Add styling in CSS reset to remove Edge password reveal icon (flutter/engine#38011)

* 0efb0ef40 [ImpellerC] Print the working directory that was tried when failing to open (flutter/engine#38079)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
@engine-flutter-autoroll @gspencergoog
Roll Flutter Engine from d82c8ad7ac7f to 0efb0ef40513 (2 revisions) (f…
f7a64fa 
…lutter#116552)

* 53d66aa55 [web] Add styling in CSS reset to remove Edge password reveal icon (flutter/engine#38011)

* 0efb0ef40 [ImpellerC] Print the working directory that was tried when failing to open (flutter/engine#38079)
@erojostt
Copy link

Apparently the issue is not resolved with the following versions of Flutter and Microsoft Edge:

  • Flutter version 3.7.11

  • Microsoft Edge Version

image

image

@lclaesen
Copy link

lclaesen commented Jun 9, 2023

Apparently the issue is not resolved with the following versions of Flutter and Microsoft Edge:

  • Flutter version 3.7.11
  • Microsoft Edge Version

image

image

As per the release notes, the fix for this lands in Flutter v3.10.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@yjbanov yjbanov yjbanov left review comments

@nbayati nbayati nbayati approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App needs tests platform-web Code specifically for the web engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[web] Disable password reveal icon if suffixIcon is used on Edge browser
6 participants
@htoor3 @skia-gold @erojostt @lclaesen @yjbanov @nbayati