Bug 191969

Summary: REGRESSION (r238078): Do not draw caps lock indicator when Strong Password button is shown
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Layout and RenderingAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, dino, ryanhaddad, simon.fraser, tsavell, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar, Regression
Version: WebKit Local Build   
Hardware: iPhone / iPad   
OS: iOS 12   
Bug Depends on: 190565    
Bug Blocks:    
Attachments:
Description Flags
Patch and layout tests none

Description Daniel Bates 2018-11-26 10:05:43 PST
Following r238078 we now support drawing the caps lock indicator in password fields on iOS. However it is not meaningful to show the caps lock indicator when the Strong Password button is visible because the password field is not editable. We should not paint the caps lock indicator when the Strong Password button is visible.
Comment 1 Radar WebKit Bug Importer 2018-11-26 10:06:14 PST
<rdar://problem/46247569>
Comment 2 Daniel Bates 2018-11-26 10:24:02 PST
Created attachment 355650 [details]
Patch and layout tests
Comment 3 Daniel Bates 2018-11-26 12:42:26 PST
Comment on attachment 355650 [details]
Patch and layout tests

Clearing flags on attachment: 355650

Committed r238513: <https://trac.webkit.org/changeset/238513>
Comment 4 Daniel Bates 2018-11-26 12:42:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Truitt Savell 2018-11-26 16:02:35 PST
The new test fast/forms/auto-fill-button/caps-lock-indicator-should-be-visible-when-after-hiding-auto-fill-strong-password-button.html is failing constantly after being added. 

the diff shows the I beam is in the wrong place in the password field. 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fforms%2Fauto-fill-button%2Fcaps-lock-indicator-should-be-visible-when-after-hiding-auto-fill-strong-password-button.html

Diff:
https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r238522%20(1020)/fast/forms/auto-fill-button/caps-lock-indicator-should-be-visible-when-after-hiding-auto-fill-strong-password-button-diffs.html
Comment 6 Daniel Bates 2018-11-26 16:48:16 PST
(In reply to Truitt Savell from comment #5)
> The new test
> fast/forms/auto-fill-button/caps-lock-indicator-should-be-visible-when-after-
> hiding-auto-fill-strong-password-button.html is failing constantly after
> being added. 
> 
> the diff shows the I beam is in the wrong place in the password field. 
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=fast%2Fforms%2Fauto-fill-button%2Fcaps-lock-
> indicator-should-be-visible-when-after-hiding-auto-fill-strong-password-
> button.html
> 
> Diff:
> https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> r238522%20(1020)/fast/forms/auto-fill-button/caps-lock-indicator-should-be-
> visible-when-after-hiding-auto-fill-strong-password-button-diffs.html

Skip the test for now. Will fix the test shortly.
Comment 7 Ryan Haddad 2018-11-26 17:03:17 PST
(In reply to Daniel Bates from comment #6)
> (In reply to Truitt Savell from comment #5)
> > The new test
> > fast/forms/auto-fill-button/caps-lock-indicator-should-be-visible-when-after-
> > hiding-auto-fill-strong-password-button.html is failing constantly after
> > being added. 
> > 
> > the diff shows the I beam is in the wrong place in the password field. 
> > 
> > History:
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=fast%2Fforms%2Fauto-fill-button%2Fcaps-lock-
> > indicator-should-be-visible-when-after-hiding-auto-fill-strong-password-
> > button.html
> > 
> > Diff:
> > https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> > r238522%20(1020)/fast/forms/auto-fill-button/caps-lock-indicator-should-be-
> > visible-when-after-hiding-auto-fill-strong-password-button-diffs.html
> 
> Skip the test for now. Will fix the test shortly.
We shouldn't skip brand new tests. I think we should roll out this change and re-land it with a working test.
Comment 8 Daniel Bates 2018-11-26 17:38:00 PST
(In reply to Ryan Haddad from comment #7)
> (In reply to Daniel Bates from comment #6)
> > (In reply to Truitt Savell from comment #5)
> > > The new test
> > > fast/forms/auto-fill-button/caps-lock-indicator-should-be-visible-when-after-
> > > hiding-auto-fill-strong-password-button.html is failing constantly after
> > > being added. 
> > > 
> > > the diff shows the I beam is in the wrong place in the password field. 
> > > 
> > > History:
> > > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > > html#showAllRuns=true&tests=fast%2Fforms%2Fauto-fill-button%2Fcaps-lock-
> > > indicator-should-be-visible-when-after-hiding-auto-fill-strong-password-
> > > button.html
> > > 
> > > Diff:
> > > https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> > > r238522%20(1020)/fast/forms/auto-fill-button/caps-lock-indicator-should-be-
> > > visible-when-after-hiding-auto-fill-strong-password-button-diffs.html
> > 
> > Skip the test for now. Will fix the test shortly.
> We shouldn't skip brand new tests. I think we should roll out this change
> and re-land it with a working test.

Feel free to roll it out.
Comment 9 Ryan Haddad 2018-11-26 18:00:06 PST
Rolled out in https://trac.webkit.org/changeset/238540/webkit
Comment 10 Daniel Bates 2018-11-26 20:17:19 PST
Committed r238545: <https://trac.webkit.org/changeset/238545>
Comment 11 Truitt Savell 2018-11-27 12:03:55 PST
The new test fast/forms/auto-fill-button/caps-lock-indicator-should-be-visible-after-hiding-auto-fill-strong-password-button.html

added in https://trac.webkit.org/changeset/238545/webkit

is timing out at around 300 seconds on Mojave WK2. 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fforms%2Fauto-fill-button%2Fcaps-lock-indicator-should-be-visible-after-hiding-auto-fill-strong-password-button.html
Comment 12 Daniel Bates 2018-11-27 13:21:10 PST
(In reply to Truitt Savell from comment #11)
> The new test
> fast/forms/auto-fill-button/caps-lock-indicator-should-be-visible-after-
> hiding-auto-fill-strong-password-button.html
> 
> added in https://trac.webkit.org/changeset/238545/webkit
> 
> is timing out at around 300 seconds on Mojave WK2. 
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=fast%2Fforms%2Fauto-fill-button%2Fcaps-lock-
> indicator-should-be-visible-after-hiding-auto-fill-strong-password-button.
> html

Fixing
Comment 13 Daniel Bates 2018-11-27 13:22:59 PST
(In reply to Daniel Bates from comment #12)
> (In reply to Truitt Savell from comment #11)
> > The new test
> > fast/forms/auto-fill-button/caps-lock-indicator-should-be-visible-after-
> > hiding-auto-fill-strong-password-button.html
> > 
> > added in https://trac.webkit.org/changeset/238545/webkit
> > 
> > is timing out at around 300 seconds on Mojave WK2. 
> > 
> > History:
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=fast%2Fforms%2Fauto-fill-button%2Fcaps-lock-
> > indicator-should-be-visible-after-hiding-auto-fill-strong-password-button.
> > html
> 
> Fixing

Committed fix in <https://trac.webkit.org/changeset/238571>.