1244437 - formatBlock div isn't possible when executed in a <p> [contenteditable, execCommand]

Status: RESOLVED FIXED

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

Description

[cyril.auburtin]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.13 Safari/537.36

Steps to reproduce:

I've similar bugs, but this one is still occuring

demo: http://jsfiddle.net/crl/eo4p501n/27/

I initially have a div, I select inside it I format it to <p>, it nests the <p> in the <div> instead of unwrapping to <p> only

thus if calling formatBlock 'div' again, it's impossible to remove the <p> (that's an additional issue that would be fixed if formatBlock 'p' unwraps the <div>)


Actual results:

a <p> is created inside <div>


Expected results:

<div > should be replaced by <p>

Comment 1

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, untouched DOM/Storage bug's priority.

If you have reason to believe, this is wrong, please write a comment and ni :jstutte.

Comment 2

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

Attachment: Bug 1244437 - part 1: Make `HTMLEditUtils::IsFormatNode` check same tags as Chromium does r=m_kato!

I realized that our HTMLEditUtils::IsFormatNode is not maintained different
from the other browsers. Therefore, only we do not check new elements defined
after HTML 4.01. This patch aligns the list of the format elements to the
others [1].

Then, this also changes some expectations of editing/run/formatblock.html
to align common behavior of the browsers.

Note that we mapped formatBlock of execCommand to cmd_paragraphState,
and the XUL command handles <blockquote> in a different path [2] and the
behavior is pretty different from the other formatBlock command implementations.
Therefore, this patch creates new command for formatBlock and makes
HTMLEditor switch behavior in any places.

1. https://source.chromium.org/chromium/chromium/src/+/ba50f40fc4a203df704dc697d6e72469da8c5019:third_party/WebKit/WebCore/editing/FormatBlockCommand.cpp;l=114-134
2. https://searchfox.org/mozilla-central/rev/6602bdf9fff5020fbc8e248c963ddddf09a77b1b/editor/libeditor/HTMLEditor.cpp#2461-2474

Comment 3

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

Attachment: Bug 1244437 - part 2: Make `ParagraphStateAtSelection` handle `<dl>`, `<dt>` and `<dd>` elements r=m_kato!

Document.execCommand accepts dl, dt and dd as a format block. However,
ParagraphStateAtSelection ignores them as not a format block element.
Therefore, it causes odd result of queryCommandValue after applying the
format. E.g., execCommand("dd") to <p>[foobar]</p> correctly updates the
HTML to <dl><dd>foobar</dd></dl>, however, queryCommandValue("dd") returns
empty string rather than dd.

Note that Chromium returns inclusive ancestor format element of common ancestor
of the range. However, the behavior is odd because it makes the indeterm
state never true and the result is not used as the target of format block
command. So, this patch does not follow them at least for now.

On the other hand, this patch adds special case for dl. With user's
operation in valid HTML structure, selection range never selects <dl>
children directly. However, may cross <dt> and <dd> elements.
Then, let's return dl for compatibility with Chromium because <dl>
ancestor may be the format block target.

Depends on D190900

Comment 4

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

Attachment: Bug 1244437 - part 3: Make `formatBlock` command handlers use replace target as the nearest inclusive ancestor format node r=m_kato!

Currently, we extend selection to outside block boundaries if selection starts
and/or ends very start/end of block boundaries. Then, replace most-distant
ancestor format node, but this is odd because the innermost format is applied
to the text and the other browsers targets the innermost ancestor formant node.
Therefore, we should align to them.

Additionally, our replacing code for <dd> and <dt> has not been implemented
yet. I guess that the old HTMLEditUtils::IsFormatNode returned false for
<dd>, <dl> and <dt> and it requires additional work for splitting parent
<dl> too. Now, we can do it with simple code.

Finally, we created new format block element if <br> element appears, but
the other browsers moves it into the new format node simply. Therefore, we
should follow them (about <blockquote>, we've already done this within
the different path).

Depends on D190901

Comment 5

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

Attachment: Bug 1244437 - part 4: Make `HTMLEditor::IsFormatElement` return `false` for `<dl>`, `<dt>` and `<dd>` if it's called for XUL paragraph state command r=m_kato!

Now, all callers of them do not require to check <dl>, <dt> and <dd> check
if it's in the HTML format block mode. Therefore, we can move the check into
HTMLEditor::IsFormatElement to make the caller side simpler.

Additionally, this patch replaces some places with
HTMLEditUtils::IsFormatElementForParagraphState because the paths handle only
XUL paragraph state command with assertion check.

Depends on D190902

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3a22d72bc35
part 1: Make `HTMLEditUtils::IsFormatNode` check same tags as Chromium does r=m_kato
https://hg.mozilla.org/integration/autoland/rev/3fced6258607
part 2: Make `ParagraphStateAtSelection` handle `<dl>`, `<dt>` and `<dd>` elements r=m_kato
https://hg.mozilla.org/integration/autoland/rev/77bccdc1a9b6
part 3: Make `formatBlock` command handlers use replace target as the nearest inclusive ancestor format node r=m_kato
https://hg.mozilla.org/integration/autoland/rev/6951f8fc8821
part 4: Make `HTMLEditor::IsFormatElement` return `false` for `<dl>`, `<dt>` and `<dd>` if it's called for XUL paragraph state command r=m_kato

Comment 7

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

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

Comment 8

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/d3a22d72bc35
https://hg.mozilla.org/mozilla-central/rev/3fced6258607
https://hg.mozilla.org/mozilla-central/rev/77bccdc1a9b6
https://hg.mozilla.org/mozilla-central/rev/6951f8fc8821

Comment 9

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

Upstream PR merged by moz-wptsync-bot