1404675 - innerText should ignore "invisible" break when serializing HTML fragment

Status: UNCONFIRMED

(Core :: DOM: Core & HTML, defect, P3)

Description

[voracity]

Attachment: innertext_test.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20170930220116

Steps to reproduce:

Set innerText of element to itself.


Actual results:

The newlines in the text changed (incorrectly relative to what was displayed previously).


Expected results:

The element shouldn't have changed, because the HTML was identical to what was there previously.

The attached test case shows what happens. Clicking 'Rewrite' just sets el.innerText = el.innerText, and the display/content of the div will change when it shouldn't.

Note that this works correctly in Chrome and in the current release version Firefox, but not on Nightly.

Comment 1

[voracity]

Note that I copied (via the JS console) what Firefox sets as the div content when you manually type it in, just so the test case starts off at the right point. You could of course type in what's there and then hit 'Rewrite' to get the same problem to happen.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Reporter, thank you for reporting the issue. Would you minding helping us get where this regressed from by the mozregression tool [1]?  Thank you.

[1] https://blog.nightly.mozilla.org/category/mozregression/

Comment 3

[Andrew Overholt [:overholt]]

Masayuki did a bunch of work improving innerText performance over in the tree of bugs rooted at bug 1346723. Maybe he has an idea of what changed here?

Comment 4

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

I see this bug even with ESR52. So, this is not a recent regression. (Could be really old bug since setting innerHTML causes converting <div> to <br>. I have no idea when such big change occurs if older build didn't this conversion.)

Comment 5

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

Oops, not innertHTML, the testcase uses innerText.

Comment 6

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

Testcase to check innerText result of specific HTML fragment.
https://jsfiddle.net/d_toybox/fjym2zt6/

Comment 7

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

According to quick testing on the testcase, end tab of <div> causes "\n" at getting innerText but <div><br></div> causes "\n\n" and <div>abc<br></div> also causes "abc\n\n". Like editor, innerText should ignore "invisible" break when serializing HTML fragment.

Comment 8

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

Looks like that there is no special rules for invisible <br> elements:
https://html.spec.whatwg.org/multipage/dom.html#the-innertext-idl-attribute:the-br-element-3

Perhaps, this should be filed as spec bug.

And we should ignore invisible <br> here:
https://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/dom/base/nsRange.cpp#3877-3878

However, unfortunately, our editor's visibility check of <br> is really complicated:
https://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/editor/libeditor/HTMLEditor.cpp#860-904

Comment 9

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

Looks like that Chromium does hack something here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp?rcl=a3e6b4ed329668a56f6d97c126fc637054a39d2a&l=789

Comment 10

[Andrew Overholt [:overholt]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #4)
> I see this bug even with ESR52. So, this is not a recent regression. (Could
> be really old bug since setting innerHTML causes converting <div> to <br>. I
> have no idea when such big change occurs if older build didn't this
> conversion.)

OK, I'll remove the regression keyword and I think it's ok if we don't get a regression window here. I've also tried to clarify the summary a bit.