Bug 191352 – Make it possible to edit images inline

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Make it possible to edit images inline

[Tim Horton]

Created attachment 354072 [details]
WIP

[Tim Horton]

<rdar://problem/30107985>

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Sam Weinig]

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).

[Tim Horton]

(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.

[Tim Horton]

Created attachment 354111 [details]
WIP

[Tim Horton]

Created attachment 354169 [details]
Patch

[EWS Watchlist]

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.

[Tim Horton]

Created attachment 354217 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Tim Horton]

I'm guessing the test failure has something to do with shouldBeNormalFlowOnly/shouldBeSelfPaintingLayer

[Tim Horton]

Created attachment 354246 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Dean Jackson]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Tim Horton]

(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!

[Tim Horton]

(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

[Tim Horton]

Created attachment 354389 [details]
Patch

[EWS Watchlist]

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.

[Tim Horton]

Created attachment 354401 [details]
Patch

[EWS Watchlist]

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.

[Simon Fraser (smfr)]

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)?

[Tim Horton]

Created attachment 354440 [details]
Patch

[EWS Watchlist]

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.

[Tim Horton]

Created attachment 354453 [details]
Patch

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 354453 [details]
Patch

Clearing flags on attachment: 354453

Committed r238065: <https://trac.webkit.org/changeset/238065>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r238065 for reason:

Breaks internal builds.

Committed r238070: <https://trac.webkit.org/changeset/238070>

[Tim Horton]

Created attachment 354575 [details]
Patch

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 354575 [details]
Patch

Clearing flags on attachment: 354575

Committed r238108: <https://trac.webkit.org/changeset/238108>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Tim Horton]

Will sort that out

[Ross Kirsling]

Fixed AppleWin build in r238152: <https://trac.webkit.org/changeset/238152>