1803044 - Reuse existing inline elements to set CSS style even if they have some attributes

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Gecko does not use existing <span> which has attribute, but the other browsers use it.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1803044 - part 1: Create a method of `EditorInlineStyle` to check whether an element represents the style r=m_kato!

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1803044 - part 2: Improve `HTMLEditUtils::ElementHasAttributeExcept` r=m_kato!

In the following patches, the new variations are required. Additionally,
HTMLEditor should ignore empty class, id and style attributes because
an older API does it.
https://searchfox.org/mozilla-central/rev/2ad13433da20a0749e1e9a10ec0ab49b987c2c8e/editor/libeditor/HTMLStyleEditor.cpp#3398-3402

Depends on D163996

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1803044 - part 3: Make `EditorBase::RemoveAttributeWithTransaction` stop creating transaction if the element does not have the removing attribute r=m_kato!

Depends on D163997

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1803044 - part 4: Make `HTMLEditor::SetAttributeOrEquivalent` insert a white-space only when old value is empty r=m_kato!

This causes some new failures after applying the following patches.

Depends on D163998

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1803044 - part 5: Make `HTMLEditor::RemoveStyleInside` check whether aElement is editable first r=m_kato!

Depends on D163999

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1803044 - part 6: Make `HTMLEditor::RemoveStyleInside` remove CSS style before touching the DOM tree structure r=m_kato!

This change makes the following changes smaller. Once we remove CSS style
first, then, it becomes easier to consider whether we should delete the element
since style attribute may be deleted by ChangeStyleTransaction if the
removing style is the last rule in it.

Depends on D164000

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1803044 - part 7: Make `HTMLEditor::RemoveStyleInside` use `HTMLEditor::ReplaceContainerWithTransaction` r=m_kato!

Depends on D164001

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1803044 - part 8: Refactor the bottom half of `HTMLEditor::RemoveStyleInside` r=m_kato!

Now, we can make there simpler because the part does one of them:

• Replaces element with new <span>
• Removes element
• Removes attribute
• Does nothing

However, there are duplicated code. We should consider what there does with
lambdas.

Depends on D164002

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1803044 - part 9: Make the style editor use existing inline element in the CSS mode as far as possible r=m_kato!

The other browsers use any inline elements to set CSS property for applying
an inline style. However, Gecko limits it to a <span> which does not have
any attributes. The other browsers' design is better for saving number of
elements and runtime cost of inserting new element (moving all children to the
new element and inserting it to the original position). Therefore, it's nicer
to follow the other browses. Then, we can avoid new WPT failures at aligning
other behaviors to the other browsers.

With doing that, removing style code requires complicated changes because
RemoveStyleInside assumes that one element has one style, but after taking
the compatible behavior, one element can have multiple styles including the
style represented by the element itself.

Note that the new expected failures are caused by bug 1183844. Gecko returns
closest ancestor element's background color for
queryCommandValue("backColor"). Therefore, it returns "transparent" of the
inner <span> element.

Depends on D164003

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95ca7cad0c4a
part 1: Create a method of `EditorInlineStyle` to check whether an element represents the style r=m_kato

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c444b60c784
part 2: Improve `HTMLEditUtils::ElementHasAttributeExcept` r=m_kato

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a067aad1a901
part 3: Make `EditorBase::RemoveAttributeWithTransaction` stop creating transaction if the element does not have the removing attribute r=m_kato

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a3b614bf90b
part 4: Make `HTMLEditor::SetAttributeOrEquivalent` insert a white-space only when old value is empty r=m_kato

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ad244ef4253
part 5: Make `HTMLEditor::RemoveStyleInside` check whether aElement is editable first r=m_kato

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/37466 for changes under testing/web-platform/tests

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71f29a225197
part 6: Make `HTMLEditor::RemoveStyleInside` remove CSS style before touching the DOM tree structure r=m_kato

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ceefc7aac79b
part 7: Make `HTMLEditor::RemoveStyleInside` use `HTMLEditor::ReplaceContainerWithTransaction` r=m_kato

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/717893e29a82
part 8: Refactor the bottom half of `HTMLEditor::RemoveStyleInside` r=m_kato

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14b38d16c5ed
part 9: Make the style editor use existing inline element in the CSS mode as far as possible r=m_kato

Comment 20

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/95ca7cad0c4a
https://hg.mozilla.org/mozilla-central/rev/3c444b60c784
https://hg.mozilla.org/mozilla-central/rev/a067aad1a901
https://hg.mozilla.org/mozilla-central/rev/5a3b614bf90b
https://hg.mozilla.org/mozilla-central/rev/4ad244ef4253
https://hg.mozilla.org/mozilla-central/rev/71f29a225197
https://hg.mozilla.org/mozilla-central/rev/ceefc7aac79b
https://hg.mozilla.org/mozilla-central/rev/717893e29a82
https://hg.mozilla.org/mozilla-central/rev/14b38d16c5ed

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot