208177 2020-02-24 22:42:51 -0800 Make Editor::applyEditingStyleToBodyElement do things in a straightforward manner 2020-03-07 13:47:12 -0800 1 1 1 Unclassified WebKit HTML Editing WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 darin darin commit-queue ews-watchlist koivisto mifenton rniwa webkit-bug-importer wenson_hsieh oldest_to_newest 1622503 0 darin 2020-02-24 22:42:51 -0800 Make Editor::applyEditingStyleToBodyElement do things in a straightforward manner 1622504 1 391626 darin 2020-02-24 22:44:18 -0800 Created attachment 391626 Patch 1622579 2 391626 koivisto 2020-02-25 06:09:25 -0800 Comment on attachment 391626 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? 1622696 3 391626 darin 2020-02-25 12:02:14 -0800 Comment on attachment 391626 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. 1622699 4 koivisto 2020-02-25 12:07:19 -0800 > 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? 1622700 5 koivisto 2020-02-25 12:08:42 -0800 I'm ok with landing as-is. 1622718 6 391626 rniwa 2020-02-25 13:08:23 -0800 Comment on attachment 391626 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? 1623349 7 391626 darin 2020-02-26 20:03:44 -0800 Comment on attachment 391626 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. 1623363 8 rniwa 2020-02-26 21:01:09 -0800 (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. 1626507 9 darin 2020-03-05 21:46:07 -0800 Having a hard time deciding whether to land this patch or not. 1626528 10 rniwa 2020-03-05 22:42:25 -0800 (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. 1626530 11 koivisto 2020-03-05 22:43:40 -0800 Yeah, go ahead and land. 1627295 12 391626 commit-queue 2020-03-07 13:46:29 -0800 Comment on attachment 391626 Patch Clearing flags on attachment: 391626 Committed r258077: <https://trac.webkit.org/changeset/258077> 1627296 13 commit-queue 2020-03-07 13:46:30 -0800 All reviewed patches have been landed. Closing bug. 1627298 14 webkit-bug-importer 2020-03-07 13:47:12 -0800 <rdar://problem/60192151> 391626 2020-02-24 22:44:18 -0800 2020-03-07 13:46:29 -0800 Patch bug-208177-20200224224416.patch text/plain 2513 darin U3VidmVyc2lvbiBSZXZpc2lvbjogMjU3MjkzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDRkNmU2YzkwZDgxNDA5 MmM0YjY1NDQ2MDU5ZjlhZGVmYjg0YTNkNS4uNzRjNTM4NDBmMjJjMzZhZThmYjljMGYxNTFiODhj OGQwMDU4OTEwZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDIwLTAyLTI0ICBEYXJp biBBZGxlciAgPGRhcmluQGFwcGxlLmNvbT4KKworICAgICAgICBNYWtlIEVkaXRvcjo6YXBwbHlF ZGl0aW5nU3R5bGVUb0JvZHlFbGVtZW50IGRvIHRoaW5ncyBpbiBhIHN0cmFpZ2h0Zm9yd2FyZCBt YW5uZXIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIw ODE3NworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICog ZWRpdGluZy9FZGl0b3IuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RWRpdG9yOjphcHBseUVkaXRp bmdTdHlsZVRvQm9keUVsZW1lbnQgY29uc3QpOiBVc2UgRG9jdW1lbnQ6OmJvZHkgYW5kCisgICAg ICAgIFN0eWxlZEVsZW1lbnQ6OnNldElubGluZVN0eWxlUHJvcGVydHkgdG8gYXBwbHkgc3R5bGVz IHRvIHRoZSBib2R5LiBUaGUgb2xkZXIgY29kZQorICAgICAgICB3YXMgbG9vcGluZyBvdmVyIGFs bCBib2R5IGVsZW1lbnRzIGluIHRoZSBkb2N1bWVudCwgZm9yIG5vIGdvb2QgcmVhc29uLCBhbmQg dXNpbmcKKyAgICAgICAgdGhlIENTUyBvYmplY3QgbW9kZWwgd3JhcHBlciBvYmplY3QgZm9yIHRo ZSBzdHlsZXMsIGFsc28gZm9yIG5vIGdvb2QgcmVhc29uLgorCiAyMDIwLTAyLTI0ICBDaGFuZ1Nl b2sgT2ggIDxjaGFuZ3Nlb2tAd2Via2l0Lm9yZz4KIAogICAgICAgICBQUy0yMDE5LTAwNjogW0dU S10gV2ViS2l0IC0gQVhPYmplY3RDYWNoZSAtIG1fZGVmZXJyZWRGb2N1c2VkTm9kZUNoYW5nZSAt IFVhRgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvZWRpdGluZy9FZGl0b3IuY3BwIGIvU291 cmNlL1dlYkNvcmUvZWRpdGluZy9FZGl0b3IuY3BwCmluZGV4IDZjNmQ4M2E3MjhmMzAyMDNiOGVm ZmNlMGYwOGJhYTQ1NWMzMjZhNmEuLjAxNDZkNTNlZjQ0Y2IwNDU4MjIyMWVjNWQxZGNmMTFhZjll N2VlZjggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2VkaXRpbmcvRWRpdG9yLmNwcAorKysg Yi9Tb3VyY2UvV2ViQ29yZS9lZGl0aW5nL0VkaXRvci5jcHAKQEAgLTM0MzIsMTggKzM0MzIsMTIg QEAgdm9pZCBFZGl0b3I6OnRleHREaWRDaGFuZ2VJblRleHRBcmVhKEVsZW1lbnQqIGUpCiAKIHZv aWQgRWRpdG9yOjphcHBseUVkaXRpbmdTdHlsZVRvQm9keUVsZW1lbnQoKSBjb25zdAogewotICAg IC8vIEZJWE1FOiBOb3QgY2xlYXIgaXQncyB2YWx1YWJsZSB0byBkbyB0aGlzIHRvIGFsbCBib2R5 IGVsZW1lbnRzIHJhdGhlciB0aGFuIGp1c3QgZG9pbmcgaXQgb24gdGhlIHNpbmdsZSBvbmUgcmV0 dXJuZWQgYnkgRG9jdW1lbnQ6OmJvZHkuCi0gICAgVmVjdG9yPFJlZjxIVE1MQm9keUVsZW1lbnQ+ PiBib2RpZXM7Ci0gICAgZm9yIChhdXRvJiBib2R5IDogZGVzY2VuZGFudHNPZlR5cGU8SFRNTEJv ZHlFbGVtZW50Pihkb2N1bWVudCgpKSkKLSAgICAgICAgYm9kaWVzLmFwcGVuZChib2R5KTsKLQot ICAgIGZvciAoYXV0byYgYm9keSA6IGJvZGllcykgewotICAgICAgICAvLyBNdXRhdGUgdXNpbmcg dGhlIENTU09NIHdyYXBwZXIgc28gd2UgZ2V0IHRoZSBzYW1lIGV2ZW50IGJlaGF2aW9yIGFzIGEg c2NyaXB0LgotICAgICAgICBhdXRvJiBzdHlsZSA9IGJvZHktPmNzc29tU3R5bGUoKTsKLSAgICAg ICAgc3R5bGUuc2V0UHJvcGVydHlJbnRlcm5hbChDU1NQcm9wZXJ0eVdvcmRXcmFwLCAiYnJlYWst d29yZCIsIGZhbHNlKTsKLSAgICAgICAgc3R5bGUuc2V0UHJvcGVydHlJbnRlcm5hbChDU1NQcm9w ZXJ0eVdlYmtpdE5ic3BNb2RlLCAic3BhY2UiLCBmYWxzZSk7Ci0gICAgICAgIHN0eWxlLnNldFBy b3BlcnR5SW50ZXJuYWwoQ1NTUHJvcGVydHlMaW5lQnJlYWssICJhZnRlci13aGl0ZS1zcGFjZSIs IGZhbHNlKTsKLSAgICB9CisgICAgYXV0byBib2R5ID0gbWFrZVJlZlB0cihkb2N1bWVudCgpLmJv ZHkoKSk7CisgICAgaWYgKCFib2R5KQorICAgICAgICByZXR1cm47CisgICAgYm9keS0+c2V0SW5s aW5lU3R5bGVQcm9wZXJ0eShDU1NQcm9wZXJ0eVdvcmRXcmFwLCBDU1NWYWx1ZUJyZWFrV29yZCk7 CisgICAgYm9keS0+c2V0SW5saW5lU3R5bGVQcm9wZXJ0eShDU1NQcm9wZXJ0eVdlYmtpdE5ic3BN b2RlLCBDU1NWYWx1ZVNwYWNlKTsKKyAgICBib2R5LT5zZXRJbmxpbmVTdHlsZVByb3BlcnR5KENT U1Byb3BlcnR5TGluZUJyZWFrLCBDU1NWYWx1ZUFmdGVyV2hpdGVTcGFjZSk7CiB9CiAKIGJvb2wg RWRpdG9yOjpmaW5kU3RyaW5nKGNvbnN0IFN0cmluZyYgdGFyZ2V0LCBGaW5kT3B0aW9ucyBvcHRp b25zKQo=