1359345 - HTMLEditor::IsVisBreak() should be renamed to HTMLEditor::IsVisibleBRElement()

Status: RESOLVED FIXED

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

Description

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

HTMLEditor::IsVisBreak() isn't unclear to me. I think that IsVisibleBRElement() is clearer. So, I think that it should be renamed.

Additionally, it should be explained by comment like:

/**
 * Returns true if aNode is a <br> element and visible.  Otherwise, false.
 * Be aware, if aNode is a <br> element but it doesn't cause line break
 * actually, this method treats it as invisible.  E.g., a single line break
 * before a block boundary, i.e., <br> in "foo<p>bar<br></p>" and
 * "foo<br><p>bar</p>", is treated as invisible.
 */

Comment 1

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

Oh, and HTMLEditor::IsVisTextNode() should be renamed to IsVisibleTextNode() for the consistency.

/**
 * Returns true if aNode is a text node and visible. Otherwise, false.
 * When aSafeToAskFrames is true, this method checks if the text node is
 * actually mapped to a text frame.  Otherwise, this method checks if it
 * has non-whitespace characters.
 */

(Although, I don't understand well the detail of nsWSRunObject, so, the explanation when aSafeToAskFrames should have more detail if you know.)

Comment 2

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

Oh, the result of IsVisibleTextNode() is "isEmpty". So, the result is reverted. It's really error prone. Should be revert the meaning of result as the comment in this bug.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Bug 1359345 - HTMLEditor::IsVisBreak() should be renamed to HTMLEditor::IsVisibleBRElement().

Review commit: https://reviewboard.mozilla.org/r/159694/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159694/

Comment 4

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

Comment on attachment 8888677 [details]
Bug 1359345 - HTMLEditor::IsVisBreak() should be renamed to HTMLEditor::IsVisibleBRElement().

https://reviewboard.mozilla.org/r/159694/#review165066

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18f6b952efe9
HTMLEditor::IsVisBreak() should be renamed to HTMLEditor::IsVisibleBRElement(). r=masayuki

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/18f6b952efe9