| Summary: | Add experimental support for the "formatForeColor" inputType | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||
| Component: | UI Events | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | bdakin, buildbot, commit-queue, enrica, rniwa, webkit-bug-importer | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 163112 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Wenson Hsieh
2016-10-12 11:45:46 PDT
Created attachment 291379 [details]
Patch
Comment on attachment 291379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291379&action=review > Source/WebCore/editing/Editor.cpp:145 > + auto colorValue = properties->getPropertyCSSValue(CSSPropertyColor); > + ASSERT(colorValue); > + return colorValue->cssText(); Why don't we just call getPropertyValue which returns String? > LayoutTests/fast/events/input-events-forecolor-data.html:19 > + if (event.inputType === "formatForeColor") > + shouldBe("event.data", "'rgb(255, 255, 255)'"); I'm not certain the functional syntax is what we want to be using here but I guess that'd be consistent with CSSOM: https://drafts.csswg.org/cssom/#serialize-a-css-component-value Comment on attachment 291379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291379&action=review Sorry, a few more comments. > LayoutTests/fast/events/input-events-forecolor-data-expected.txt:2 > +PASS event.data is 'rgb(255, 255, 255)' > +PASS event.data is 'rgb(255, 255, 255)' It's not clear why these results are correct. At minimum we should print out the event name. > LayoutTests/fast/events/input-events-forecolor-data.html:12 > + document.getElementById("editable").focus(); > + testRunner.execCommand("ForeColor", "rgb(255, 255, 255)"); You don't need to wrap this inside the if. execCommand('foreColor') always works and so does focus(). You should probably test more colors though. Comment on attachment 291379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291379&action=review >> Source/WebCore/editing/Editor.cpp:145 >> + return colorValue->cssText(); > > Why don't we just call getPropertyValue which returns String? Good point! Changed to return getPropertyValue(). >> LayoutTests/fast/events/input-events-forecolor-data-expected.txt:2 >> +PASS event.data is 'rgb(255, 255, 255)' > > It's not clear why these results are correct. At minimum we should print out the event name. Sounds good. I'll make it more clear by printing out the event type/inputType. >> LayoutTests/fast/events/input-events-forecolor-data.html:12 >> + testRunner.execCommand("ForeColor", "rgb(255, 255, 255)"); > > You don't need to wrap this inside the if. execCommand('foreColor') always works and so does focus(). > > You should probably test more colors though. Ok, done! Also added more colors in this test. >> LayoutTests/fast/events/input-events-forecolor-data.html:19 >> + shouldBe("event.data", "'rgb(255, 255, 255)'"); > > I'm not certain the functional syntax is what we want to be using here > but I guess that'd be consistent with CSSOM: https://drafts.csswg.org/cssom/#serialize-a-css-component-value Good point -- let's keep this issue in mind. Created attachment 291384 [details]
Patch for landing
Comment on attachment 291379 [details] Patch Attachment 291379 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2272193 New failing tests: fast/events/input-events-forecolor-data.html Created attachment 291386 [details]
Archive of layout-test-results from ews101 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 291394 [details]
Patch for landing
Comment on attachment 291394 [details] Patch for landing Clearing flags on attachment: 291394 Committed r207244: <http://trac.webkit.org/changeset/207244> Comment on attachment 291379 [details] Patch Attachment 291379 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2272726 New failing tests: fast/events/input-events-forecolor-data.html Created attachment 291401 [details]
Archive of layout-test-results from ews114 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
|