Make it possible to edit images inline
Created attachment 354072 [details] WIP
<rdar://problem/30107985>
Attachment 354072 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:960: developmentRegion is not en. [xcodeproj/settings] [5] Total errors found: 1 in 54 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 354072 [details] WIP Attachment 354072 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9891149 New failing tests: css3/flexbox/z-index.html
Created attachment 354076 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 354072 [details] WIP Attachment 354072 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9891569 Number of test failures exceeded the failure limit.
Created attachment 354078 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 354072 [details] WIP Attachment 354072 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9892645 New failing tests: css3/flexbox/z-index.html
Created attachment 354084 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
I think you forgot to upload the whole patch. From what I can see in the WebCore part, will this provide a generic mechanism for hosting arbitrary views (NS or UI) as an element (I'm getting this from the generic sounding EmbeddedView enum value in GraphicsLayer).
(In reply to Sam Weinig from comment #10) > I think you forgot to upload the whole patch. > > From what I can see in the WebCore part, will this provide a generic > mechanism for hosting arbitrary views (NS or UI) as an element (I'm getting > this from the generic sounding EmbeddedView enum value in GraphicsLayer). It’s also adopted for contenteditable image, if a pref is turned on. But right now it just plops in a native view, doesn’t do any serialization of the image in either direction, etc. There’s more to do! But this is the first step. Just need a changelog.
Created attachment 354111 [details] WIP
Created attachment 354169 [details] Patch
Attachment 354169 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:960: developmentRegion is not en. [xcodeproj/settings] [5] ERROR: Tools/TestRunnerShared/spi/PencilKitTestSPI.h:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 61 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 354217 [details] Patch
Attachment 354217 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:960: developmentRegion is not en. [xcodeproj/settings] [5] ERROR: Tools/TestRunnerShared/spi/PencilKitTestSPI.h:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 61 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 354217 [details] Patch Attachment 354217 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9906800 New failing tests: css3/flexbox/z-index.html
Created attachment 354227 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 354217 [details] Patch Attachment 354217 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9906819 New failing tests: css3/flexbox/z-index.html
Created attachment 354229 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 354217 [details] Patch Attachment 354217 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9906913 New failing tests: css3/flexbox/z-index.html
Created attachment 354231 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 354217 [details] Patch Attachment 354217 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9907775 New failing tests: css3/flexbox/z-index.html
Created attachment 354233 [details] Archive of layout-test-results from ews114 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
I'm guessing the test failure has something to do with shouldBeNormalFlowOnly/shouldBeSelfPaintingLayer
Created attachment 354246 [details] Patch
Attachment 354246 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:960: developmentRegion is not en. [xcodeproj/settings] [5] ERROR: Tools/TestRunnerShared/spi/PencilKitTestSPI.h:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 61 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 354246 [details] Patch Attachment 354246 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9910833 New failing tests: css3/flexbox/z-index.html
Created attachment 354254 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 354246 [details] Patch Attachment 354246 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9911083 New failing tests: css3/flexbox/z-index.html
Created attachment 354260 [details] Archive of layout-test-results from ews114 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 354246 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354246&action=review I feel like the EmbeddedView part should be a separate patch, and I'm not really qualified to review, but whatever 🤗 > Source/WebCore/platform/graphics/GraphicsLayer.cpp:745 > + return ++nextEmbeddedViewID; Shouldn't "next" mean nextEmbeddedViewID++? Or is 0 special? (/me reads backwards and discovers that 0 is actually special in that it is treated as no assigned ID) > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1170 > + contentsLayerChanged = true; Why not just put noteSublayersChanged() here and avoid contentsLayerChanged entirely? > Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm:1031 > + return { }; Why { } vs 0? > Source/WebCore/rendering/RenderImage.cpp:227 > + auto editableValue = element()->attributeWithoutSynchronization(x_apple_editable_imageAttr); > + if (editableValue.isNull()) > + return false; > + > + return editableValue.isEmpty() || equalLettersIgnoringASCIICase(editableValue, "true"); I think this should follow the controls attribute behaviour on HTMLMediaElement. e.g. return element()->hasAttributeWithoutSynchronization(x_apple_editable_imageAttr); and don't even look at the value. > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreeTransaction.mm:44 > + , embeddedViewID(0) You use 0 here :) Also, why isn't this in the .h file? > Source/WebKit/Shared/WebPreferences.yaml:1187 > +EditableImagesEnabled: > + type: bool > + defaultValue: false No webcoreBinding needed? > LayoutTests/editing/images/basic-editable-image.html:13 > + const numberOfStrokesInEditableImageAfterDrawing = (await UIHelper.numberOfStrokesInEditableImage()); Please use WebML to detect if it was a square. Nit: No need for the () around awaits. > LayoutTests/editing/images/basic-editable-image.html:15 > + testRunner.notifyDone(); No need to do this - I believe we wait until all promises are resolved by default.
Comment on attachment 354246 [details] Patch Attachment 354246 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9912397 New failing tests: css3/flexbox/z-index.html
Created attachment 354272 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 354246 [details] Patch Attachment 354246 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9912298 New failing tests: css3/flexbox/z-index.html
Created attachment 354274 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6
(In reply to Dean Jackson from comment #32) > Comment on attachment 354246 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=354246&action=review > > I feel like the EmbeddedView part should be a separate patch, and I'm not > really qualified to review, but whatever 🤗 I didn't want to do that because it's not testable by itself. > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1170 > > + contentsLayerChanged = true; > > Why not just put noteSublayersChanged() here and avoid contentsLayerChanged > entirely? Sure > > Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm:1031 > > + return { }; > > Why { } vs 0? 🤗 > > Source/WebCore/rendering/RenderImage.cpp:227 > > + auto editableValue = element()->attributeWithoutSynchronization(x_apple_editable_imageAttr); > > + if (editableValue.isNull()) > > + return false; > > + > > + return editableValue.isEmpty() || equalLettersIgnoringASCIICase(editableValue, "true"); > > I think this should follow the controls attribute behaviour on > HTMLMediaElement. e.g. > > return > element()->hasAttributeWithoutSynchronization(x_apple_editable_imageAttr); > > and don't even look at the value. GOOD IDEA > > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreeTransaction.mm:44 > > + , embeddedViewID(0) > > You use 0 here :) > > Also, why isn't this in the .h file? Because I didn't want to move all of them. > > Source/WebKit/Shared/WebPreferences.yaml:1187 > > +EditableImagesEnabled: > > + type: bool > > + defaultValue: false > > No webcoreBinding needed? Defaults to a WebCore::Setting with the same name, right? > > LayoutTests/editing/images/basic-editable-image.html:13 > > + const numberOfStrokesInEditableImageAfterDrawing = (await UIHelper.numberOfStrokesInEditableImage()); > > Please use WebML to detect if it was a square. Patches welcome. > Nit: No need for the () around awaits. > > LayoutTests/editing/images/basic-editable-image.html:15 > > + testRunner.notifyDone(); > > No need to do this - I believe we wait until all promises are resolved by > default. Both good to know!
(In reply to Tim Horton from comment #37) > (In reply to Dean Jackson from comment #32) > > Comment on attachment 354246 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=354246&action=review > > > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1170 > > > + contentsLayerChanged = true; > > > > Why not just put noteSublayersChanged() here and avoid contentsLayerChanged > > entirely? > > Sure Wait, no, it's because if you're setting to 0 and it's already 0 we won't rebuild :P
Created attachment 354389 [details] Patch
Attachment 354389 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:960: developmentRegion is not en. [xcodeproj/settings] [5] ERROR: Tools/TestRunnerShared/spi/PencilKitTestSPI.h:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 61 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 354401 [details] Patch
Attachment 354401 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:960: developmentRegion is not en. [xcodeproj/settings] [5] ERROR: Tools/TestRunnerShared/spi/PencilKitTestSPI.h:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 61 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 354401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354401&action=review > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1168 > + m_contentsLayer = createPlatformCALayerForEmbeddedView(platformType, embeddedViewID, this); Probably don't want to create this very time. > Source/WebCore/platform/graphics/ca/PlatformCALayer.h:80 > + LayerTypeCustom, > + LayerTypeEditableImageLayer, I would keep Custom at the end. > Source/WebCore/rendering/RenderLayerCompositor.cpp:2464 > + m_reevaluateCompositingAfterLayout = true; I don't think you want this. > Source/WebKit/UIProcess/ios/WKDrawingView.mm:47 > + _pencilView = adoptNS([allocPKCanvasViewInstance() initWithFrame:CGRectMake(0, 0, 768, 400)]); Hardcoded frame! > Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:787 > +PKCanvasView *findEditableImageCanvas(void) (void)?
Created attachment 354440 [details] Patch
Attachment 354440 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:960: developmentRegion is not en. [xcodeproj/settings] [5] ERROR: Tools/TestRunnerShared/spi/PencilKitTestSPI.h:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 61 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 354453 [details] Patch
Attachment 354453 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:960: developmentRegion is not en. [xcodeproj/settings] [5] ERROR: Tools/TestRunnerShared/spi/PencilKitTestSPI.h:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 60 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 354453 [details] Patch Clearing flags on attachment: 354453 Committed r238065: <https://trac.webkit.org/changeset/238065>
All reviewed patches have been landed. Closing bug.
Reverted r238065 for reason: Breaks internal builds. Committed r238070: <https://trac.webkit.org/changeset/238070>
Created attachment 354575 [details] Patch
Attachment 354575 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:960: developmentRegion is not en. [xcodeproj/settings] [5] ERROR: Tools/TestRunnerShared/spi/PencilKitTestSPI.h:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 60 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 354575 [details] Patch Clearing flags on attachment: 354575 Committed r238108: <https://trac.webkit.org/changeset/238108>
This change broke the Windows build: C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\graphics\ca\win\PlatformCALayerWin.cpp(50): error C2259: 'WebCore::PlatformCALayerWin': cannot instantiate abstract class [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\graphics\ca\win\PlatformCALayerWin.cpp(50): note: due to following members: C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\graphics\ca\win\PlatformCALayerWin.cpp(50): note: 'WebCore::GraphicsLayer::EmbeddedViewID WebCore::PlatformCALayer::embeddedViewID(void) const': is abstract C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\graphics\ca\PlatformCALayer.h(240): note: see declaration of 'WebCore::PlatformCALayer::embeddedViewID' C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\graphics\ca\win\PlatformCALayerWin.cpp(55): error C2259: 'WebCore::PlatformCALayerWin': cannot instantiate abstract class [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\graphics\ca\win\PlatformCALayerWin.cpp(55): note: due to following members: C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\graphics\ca\win\PlatformCALayerWin.cpp(55): note: 'WebCore::GraphicsLayer::EmbeddedViewID WebCore::PlatformCALayer::embeddedViewID(void) const': is abstract C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\graphics\ca\PlatformCALayer.h(240): note: see declaration of 'WebCore::PlatformCALayer::embeddedViewID https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Build%29/builds/252/steps/compile-webkit/logs/stdio
Will sort that out
Fixed AppleWin build in r238152: <https://trac.webkit.org/changeset/238152>