Bug 208177

Created attachment 391626 [details] Patch

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?

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.

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

I'm ok with landing as-is.

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?

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.

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

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

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

Yeah, go ahead and land.

Comment on attachment 391626 [details] Patch Clearing flags on attachment: 391626 Committed r258077: <https://trac.webkit.org/changeset/258077>

All reviewed patches have been landed. Closing bug.

<rdar://problem/60192151>