Bug 180651

Summary: Add more auto fill button types
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, ews-watchlist, rniwa, sam, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 180801    
Attachments:
Description Flags
Patch and layout tests
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Patch and layout tests
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Patch and layout tests
none
Archive of layout-test-results from ews101 for mac-elcapitan
none
Patch and layout tests
none
Patch and layout tests bfulgham: review+, bfulgham: commit-queue-

Daniel Bates
Reported 2017-12-11 09:24:26 PST
Add more auto fill button types.
Attachments
Patch and layout tests (63.16 KB, patch)
2017-12-11 10:06 PST, Daniel Bates
no flags
Archive of layout-test-results from ews103 for mac-elcapitan (2.30 MB, application/zip)
2017-12-11 11:04 PST, EWS Watchlist
no flags
Patch and layout tests (63.01 KB, patch)
2017-12-11 11:18 PST, Daniel Bates
no flags
Archive of layout-test-results from ews100 for mac-elcapitan (2.25 MB, application/zip)
2017-12-11 12:11 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews115 for mac-elcapitan (3.23 MB, application/zip)
2017-12-11 12:33 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.50 MB, application/zip)
2017-12-11 12:50 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (2.60 MB, application/zip)
2017-12-11 12:53 PST, EWS Watchlist
no flags
Patch and layout tests (63.08 KB, patch)
2017-12-11 13:54 PST, Daniel Bates
no flags
Archive of layout-test-results from ews101 for mac-elcapitan (2.17 MB, application/zip)
2017-12-11 14:55 PST, EWS Watchlist
no flags
Patch and layout tests (63.09 KB, patch)
2017-12-11 15:03 PST, Daniel Bates
no flags
Patch and layout tests (65.59 KB, patch)
2017-12-11 16:30 PST, Daniel Bates
bfulgham: review+
bfulgham: commit-queue-
Daniel Bates
Comment 1 2017-12-11 09:24:43 PST
Daniel Bates
Comment 2 2017-12-11 10:06:29 PST
Created attachment 328992 [details] Patch and layout tests
EWS Watchlist
Comment 3 2017-12-11 11:04:23 PST Comment hidden (obsolete)
EWS Watchlist
Comment 4 2017-12-11 11:04:24 PST Comment hidden (obsolete)
Daniel Bates
Comment 5 2017-12-11 11:18:02 PST
Created attachment 329002 [details] Patch and layout tests
EWS Watchlist
Comment 6 2017-12-11 12:11:28 PST Comment hidden (obsolete)
EWS Watchlist
Comment 7 2017-12-11 12:11:30 PST Comment hidden (obsolete)
EWS Watchlist
Comment 8 2017-12-11 12:33:42 PST Comment hidden (obsolete)
EWS Watchlist
Comment 9 2017-12-11 12:33:43 PST Comment hidden (obsolete)
EWS Watchlist
Comment 10 2017-12-11 12:50:50 PST Comment hidden (obsolete)
EWS Watchlist
Comment 11 2017-12-11 12:50:51 PST Comment hidden (obsolete)
EWS Watchlist
Comment 12 2017-12-11 12:53:00 PST Comment hidden (obsolete)
EWS Watchlist
Comment 13 2017-12-11 12:53:02 PST Comment hidden (obsolete)
Daniel Bates
Comment 14 2017-12-11 13:54:18 PST
Created attachment 329029 [details] Patch and layout tests
EWS Watchlist
Comment 15 2017-12-11 14:55:07 PST Comment hidden (obsolete)
EWS Watchlist
Comment 16 2017-12-11 14:55:08 PST Comment hidden (obsolete)
Daniel Bates
Comment 17 2017-12-11 15:03:56 PST
Created attachment 329041 [details] Patch and layout tests
Daniel Bates
Comment 18 2017-12-11 16:30:03 PST
Created attachment 329054 [details] Patch and layout tests
Brent Fulgham
Comment 19 2017-12-13 09:07:57 PST
Comment on attachment 329054 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=329054&action=review Looks good. I had a minor suggestion about RefPtr -> Ref, and I think you should have two separate AX labels. But otherwise this is great. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3089 > + return @"strong password"; I think these should be separate roles. I think it would be confusing to get the same role reported by AX -- how would a blind person know which of the password fields they were on? > Source/WebCore/html/HTMLInputElement.cpp:2046 > + firstStop.m_position = CSSValuePool::singleton().createValue(3, CSSPrimitiveValue::UnitType::CSS_EMS); Not to be addressed in this patch: It sure seems like these should be constructor arguments, with the CSSValuePool::singleton being used inside the constructor! This is a lot of extra typing! > Source/WebCore/html/HTMLInputElement.cpp:2056 > + return gradient; It seems weird to return a RefPtr<>, when it's only use case immediately dereferences the pointer. Should we return a Ref<> here instead? > Source/WebCore/html/HTMLInputElement.h:247 > + AutoFillButtonType autoFillButtonType() const { return static_cast<AutoFillButtonType>(m_autoFillButtonType); } +1 :-) > Source/WebCore/html/TextFieldInputType.cpp:411 > + return AXAutoFillStrongPasswordLabel(); I think these should be two separate AX labels. > Source/WebCore/platform/LocalizedStrings.cpp:646 > +} Please add a label for the confirmation password field.
Daniel Bates
Comment 20 2017-12-13 16:16:08 PST
Comment on attachment 329054 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=329054&action=review >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3089 >> + return @"strong password"; > > I think these should be separate roles. I think it would be confusing to get the same role reported by AX -- how would a blind person know which of the password fields they were on? Will fix. The strong password and strong confirmation password buttons will likely trigger the same action and hence it may be sufficient for them to be considered having the same role. It does not hurt to consider them separate for now until we finalize the accessibility story for these buttons. >> Source/WebCore/html/HTMLInputElement.cpp:2046 >> + firstStop.m_position = CSSValuePool::singleton().createValue(3, CSSPrimitiveValue::UnitType::CSS_EMS); > > Not to be addressed in this patch: It sure seems like these should be constructor arguments, with the CSSValuePool::singleton being used inside the constructor! This is a lot of extra typing! Notice that the unit type of the gradient position can be arbitrary and depends on the look that a person is trying to achieve with the gradient. I am unclear of the value of using a such a constructor given this constraint. >> Source/WebCore/html/HTMLInputElement.cpp:2056 >> + return gradient; > > It seems weird to return a RefPtr<>, when it's only use case immediately dereferences the pointer. Should we return a Ref<> here instead? Will fix. >> Source/WebCore/html/TextFieldInputType.cpp:411 >> + return AXAutoFillStrongPasswordLabel(); > > I think these should be two separate AX labels. Will do. >> Source/WebCore/platform/LocalizedStrings.cpp:646 >> +} > > Please add a label for the confirmation password field. Will do.
Daniel Bates
Comment 21 2017-12-13 16:29:41 PST
Daniel Bates
Comment 22 2017-12-13 17:02:44 PST
Committed Windows build fix in <https://trac.webkit.org/changeset/225885>.
Daniel Bates
Comment 23 2017-12-13 18:38:36 PST
Committed updated results in <https://trac.webkit.org/changeset/225889>.
Daniel Bates
Comment 24 2017-12-13 23:02:04 PST
Committed macOS El Capitan and Apple Windows results in <https://trac.webkit.org/changeset/225894>.
Note You need to log in before you can comment on or make changes to this bug.