Bug 239423 – [macOS] Image controls are editable and prevent drops in editable web views

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

rdar://81027082

[Wenson Hsieh]

Created attachment 457755 [details]
For EWS

[Darin Adler]

Comment on attachment 457755 [details]
For EWS

View in context: https://bugs.webkit.org/attachment.cgi?id=457755&action=review

> Source/WebCore/dom/mac/ImageControlsMac.cpp:93
> +    controlLayer->setContentEditable("false"_s);

Mildly sad that we allocate and then deallocate the String containing "false" here. There is, however, a clean way to fix that. We can change HTMLElement::setContentEditable to take a StringView. Will still work with the String&& that the JavaScript bindings will pass to it. Probably will have to edit DOMHTMLElement.mm to keep it compiling since an NSString * knows how to turn itself into a String but not a StringView. Not really necessary to do *any* of this as part of this patch, just something I am noticing in passing.

[Chris Dumez]

Comment on attachment 457755 [details]
For EWS

View in context: https://bugs.webkit.org/attachment.cgi?id=457755&action=review

>> Source/WebCore/dom/mac/ImageControlsMac.cpp:93
>> +    controlLayer->setContentEditable("false"_s);
> 
> Mildly sad that we allocate and then deallocate the String containing "false" here. There is, however, a clean way to fix that. We can change HTMLElement::setContentEditable to take a StringView. Will still work with the String&& that the JavaScript bindings will pass to it. Probably will have to edit DOMHTMLElement.mm to keep it compiling since an NSString * knows how to turn itself into a String but not a StringView. Not really necessary to do *any* of this as part of this patch, just something I am noticing in passing.

I totally agree that setContentEditable() should take a StringView. I also think that the most efficient code may be:
controlLayer->setAttributeWithoutSynchronization(contenteditableAttr, "false"_s);
or even
controlLayer->setAttributeWithoutSynchronization(contenteditableAttr, HTMLElement::falseName());
if we expose falseName() in the HTMLElement header (it is currently in the cpp only).

To avoid the string comparisons inside setContentEditable().

[Wenson Hsieh]

Thanks for the reviews, Chris and Darin!

(In reply to Chris Dumez from comment #3)
> Comment on attachment 457755 [details]
> For EWS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457755&action=review
> 
> >> Source/WebCore/dom/mac/ImageControlsMac.cpp:93
> >> +    controlLayer->setContentEditable("false"_s);
> > 
> > Mildly sad that we allocate and then deallocate the String containing "false" here. There is, however, a clean way to fix that. We can change HTMLElement::setContentEditable to take a StringView. Will still work with the String&& that the JavaScript bindings will pass to it. Probably will have to edit DOMHTMLElement.mm to keep it compiling since an NSString * knows how to turn itself into a String but not a StringView. Not really necessary to do *any* of this as part of this patch, just something I am noticing in passing.
> 
> I totally agree that setContentEditable() should take a StringView. I also
> think that the most efficient code may be:
> controlLayer->setAttributeWithoutSynchronization(contenteditableAttr,
> "false"_s);
> or even
> controlLayer->setAttributeWithoutSynchronization(contenteditableAttr,
> HTMLElement::falseName());
> if we expose falseName() in the HTMLElement header (it is currently in the
> cpp only).
> 
> To avoid the string comparisons inside setContentEditable().

That's a great point — I filed <https://webkit.org/b/239424> to track refactoring setContentEditable to take a StringView; I'll also tweak this patch so that it does:

```
controlLayer->setAttributeWithoutSynchronization(contenteditableAttr, "false"_s);
```

...instead of using `setContentEditable()`.

[Wenson Hsieh]

Created attachment 457758 [details]
For landing

[EWS]

Committed r292944 (249709@main): <https://commits.webkit.org/249709@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457758 [details].