WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
164538
[WK2] autocorrect and autocapitalize attributes do not work in contenteditable elements
https://bugs.webkit.org/show_bug.cgi?id=164538
Summary
[WK2] autocorrect and autocapitalize attributes do not work in contenteditabl...
Wenson Hsieh
Reported
2016-11-08 19:51:43 PST
1. Go to whsieh.github.io/examples/contenteditable on an iOS device 2. Type in the contenteditable divs directly underneath the "autocapitalize disabled" and "autocorrect disabled" headings 3. Observe that autocapitalization and autocorrect are enabled, even though their attributes are set to "none" and "off", respectively.
Attachments
Patch
(23.23 KB, patch)
2016-11-08 20:35 PST
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews113 for mac-yosemite
(1.92 MB, application/zip)
2016-11-10 00:33 PST
,
Build Bot
no flags
Details
Patch
(33.21 KB, patch)
2016-11-10 15:58 PST
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Fixed bad rebase in LayoutTests/ChangeLog
(32.58 KB, patch)
2016-11-10 16:21 PST
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Refactor layout tests + address other feedback
(38.88 KB, patch)
2016-11-10 21:02 PST
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Tweaked layout tests
(37.64 KB, patch)
2016-11-11 00:27 PST
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Remove WebAutocapitalizeType from WebCore
(58.55 KB, patch)
2016-11-11 08:32 PST
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Patch
(67.36 KB, patch)
2016-11-11 11:15 PST
,
Wenson Hsieh
rniwa
: review+
Details
Formatted Diff
Diff
Patch for landing
(137.18 KB, patch)
2016-11-11 14:56 PST
,
Wenson Hsieh
wenson_hsieh
: commit-queue+
Details
Formatted Diff
Diff
Patch for landing
(70.78 KB, text/plain)
2016-11-11 14:57 PST
,
Wenson Hsieh
wenson_hsieh
: commit-queue+
Details
Patch for landing
(71.02 KB, patch)
2016-11-11 15:05 PST
,
Wenson Hsieh
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
Patch for landing
(71.97 KB, patch)
2016-11-11 15:47 PST
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Wenson Hsieh
Comment 1
2016-11-08 19:54:50 PST
<
rdar://problem/8418711
>
Wenson Hsieh
Comment 2
2016-11-08 20:35:09 PST
Created
attachment 294212
[details]
Patch
Ryosuke Niwa
Comment 3
2016-11-08 21:59:43 PST
Note that these are non-standard attributes only supposed by WebKit and Blink.
Build Bot
Comment 4
2016-11-10 00:33:53 PST
Comment on
attachment 294212
[details]
Patch
Attachment 294212
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/2487756
New failing tests: storage/domstorage/sessionstorage/blocked-file-access.html
Build Bot
Comment 5
2016-11-10 00:33:56 PST
Created
attachment 294341
[details]
Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Joseph Pecoraro
Comment 6
2016-11-10 11:33:29 PST
Comment on
attachment 294212
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=294212&action=review
> Source/WebCore/ChangeLog:9 > + Adds autocorrect() and autocapitalizeType() to HTMLElements. These check the `autocorrect` and `autocapitalize`
Should we also move the IDL attributes to HTMLElement so that users can use elem.autocapitalize / elem.autocorrect getters/setters on any element? It seems weird to me that we would support contentEditable / spellcheck IDL attributes on any element, and effectively support autocorrect/autocapitalize, but the only way to toggle them dynamically in script would be with elem.setAttribute instead of normal getters/setters (which already exist on form controls, like <input>).
> Tools/WebKitTestRunner/ios/UIKitSPI.h:62 > +@interface UIKeyboardImpl : UIView > ++ (UIKeyboardImpl *)sharedInstance; > +- (void)removeAllDynamicDictionaries; > +@end
Any reason we are using the UIKeyboardImpl instance method and not the UIKeyboard class method?
> LayoutTests/ChangeLog:15 > + * fast/events/ios/contenteditable-autocapitalize-none-expected.txt: Added. > + * fast/events/ios/contenteditable-autocapitalize-none.html: Added. > + * fast/events/ios/contenteditable-autocorrect-off-expected.txt: Added. > + * fast/events/ios/contenteditable-autocorrect-off.html: Added.
Can we also have tests for the other cases? Most notably autocapitalize-sentences / autocapitalize-characters and autocorrect-on?
Wenson Hsieh
Comment 7
2016-11-10 12:29:33 PST
Comment on
attachment 294212
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=294212&action=review
>> Source/WebCore/ChangeLog:9 >> + Adds autocorrect() and autocapitalizeType() to HTMLElements. These check the `autocorrect` and `autocapitalize` > > Should we also move the IDL attributes to HTMLElement so that users can use elem.autocapitalize / elem.autocorrect getters/setters on any element? > > It seems weird to me that we would support contentEditable / spellcheck IDL attributes on any element, and effectively support autocorrect/autocapitalize, but the only way to toggle them dynamically in script would be with elem.setAttribute instead of normal getters/setters (which already exist on form controls, like <input>).
Sounds good -- I'll add IDL attributes for autocapitalize and autocorrect.
>> Tools/WebKitTestRunner/ios/UIKitSPI.h:62 >> +@end > > Any reason we are using the UIKeyboardImpl instance method and not the UIKeyboard class method?
Oh, I didn't see +[UIKeyboard removeAllDynamicDictionaries]! We should definitely be using that instead -- fixed.
>> LayoutTests/ChangeLog:15 >> + * fast/events/ios/contenteditable-autocorrect-off.html: Added. > > Can we also have tests for the other cases? Most notably autocapitalize-sentences / autocapitalize-characters and autocorrect-on?
Sounds good -- I'll add those as well.
Ryosuke Niwa
Comment 8
2016-11-10 12:44:24 PST
If we're adding this feature on Mac, I'd suggest that we propose it on Web Platform WG or editing TF.
Wenson Hsieh
Comment 9
2016-11-10 12:48:54 PST
(In reply to
comment #8
)
> If we're adding this feature on Mac, I'd suggest that we propose it on Web > Platform WG or editing TF.
These attributes are guarded by IOS_AUTOCORRECT_AND_AUTOCAPITALIZE, which appears to be only defined as 1 for iOS.
Ryosuke Niwa
Comment 10
2016-11-10 12:55:55 PST
(In reply to
comment #9
)
> (In reply to
comment #8
) > > If we're adding this feature on Mac, I'd suggest that we propose it on Web > > Platform WG or editing TF. > > These attributes are guarded by IOS_AUTOCORRECT_AND_AUTOCAPITALIZE, which > appears to be only defined as 1 for iOS.
Oh, you're just fixing this for iOS WK2.
Wenson Hsieh
Comment 11
2016-11-10 15:58:18 PST
Created
attachment 294428
[details]
Patch
Wenson Hsieh
Comment 12
2016-11-10 16:21:40 PST
Created
attachment 294437
[details]
Fixed bad rebase in LayoutTests/ChangeLog
Joseph Pecoraro
Comment 13
2016-11-10 17:56:36 PST
Comment on
attachment 294437
[details]
Fixed bad rebase in LayoutTests/ChangeLog View in context:
https://bugs.webkit.org/attachment.cgi?id=294437&action=review
Looks good to me!
> Source/WebCore/html/HTMLElement.h:26 > #include "StyledElement.h" > +#if ENABLE(IOS_AUTOCORRECT_AND_AUTOCAPITALIZE)
Style: You should have a blank line before the #if line.
> LayoutTests/fast/events/ios/contenteditable-autocapitalize-expected.txt:5 > +With autocapitalize: none, the output is: "to" > +With autocapitalize: sentences, the output is: "To" > +With autocapitalize: characters, the output is: "TO"
You could even test "words" but it requires a slightly longer test string. Either way this is a big improvement!
> LayoutTests/fast/events/ios/contenteditable-autocorrect-expected.txt:3 > +With autocorrect off, the result is: "Ti "
What is the special character here? Is it significant? Should we add <meta charset="utf-8"> to the test?
Wenson Hsieh
Comment 14
2016-11-10 18:02:39 PST
Comment on
attachment 294437
[details]
Fixed bad rebase in LayoutTests/ChangeLog View in context:
https://bugs.webkit.org/attachment.cgi?id=294437&action=review
>> Source/WebCore/html/HTMLElement.h:26 >> +#if ENABLE(IOS_AUTOCORRECT_AND_AUTOCAPITALIZE) > > Style: You should have a blank line before the #if line.
Got it -- added a newline.
>> LayoutTests/fast/events/ios/contenteditable-autocorrect-expected.txt:3 >> +With autocorrect off, the result is: "Ti " > > What is the special character here? Is it significant? Should we add <meta charset="utf-8"> to the test?
Oh, interesting -- it looks like we insert a non-breaking space when autocorrecting! I'll add <meta charset="utf-8"> -- that should make this test more robust if we decide to change things in the future.
Ryosuke Niwa
Comment 15
2016-11-10 18:06:29 PST
Comment on
attachment 294437
[details]
Fixed bad rebase in LayoutTests/ChangeLog View in context:
https://bugs.webkit.org/attachment.cgi?id=294437&action=review
> Source/WebCore/html/HTMLElement.cpp:1070 > +bool HTMLElement::autocorrect() const
This function should be called shouldAutocorrect instead?
> Source/WebCore/html/HTMLElement.cpp:1090 > +void HTMLElement::setAutocorrect(bool autocorrect) > +{ > + setAttributeWithoutSynchronization(autocorrectAttr, autocorrect ? AtomicString("on", AtomicString::ConstructFromLiteral) : AtomicString("off", AtomicString::ConstructFromLiteral)); > +}
Please put this right beneath autocorrect().
> Source/WebCore/html/HTMLElement.idl:52 > + [Conditional=IOS_AUTOCORRECT_AND_AUTOCAPITALIZE] attribute boolean autocorrect; > + [Conditional=IOS_AUTOCORRECT_AND_AUTOCAPITALIZE, TreatNullAs=EmptyString] attribute DOMString autocapitalize; > +
This would make all HTMLElement have these properties instead of just HTMLInputElement. Adding any property on HTMLElement can break content that relies on those properties to not exist. So at minimum, we should propose this in the standards.
> LayoutTests/fast/events/ios/contenteditable-autocapitalize.html:8 > + let write = s => output.innerHTML += s + "<br>";
Please don't use one letter variable like s.
> LayoutTests/fast/events/ios/contenteditable-autocapitalize.html:62 > + switch (progress) { > + case 2: > + outputEditableAndSetAutocapitalize("sentences"); > + break; > + case 3: > + testRunner.runUIScript(getUIScript(), incrementProgress); > + break; > + case 5: > + outputEditableAndSetAutocapitalize("characters"); > + break; > + case 6: > + testRunner.runUIScript(getUIScript(), incrementProgress); > + break; > + case 8: > + outputEditableAndSetAutocapitalize(null); > + break; > + case 9: > + testRunner.notifyDone(); > + break; > + }
Instead of having a giant switch, why can't we just use an array of functions that we loop over? Also, where did 4 & 7 go?
> LayoutTests/fast/events/ios/contenteditable-autocorrect.html:57 > + case 3: > + write(`With autocorrect off, the result is: "${editable.textContent}"`); > + editable.textContent = ""; > + editable.autocorrect = "on"; > + editable.blur(); > + break; > + case 4: > + write(`Trying again with autocorrect on:`); > + testRunner.runUIScript(getUIScript(), incrementProgress); > + break; > + case 8: > + write(`With autocorrect on, the result is: "${editable.textContent}"`); > + editable.textContent = ""; > + editable.blur(); > + break; > + case 9: > + testRunner.notifyDone(); > + break;
Again, numbers getting skipped is very cryptic.
Wenson Hsieh
Comment 16
2016-11-10 18:15:51 PST
Comment on
attachment 294437
[details]
Fixed bad rebase in LayoutTests/ChangeLog View in context:
https://bugs.webkit.org/attachment.cgi?id=294437&action=review
>> LayoutTests/fast/events/ios/contenteditable-autocapitalize.html:8 >> + let write = s => output.innerHTML += s + "<br>"; > > Please don't use one letter variable like s.
Ok -- I'll rename this.
>> LayoutTests/fast/events/ios/contenteditable-autocapitalize.html:62 >> + } > > Instead of having a giant switch, why can't we just use an array of functions that we loop over? > Also, where did 4 & 7 go?
That's a good idea! I'll change it to do that. 1, 4 and 7 are missing from the switch case because no action needs to be taken at those progress counts to move the test forward. This is because at those points in time, I'm waiting for the rest of the input to happen (for instance, when progress == 4, we've typed 't', and are waiting for the 'o' to arrive before logging the output and continuing with the next script).
>> LayoutTests/fast/events/ios/contenteditable-autocorrect.html:57 >> + break; > > Again, numbers getting skipped is very cryptic.
I'll employ the "array of progress handlers" tactic here.
Wenson Hsieh
Comment 17
2016-11-10 18:18:55 PST
Comment on
attachment 294437
[details]
Fixed bad rebase in LayoutTests/ChangeLog View in context:
https://bugs.webkit.org/attachment.cgi?id=294437&action=review
>> Source/WebCore/html/HTMLElement.cpp:1070 >> +bool HTMLElement::autocorrect() const > > This function should be called shouldAutocorrect instead?
Sounds good -- renamed.
>> Source/WebCore/html/HTMLElement.cpp:1090 >> +} > > Please put this right beneath autocorrect().
Done!
>> Source/WebCore/html/HTMLElement.idl:52 >> + > > This would make all HTMLElement have these properties instead of just HTMLInputElement. > Adding any property on HTMLElement can break content that relies on those properties to not exist. > So at minimum, we should propose this in the standards.
Ok. I will propose this.
Wenson Hsieh
Comment 18
2016-11-10 21:02:40 PST
Created
attachment 294468
[details]
Refactor layout tests + address other feedback
Ryosuke Niwa
Comment 19
2016-11-10 21:33:10 PST
Comment on
attachment 294468
[details]
Refactor layout tests + address other feedback View in context:
https://bugs.webkit.org/attachment.cgi?id=294468&action=review
> Source/WebCore/html/HTMLFormControlElement.h:96 > + WEBCORE_EXPORT WebAutocapitalizeType autocapitalizeType() const final;
Web* type shouldn't really be used in WebCore. We should add a new enum to be used in WebCore which then gets converted to Web* in WebKit layer.
> LayoutTests/fast/events/ios/contenteditable-autocapitalize.html:9 > + function getUIScript() {
This doesn't really need to be a function. I think it's cleaner to have this template literal inside runUIScript where you call testRunner.runUIScript.
> LayoutTests/fast/events/ios/contenteditable-autocapitalize.html:79 > + waitForNumberOfInputEvents(2).then(function() { > + outputEditableAndSetAutocapitalize("sentences"); > + });
This still has a problem that it's unclear why we have to wait for 2. A better approach is to wrap the whole process of running UI script and waiting for the events in a promise. e.g. let remainingInputEventCount = 0; let resolveInputEvent = null; function handleInput() { remainingInputEventCount--; if (resolveInputEvent && !remainingInputEventCount) resolveInputEvent(); } function runUIScriptAndWait(inputEventCount = 2) { return Promise.all([new Promise((resolve) => { remainingInputEventCount = inputEventCount; resolveInputEvent = resolve; }), runUIScript()]); } function runTest() { runUIScriptAndWait().then(() => outputEditableAndSetAutocapitalize("sentences")) .then(() => runUIScriptAndWait()).then(() => outputEditableAndSetAutocapitalize("characters")) .then(() => runUIScriptAndWait()).then(() => outputEditableAndSetAutocapitalize(null)); }
Wenson Hsieh
Comment 20
2016-11-10 23:45:39 PST
Comment on
attachment 294468
[details]
Refactor layout tests + address other feedback View in context:
https://bugs.webkit.org/attachment.cgi?id=294468&action=review
>> Source/WebCore/html/HTMLFormControlElement.h:96 >> + WEBCORE_EXPORT WebAutocapitalizeType autocapitalizeType() const final; > > Web* type shouldn't really be used in WebCore. We should add a new enum to be used in WebCore > which then gets converted to Web* in WebKit layer.
I agree that "Web-" prefixed enums don't really belong in WebCore. However, it's not actually clear to me why there needs to be a Web- prefix here in the first place, considering we use WebCore classes in other places in WebKit and WebKit2 as well (e.g. various types of HTMLElements). Is there a reason this can't just be AutocapitalizeType? Either way, I think this refactoring would make more sense as a separate patch.
>> LayoutTests/fast/events/ios/contenteditable-autocapitalize.html:9 >> + function getUIScript() { > > This doesn't really need to be a function. > I think it's cleaner to have this template literal inside runUIScript where you call testRunner.runUIScript.
Good point -- folded into runUIScript.
>> LayoutTests/fast/events/ios/contenteditable-autocapitalize.html:79 >> + }); > > This still has a problem that it's unclear why we have to wait for 2. > > A better approach is to wrap the whole process of running UI script and waiting for the events in a promise. > e.g. > > let remainingInputEventCount = 0; > let resolveInputEvent = null; > function handleInput() { > remainingInputEventCount--; > if (resolveInputEvent && !remainingInputEventCount) > resolveInputEvent(); > } > > function runUIScriptAndWait(inputEventCount = 2) { > return Promise.all([new Promise((resolve) => { > remainingInputEventCount = inputEventCount; > resolveInputEvent = resolve; > }), runUIScript()]); > } > > function runTest() { > runUIScriptAndWait().then(() => outputEditableAndSetAutocapitalize("sentences")) > .then(() => runUIScriptAndWait()).then(() => outputEditableAndSetAutocapitalize("characters")) > .then(() => runUIScriptAndWait()).then(() => outputEditableAndSetAutocapitalize(null)); > }
runUIScript will never resolve before the expected input event count promise resolves, since the UI script completes when the keyboard is done dismissing. This only happens after the expected input events have been observed and outputEditableAndSetAutocapitalize has been invoked, so the approach above causes us to time out. I can try to refactor the test here so that we can follow a runUIScriptAndWait().then()... pattern.
Wenson Hsieh
Comment 21
2016-11-11 00:27:58 PST
Created
attachment 294480
[details]
Tweaked layout tests
Ryosuke Niwa
Comment 22
2016-11-11 01:19:42 PST
(In reply to
comment #20
)
> Comment on
attachment 294468
[details]
> Refactor layout tests + address other feedback > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=294468&action=review
> > >> Source/WebCore/html/HTMLFormControlElement.h:96 > >> + WEBCORE_EXPORT WebAutocapitalizeType autocapitalizeType() const final; > > > > Web* type shouldn't really be used in WebCore. We should add a new enum to be used in WebCore > > which then gets converted to Web* in WebKit layer. > > I agree that "Web-" prefixed enums don't really belong in WebCore. However, > it's not actually clear to me why there needs to be a Web- prefix here in > the first place, considering we use WebCore classes in other places in > WebKit and WebKit2 as well (e.g. various types of HTMLElements). Is there a > reason this can't just be AutocapitalizeType?
If Web- prefixes are used outside WebCore, then it needs to be separate enums from the ones used in WebCore for licensing reasons. WebCore is LGPL. WebKit/WebKit2 are BSD.
> >> LayoutTests/fast/events/ios/contenteditable-autocapitalize.html:9 > >> + function getUIScript() { > > > > This doesn't really need to be a function. > > I think it's cleaner to have this template literal inside runUIScript where you call testRunner.runUIScript. > > Good point -- folded into runUIScript. > > >> LayoutTests/fast/events/ios/contenteditable-autocapitalize.html:79 > >> + }); > > > > This still has a problem that it's unclear why we have to wait for 2. > > > > A better approach is to wrap the whole process of running UI script and waiting for the events in a promise. > > e.g. > > > > let remainingInputEventCount = 0; > > let resolveInputEvent = null; > > function handleInput() { > > remainingInputEventCount--; > > if (resolveInputEvent && !remainingInputEventCount) > > resolveInputEvent(); > > } > > > > function runUIScriptAndWait(inputEventCount = 2) { > > return Promise.all([new Promise((resolve) => { > > remainingInputEventCount = inputEventCount; > > resolveInputEvent = resolve; > > }), runUIScript()]); > > } > > > > function runTest() { > > runUIScriptAndWait().then(() => outputEditableAndSetAutocapitalize("sentences")) > > .then(() => runUIScriptAndWait()).then(() => outputEditableAndSetAutocapitalize("characters")) > > .then(() => runUIScriptAndWait()).then(() => outputEditableAndSetAutocapitalize(null)); > > } > > runUIScript will never resolve before the expected input event count promise > resolves, since the UI script completes when the keyboard is done > dismissing. This only happens after the expected input events have been > observed and outputEditableAndSetAutocapitalize has been invoked, so the > approach above causes us to time out.
What is outputEditableAndSetAutocapitalize anything to do with UI scripts? It is somehow updating some dates to which UI script responds? It would be misleading to do that in a function called outputEditableAndSetAutocapitalize. We should probably separate that into a separate function which communicates back to the UI script.
Wenson Hsieh
Comment 23
2016-11-11 07:27:46 PST
(In reply to
comment #22
)
> (In reply to
comment #20
) > > runUIScript will never resolve before the expected input event count promise > > resolves, since the UI script completes when the keyboard is done > > dismissing. This only happens after the expected input events have been > > observed and outputEditableAndSetAutocapitalize has been invoked, so the > > approach above causes us to time out. > > What is outputEditableAndSetAutocapitalize anything to do with UI scripts? > It is somehow updating some dates to which UI script responds? It would be > misleading to do that in a function called > outputEditableAndSetAutocapitalize. We should probably separate that into a > separate function which communicates back to the UI script.
outputEditableAndSetAutocapitalize dismisses the keyboard by blurring the contenteditable. The UI script does not complete until the keyboard has dismissed. I've actually removed outputEditableAndSetAutocapitalize (and outputEditableAndSetAutocorrect) entirely and moved the logic into runUIScriptAndExpectInputEvents, so runTest() now looks something like: runUIScriptAndExpectInputEvents(2, "sentences") .then(() => runUIScriptAndExpectInputEvents(2, "characters")) .then(() => runUIScriptAndExpectInputEvents(2, null)) .then(() => testRunner.notifyDone());
Wenson Hsieh
Comment 24
2016-11-11 08:32:34 PST
Created
attachment 294498
[details]
Remove WebAutocapitalizeType from WebCore
Joseph Pecoraro
Comment 25
2016-11-11 10:05:59 PST
Comment on
attachment 294498
[details]
Remove WebAutocapitalizeType from WebCore View in context:
https://bugs.webkit.org/attachment.cgi?id=294498&action=review
> Source/WebKit/mac/DOM/WebAutocapitalizeTypes.h:26 > +#pragma once
This normally is not needed in ObjC headers. Speaking of the ObjC DOM APIs, should we move autocorrect and autocapitalize to DOMHTMLElement to match the IDL?
Wenson Hsieh
Comment 26
2016-11-11 11:05:11 PST
(In reply to
comment #25
)
> Comment on
attachment 294498
[details]
> Remove WebAutocapitalizeType from WebCore > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=294498&action=review
> > > Source/WebKit/mac/DOM/WebAutocapitalizeTypes.h:26 > > +#pragma once > > This normally is not needed in ObjC headers.
Good catch -- removed.
> > Speaking of the ObjC DOM APIs, should we move autocorrect and autocapitalize > to DOMHTMLElement to match the IDL?
Yes, this seems reasonable. I'll make this change.
Wenson Hsieh
Comment 27
2016-11-11 11:15:22 PST
Created
attachment 294510
[details]
Patch
Ryosuke Niwa
Comment 28
2016-11-11 13:30:25 PST
Comment on
attachment 294510
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=294510&action=review
> Source/WebCore/html/Autocapitalize.h:39 > +typedef enum { > + AutocapitalizeTypeDefault, > + AutocapitalizeTypeNone, > + AutocapitalizeTypeWords, > + AutocapitalizeTypeSentences, > + AutocapitalizeTypeAllCharacters > +} AutocapitalizeType;
Please use modern C++ style enum: enum class AutocapitalizeType { Default, ... };
> Source/WebCore/html/HTMLElement.h:27 > +#if ENABLE(IOS_AUTOCORRECT_AND_AUTOCAPITALIZE) > +#include "Autocapitalize.h" > +#endif
We typically put this if-def inside the header instead.
> Source/WebKit/mac/DOM/WebAutocapitalizeTypes.h:32 > +typedef enum { > + WebAutocapitalizeTypeDefault, > + WebAutocapitalizeTypeNone, > + WebAutocapitalizeTypeWords, > + WebAutocapitalizeTypeSentences, > + WebAutocapitalizeTypeAllCharacters > +} WebAutocapitalizeType;
Please use modern enum syntax. Even if you couldn't use enum class, please use: enum WebAutocapitalizeType { ~ }; instead as C++ doesn't require typedef for enums.
> Source/WebKit2/Shared/AssistedNodeInformation.h:36 > +typedef enum {
Ditto about not using typedef. I'm a bit confused here. I think you misunderstood what I said. It's fine for WebCore header & enum to be used in WebKit1 / WebKit2 code. What I mean is that WebKit1 / WebKit2 can't export those enums as API. Since we're already converting these enums to UIKit types in WebKit1 / WebKit2, we don't need a separate enum inside them. Furthermore, relying on these two declarations to match is fragile.
Ryosuke Niwa
Comment 29
2016-11-11 13:31:09 PST
Comment on
attachment 294510
[details]
Patch r=me provided you get rid of enum declarations in WebKit & WebKit2.
Wenson Hsieh
Comment 30
2016-11-11 13:59:40 PST
(In reply to
comment #29
)
> Comment on
attachment 294510
[details]
> Patch > > r=me provided you get rid of enum declarations in WebKit & WebKit2.
Thanks! I'll remove WKAutocapitalizeType. However, we still need a WebAutocapitalizeType declared, otherwise we'll break UIKit which uses WebKitLegacy's WebAutocapitalizeType. There doesn't seem to be a reason for UIKit to be doing this though, and I think we should change UIKit to not care about WebAutocapitalizeType (and just receive a UITextAutocapitalizationType from WebKitLegacy). I will introduce a WebAutocapitalizeType, but make the conversion more robust using a helper function with a switch case.
Wenson Hsieh
Comment 31
2016-11-11 14:56:13 PST
Created
attachment 294542
[details]
Patch for landing
Wenson Hsieh
Comment 32
2016-11-11 14:57:59 PST
Created
attachment 294543
[details]
Patch for landing
Wenson Hsieh
Comment 33
2016-11-11 15:05:36 PST
Created
attachment 294547
[details]
Patch for landing
WebKit Commit Bot
Comment 34
2016-11-11 15:31:24 PST
Comment on
attachment 294547
[details]
Patch for landing Rejecting
attachment 294547
[details]
from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 294547, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: fs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 208614 = b6b117a0e51cdf7c69db580820553ecb9c0eecbf
r208615
= f991ec35da7209373ed165c0906a2bc60495960e
r208616
= d7dac224a36de4546d35bbb4b568156ea2b82e4f
r208617
= 2fbddb6e7619c8ab572795f1d2513e9c3032cda7 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output:
http://webkit-queues.webkit.org/results/2500173
Wenson Hsieh
Comment 35
2016-11-11 15:47:17 PST
Created
attachment 294553
[details]
Patch for landing
WebKit Commit Bot
Comment 36
2016-11-11 16:23:07 PST
Comment on
attachment 294553
[details]
Patch for landing Clearing flags on attachment: 294553 Committed
r208624
: <
http://trac.webkit.org/changeset/208624
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug