Bug 208177 – Make Editor::applyEditingStyleToBodyElement do things in a straightforward manner

• Component: HTML Editing
• Status: RESOLVED FIXED

[Darin Adler]

Make Editor::applyEditingStyleToBodyElement do things in a straightforward manner

[Darin Adler]

Created attachment 391626 [details]
Patch

[Antti Koivisto]

Comment on attachment 391626 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:-3442
> -        // Mutate using the CSSOM wrapper so we get the same event behavior as a script.
> -        auto& style = body->cssomStyle();

This originates from https://bugs.webkit.org/show_bug.cgi?id=78589 and was based on Ryosuke's comments ("In my second thought, not instantiating mutation scope is fine because all calls to setInlineStyleProperty are invisible to scripts except ones in Editor.cpp, RemoveCSSPropertyCommand.cpp, & ReplaceSelectionCommand.cpp. Please revert those before landing the patch.")

Has something changed so we don't need this anymore here?

[Darin Adler]

Comment on attachment 391626 [details]
Patch

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

>> Source/WebCore/editing/Editor.cpp:-3442
>> -        auto& style = body->cssomStyle();
> 
> This originates from https://bugs.webkit.org/show_bug.cgi?id=78589 and was based on Ryosuke's comments ("In my second thought, not instantiating mutation scope is fine because all calls to setInlineStyleProperty are invisible to scripts except ones in Editor.cpp, RemoveCSSPropertyCommand.cpp, & ReplaceSelectionCommand.cpp. Please revert those before landing the patch.")
> 
> Has something changed so we don't need this anymore here?

The call sites in editing commands (RemoveCSSPropertyCommand, ReplaceSelectionCommand.cpp) are a web-visible part of how editing behaves on webpages. So Ryoskue’s logic makes sense there, although using the CSS object model wrappers seems like a roundabout way to implement it.

This function is used for something quite different, a mode switch on the entire web view. I don’t think the mutation event issues are the same, and the most important cases I am aware of, like in the iOS and macOS Mail apps, also disable content scripting.

However, there could be something I missed: maybe someone can think of a reason I would need to "instantiate mutation scope" here. I didn’t do that.

I think I need advice on how to write a test that can demonstrate why this matters. I’d be willing to do that.

[Antti Koivisto]

> This function is used for something quite different, a mode switch on the
> entire web view. I don’t think the mutation event issues are the same, and
> the most important cases I am aware of, like in the iOS and macOS Mail apps,
> also disable content scripting.

Ok, maybe I just took the comment too broadly originally and this was never needed in the first place.

> However, there could be something I missed: maybe someone can think of a
> reason I would need to "instantiate mutation scope" here. I didn’t do that.
> 
> I think I need advice on how to write a test that can demonstrate why this
> matters. I’d be willing to do that.

Ryosuke might have ideas?

[Antti Koivisto]

I'm ok with landing as-is.

[Ryosuke Niwa]

Comment on attachment 391626 [details]
Patch

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

I'm not certain that we want to land this patch.

> Source/WebCore/editing/Editor.cpp:-3435
> -    // FIXME: Not clear it's valuable to do this to all body elements rather than just doing it on the single one returned by Document::body.

Isn't this because we can have multiple body elements in one of those badly written CMS pages?

[Darin Adler]

Comment on attachment 391626 [details]
Patch

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

>> Source/WebCore/editing/Editor.cpp:-3435
>> -    // FIXME: Not clear it's valuable to do this to all body elements rather than just doing it on the single one returned by Document::body.
> 
> Isn't this because we can have multiple body elements in one of those badly written CMS pages?

Maybe; but when would that intersect with an editable WebView? This isn’t used by web browsers.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #7)
> Comment on attachment 391626 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391626&action=review
> 
> >> Source/WebCore/editing/Editor.cpp:-3435
> >> -    // FIXME: Not clear it's valuable to do this to all body elements rather than just doing it on the single one returned by Document::body.
> > 
> > Isn't this because we can have multiple body elements in one of those badly written CMS pages?
> 
> Maybe; but when would that intersect with an editable WebView? This isn’t
> used by web browsers.

Oh, I see. It's probably fine then. I knew we used to have a lot of code dealing with duplicated body elements because ReplaceSelectionCommand / createMarkup used to generate them sometimes but I think we've mostly resolved that problem by now.

[Darin Adler]

Having a hard time deciding whether to land this patch or not.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #9)
> Having a hard time deciding whether to land this patch or not.

It seems okay to land this change given it doesn't affect editing commands / general web visible behavior.

[Antti Koivisto]

Yeah, go ahead and land.

[WebKit Commit Bot]

Comment on attachment 391626 [details]
Patch

Clearing flags on attachment: 391626

Committed r258077: <https://trac.webkit.org/changeset/258077>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60192151>