From 4493e43e5db5f596dcae43136d6d7127c0b76ce5 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Thu, 15 Dec 2022 09:11:47 +0000 Subject: [PATCH] Bug 1789574 - part 1: Make `HTMLEditor` preserve order of inline style elements too r=m_kato Currently, `HTMLEditor` preserves styled elements with the reversed order in the loop. https://searchfox.org/mozilla-central/rev/d7d5c89ee7bc70dec0fd846689ca030f94931393/editor/libeditor/HTMLEditSubActionHandler.cpp#8638-8658 I.e., if `` is restored in the HTML styling mode, it will be restored as `...`. This blocks writing tests for bug 1789574. Therefore, this should be fixed first. (As I commented in the method, ideally we should store all inline elements and restore all of them, but currently we don't need to do it. It requires redesign of the style cache, but it seems that we don't know web apps which is broken by current behavior. Therefore, I think that we should do it when we meet a trouble with the behavior.) Depends on D164519 Differential Revision: https://phabricator.services.mozilla.com/D164520 --- editor/libeditor/HTMLEditSubActionHandler.cpp | 84 ++++++++++++++++++- editor/libeditor/PendingStyles.h | 9 +- .../meta/editing/run/inserttext.html.ini | 15 ---- .../meta/editing/run/multitest.html.ini | 39 --------- .../tests/editing/data/multitest.js | 13 +-- 5 files changed, 92 insertions(+), 68 deletions(-) diff --git a/editor/libeditor/HTMLEditSubActionHandler.cpp b/editor/libeditor/HTMLEditSubActionHandler.cpp index 2a409b6884aa..6aafcd8f5a3a 100644 --- a/editor/libeditor/HTMLEditSubActionHandler.cpp +++ b/editor/libeditor/HTMLEditSubActionHandler.cpp @@ -9028,7 +9028,87 @@ nsresult HTMLEditor::GetInlineStyles( MOZ_ASSERT(IsEditActionDataAvailable()); MOZ_ASSERT(aPendingStyleCacheArray.IsEmpty()); - bool useCSS = IsCSSEnabled(); + if (!IsCSSEnabled()) { + // In the HTML styling mode, we should preserve the order of inline styles + // specified with HTML elements, then, we can keep same order as original + // one when we create new elements to apply the styles at new place. + // XXX Currently, we don't preserve all inline parents, therefore, we cannot + // restore all inline elements as-is. Perhaps, we should store all + // inline elements with more details (e.g., all attributes), and store + // same elements. For example, web apps may give style as: + // em { + // font-style: italic; + // } + // em em { + // font-style: normal; + // font-weight: bold; + // } + // but we cannot restore the style as-is. + nsString value; + const bool givenElementIsEditable = + HTMLEditUtils::IsSimplyEditableNode(aElement); + auto NeedToAppend = [&](nsStaticAtom& aTagName, nsStaticAtom* aAttribute) { + if (mPendingStylesToApplyToNewContent->GetStyleState( + aTagName, aAttribute) != PendingStyleState::NotUpdated) { + return false; // The style has already been changed. + } + if (aPendingStyleCacheArray.Contains(aTagName, aAttribute)) { + return false; // Already preserved + } + return true; + }; + for (Element* const inclusiveAncestor : + aElement.InclusiveAncestorsOfType()) { + if (HTMLEditUtils::IsBlockElement(*inclusiveAncestor) || + (givenElementIsEditable && + !HTMLEditUtils::IsSimplyEditableNode(*inclusiveAncestor))) { + break; + } + if (inclusiveAncestor->IsAnyOfHTMLElements( + nsGkAtoms::b, nsGkAtoms::i, nsGkAtoms::u, nsGkAtoms::s, + nsGkAtoms::strike, nsGkAtoms::tt, nsGkAtoms::em, + nsGkAtoms::strong, nsGkAtoms::dfn, nsGkAtoms::code, + nsGkAtoms::samp, nsGkAtoms::var, nsGkAtoms::cite, nsGkAtoms::abbr, + nsGkAtoms::acronym, nsGkAtoms::sub, nsGkAtoms::sup)) { + nsStaticAtom& tagName = const_cast( + *inclusiveAncestor->NodeInfo()->NameAtom()->AsStatic()); + if (NeedToAppend(tagName, nullptr)) { + aPendingStyleCacheArray.AppendElement( + PendingStyleCache(tagName, nullptr, EmptyString())); + } + continue; + } + if (inclusiveAncestor->IsHTMLElement(nsGkAtoms::font)) { + if (NeedToAppend(*nsGkAtoms::font, nsGkAtoms::face)) { + inclusiveAncestor->GetAttr(kNameSpaceID_None, nsGkAtoms::face, value); + if (!value.IsEmpty()) { + aPendingStyleCacheArray.AppendElement( + PendingStyleCache(*nsGkAtoms::font, nsGkAtoms::face, value)); + value.Truncate(); + } + } + if (NeedToAppend(*nsGkAtoms::font, nsGkAtoms::size)) { + inclusiveAncestor->GetAttr(kNameSpaceID_None, nsGkAtoms::size, value); + if (!value.IsEmpty()) { + aPendingStyleCacheArray.AppendElement( + PendingStyleCache(*nsGkAtoms::font, nsGkAtoms::size, value)); + value.Truncate(); + } + } + if (NeedToAppend(*nsGkAtoms::font, nsGkAtoms::color)) { + inclusiveAncestor->GetAttr(kNameSpaceID_None, nsGkAtoms::color, + value); + if (!value.IsEmpty()) { + aPendingStyleCacheArray.AppendElement( + PendingStyleCache(*nsGkAtoms::font, nsGkAtoms::color, value)); + value.Truncate(); + } + } + continue; + } + } + return NS_OK; + } for (nsStaticAtom* property : {nsGkAtoms::b, nsGkAtoms::i, @@ -9067,7 +9147,7 @@ nsresult HTMLEditor::GetInlineStyles( nsString value; // Don't use nsAutoString here because it requires memcpy // at creating new PendingStyleCache instance. // Don't use CSS for , we don't support it usefully (bug 780035) - if (!useCSS || (property == nsGkAtoms::size)) { + if (property == nsGkAtoms::size) { isSet = HTMLEditUtils::IsInlineStyleSetByElement(aElement, style, nullptr, &value); } else if (style.IsCSSEditable(aElement)) { diff --git a/editor/libeditor/PendingStyles.h b/editor/libeditor/PendingStyles.h index 368802f2ee00..e8c5d01057f6 100644 --- a/editor/libeditor/PendingStyles.h +++ b/editor/libeditor/PendingStyles.h @@ -105,8 +105,8 @@ class PendingStyleCache final { class MOZ_STACK_CLASS AutoPendingStyleCacheArray final : public AutoTArray { public: - index_type IndexOf(const nsStaticAtom& aTag, - const nsStaticAtom* aAttribute) const { + [[nodiscard]] index_type IndexOf(const nsStaticAtom& aTag, + const nsStaticAtom* aAttribute) const { for (index_type index = 0; index < Length(); ++index) { const PendingStyleCache& styleCache = ElementAt(index); if (&styleCache.TagRef() == &aTag && @@ -116,6 +116,11 @@ class MOZ_STACK_CLASS AutoPendingStyleCacheArray final } return NoIndex; } + + [[nodiscard]] bool Contains(const nsStaticAtom& aTag, + const nsStaticAtom* aAttribute) const { + return IndexOf(aTag, aAttribute) != NoIndex; + } }; enum class PendingStyleState { diff --git a/testing/web-platform/meta/editing/run/inserttext.html.ini b/testing/web-platform/meta/editing/run/inserttext.html.ini index b654a68d1842..2b5a76cf7b2f 100644 --- a/testing/web-platform/meta/editing/run/inserttext.html.ini +++ b/testing/web-platform/meta/editing/run/inserttext.html.ini @@ -330,9 +330,6 @@ [[["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc{def}ghi
" compare innerHTML] expected: FAIL - [[["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc[def\]ghi
" compare innerHTML] - expected: FAIL - [[["inserttext","d"\],["inserttext","e"\],["inserttext","f"\],["inserttext","g"\]\] "
abc[defg\]hi
" compare innerHTML] expected: FAIL @@ -347,15 +344,3 @@ [[["inserttext","c"\],["inserttext","d"\],["inserttext","e"\]\] "
ab[cde\]fghi
" compare innerHTML] expected: FAIL - - [[["inserttext","d"\],["inserttext","e"\]\] "
abc[de\]fghi
" compare innerHTML] - expected: FAIL - - [[["inserttext","d"\],["inserttext","e"\]\] "
abc{de\]fghi
" compare innerHTML] - expected: FAIL - - [[["inserttext","d"\],["inserttext","e"\]\] "
abc[de\]fghi
" compare innerHTML] - expected: FAIL - - [[["inserttext","d"\],["inserttext","e"\]\] "
abc{de\]fghi
" compare innerHTML] - expected: FAIL diff --git a/testing/web-platform/meta/editing/run/multitest.html.ini b/testing/web-platform/meta/editing/run/multitest.html.ini index 2ef6f09352a6..535ef516c10b 100644 --- a/testing/web-platform/meta/editing/run/multitest.html.ini +++ b/testing/web-platform/meta/editing/run/multitest.html.ini @@ -1144,27 +1144,12 @@ [[["forwarddelete",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc{def}ghi
" compare innerHTML] expected: FAIL - [[["delete",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc[def\]ghi
" compare innerHTML] - expected: FAIL - - [[["forwarddelete",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc[def\]ghi
" compare innerHTML] - expected: FAIL - [[["delete",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc[def\]ghi
" compare innerHTML] expected: FAIL - [[["forwarddelete",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc[def\]ghi
" compare innerHTML] - expected: FAIL - - [[["delete",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc[def\]ghi
" compare innerHTML] - expected: FAIL - [[["forwarddelete",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc[def\]ghi
" compare innerHTML] expected: FAIL - [[["delete",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\],["inserttext","g"\]\] "
abc[defg\]hi
" compare innerHTML] - expected: FAIL - [[["forwarddelete",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\],["inserttext","g"\]\] "
abc[defg\]hi
" compare innerHTML] expected: FAIL @@ -1183,12 +1168,6 @@ [[["insertparagraph",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc{def}ghi
" compare innerHTML] expected: FAIL - [[["insertparagraph",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc[def\]ghi
" compare innerHTML] - expected: FAIL - - [[["insertparagraph",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc[def\]ghi
" compare innerHTML] - expected: FAIL - [[["insertparagraph",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc[def\]ghi
" compare innerHTML] expected: FAIL @@ -1210,9 +1189,6 @@ [[["forwarddelete",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc[def\]ghi
" compare innerHTML] expected: FAIL - [[["insertlinebreak",""\],["inserttext","d"\],["inserttext","e"\]\] "
abc[de\]fghi
" compare innerHTML] - expected: FAIL - [[["insertparagraph",""\],["inserttext","d"\],["inserttext","e"\],["inserttext","f"\]\] "
abc[def\]ghi
" compare innerHTML] expected: FAIL @@ -1225,9 +1201,6 @@ [[["forwarddelete",""\],["inserttext","c"\],["inserttext","d"\],["inserttext","e"\]\] "
ab[cde\]fghi
" compare innerHTML] expected: FAIL - [[["insertlinebreak",""\],["inserttext","c"\],["inserttext","d"\],["inserttext","e"\]\] "
ab[cde\]fghi
" compare innerHTML] - expected: FAIL - [[["delete",""\],["inserttext","d"\],["inserttext","e"\]\] "
abc[de\]fghi
" compare innerHTML] expected: FAIL @@ -1240,12 +1213,6 @@ [[["forwarddelete",""\],["inserttext","d"\],["inserttext","e"\]\] "
abc{de\]fghi
" compare innerHTML] expected: FAIL - [[["insertlinebreak",""\],["inserttext","d"\],["inserttext","e"\]\] "
abc[de\]fghi
" compare innerHTML] - expected: FAIL - - [[["insertlinebreak",""\],["inserttext","d"\],["inserttext","e"\]\] "
abc{de\]fghi
" compare innerHTML] - expected: FAIL - [[["delete",""\],["inserttext","d"\],["inserttext","e"\]\] "
abc[de\]fghi
" compare innerHTML] expected: FAIL @@ -1257,9 +1224,3 @@ [[["forwarddelete",""\],["inserttext","d"\],["inserttext","e"\]\] "
abc{de\]fghi
" compare innerHTML] expected: FAIL - - [[["insertlinebreak",""\],["inserttext","d"\],["inserttext","e"\]\] "
abc[de\]fghi
" compare innerHTML] - expected: FAIL - - [[["insertlinebreak",""\],["inserttext","d"\],["inserttext","e"\]\] "
abc{de\]fghi
" compare innerHTML] - expected: FAIL diff --git a/testing/web-platform/tests/editing/data/multitest.js b/testing/web-platform/tests/editing/data/multitest.js index b1259afcbf61..135f133ebc34 100644 --- a/testing/web-platform/tests/editing/data/multitest.js +++ b/testing/web-platform/tests/editing/data/multitest.js @@ -2481,27 +2481,20 @@ var browserTests = [ [["styleWithCSS", "false"],["bold",""],["inserttext","d"]], ["
abcd
", "
abcd
", - "
abcd
", - "
abcd
", - "
abcd
", - "
abcd
"], + "
abcd
"], [true,true,true], {"bold":[false,true,"",false,false,""]}], ["
abc[]
", [["styleWithCSS", "false"],["bold",""],["inserttext","d"]], ["
abcd
", "
abcd
", - "
abcd
", - "
abcd
", - "
abcd
", - "
abcd
"], + "
abcd
"], [true,true,true], {"bold":[false,true,"",false,false,""]}], // In this case, second line text should be keep bold style. ["
abc[]

", [["styleWithCSS", "false"],["bold",""],["inserttext","d"]], - ["
abcd

", - "
abcd

"], + "
abcd

", [true,true,true], {"bold":[false,true,"",false,false,""]}], // Tests putting caret to right paragraph at insert paragraph, and preserve