Bug 163348

Created attachment 291379 [details] Patch

<rdar://problem/28739334>

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