1937696 - `HTMLEditor::InsertPaddingBRElementIfNeeded` should insert a padding `<br>` instead of a normal `<br>`

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

I was thinking that HTMLEditor should stop using the special <br> element with the flags because it's hard to maintain the flag especially when something are inserted after the padding <br>. However, it's also referred by ContentEventHandler to ignore the padding <br> element. Therefore, now, new behavior may make IME confused with <br> element appearance during composition (see bug 1932000).

For now, let's make the new method puts a padding <br> rather than a normal <br> element.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1925635

Comment 2

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

Accoding to this line's history, this is not a recent regression.

Comment 3

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

Attachment: Bug 1937696 - part 1: Make `HTMLEditor::InsertPaddingBRElementIfNeeded` insert a padding `<br>` rather than a normal `<br>` r=m_kato!

Using normal <br> for padding <br> makes notifying IME of the mutation
because ContentEventHandler ignores only padding <br> elements. Therefore,
we should make it use a padding <br> for hiding the mutations from IME since
IME may be confused at unexpected text change notifications.

Depends on D231666

Comment 4

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

Eitan, I have a question about the a11y module behavior.

With the above patch, I'd like to change padding <br> elements which make empty block or empty last line in the block visible to be marked as so. I.e., previously, we used normal <br>s in some cases even whey they are actually padding <br>s, including the case to put into an empty block. However, normal <br> element insertion and deletion will cause sending text change notification to IME. IME may not expect the change and may be confused. Therefore, I'd like to fix this bug.

However, doing this causes a new failure of accessible/tests/browser/text/browser_editabletext.js, it's here. It expects "before after", but it becomes "beforeafter", i.e., the space won't be inserted. I think that the space was caused by a normal <br> in the following DOM tree:

<style>
div[contenteditable]::before {
  content: "before"
}
div[contenteditable]::after {
  content: "after"
}
</style>
<div contenteditable><br></div>

So, the space works as a word separator between the before and after pseudo text. However, once the <br> is marked as a padding <br>, it disappears, it may be caused by these lines. Visually, "before" and "after" appear in different line because of the <br>. So, they look separated words. However, from a11y text, it looks like not 2 words.

On the other hand, the test utility function replaces the expectation with an ASCII white-space with no text. So, it also looks like that current test allows known issue. If so, I'll change the test. However, combining 2 words may be a serious issue in the case. If you think I should change the a11y module, I'd like to know where should I touch.

Could you let me know you ideas? Thanks.

Comment 5

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

Ah, if I need to keep the behavior of the a11y module, I should change the behavior of BRFrame::AccessibleType() to return a11y::eHTMLBRType if it's the only content of the parent block.

Comment 6

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

Attachment: Bug 1937696 - part 2: Make `BRFrame::AccessibleType()` return `a11y::eNoType` only when the padding `<br>` is not the only content of the parent block r=eeejay!

Currently, it returns a11y::eNoType when the content is marked as a padding
line break which is for either empty editor in the document level or empty
last line immediately before a block boundary. However, by the previous patch,
<br> element will have the "padding <br> for the last empty line" strictly.

The padding <br> elements are basically meaningless. However, it may be
meaningful only in some edge cases.

In the former case, the <br> is always put in empty <body> and the <body>
may have both before and after pseudo elements. Therefore, it should always be
treated as a normal <br>. Otherwise, the generated content will be treated
as one word.

In the latter case, there are 3 cases:

1. following another line break to make the empty line visible
2. following a block boundary to make the empty line between block boundaries
   visible
3. following a visible content due to a bug of our editor

At least, the method should not treat the <br> as meaning in the #2 case
because it may split the before and after pseudo elements' content of the parent
block.

Therefore, this patch takes care of only the 2nd case as a normal <br>.

Depends on D232601

Comment 7

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

Okay, I posted a patch for the a11y module.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9aa5d49f9fe9
part 1: Make `HTMLEditor::InsertPaddingBRElementIfNeeded` insert a padding `<br>` rather than a normal `<br>` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/85005cf3844b
part 2: Make `BRFrame::AccessibleType()` return `a11y::eNoType` only when the padding `<br>` is not the only content of the parent block r=eeejay

Comment 9

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/9aa5d49f9fe9
https://hg.mozilla.org/mozilla-central/rev/85005cf3844b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

What's the user-facing impact of this bug?

Comment 11

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)

What's the user-facing impact of this bug?

IME may receive <br> element updates as \n changes around composition string. This could cause IME being confused since IME might assume that the editor won't be modified by somebody else. However, the most sensitive platform about this notification, Windows, has its own cache in TSFTextStore, so, the change should be hidden on Windows during composition. On Linux and macOS, this shouldn't cause a bug because the notifications won't be sent to their IME. I'm not sure about Android. However, if it causes unexpected commit of composition string, I think there should've already been reported regressions.

Therefore, I think that this should ride the train.