Wrong innerText result for multiple continuous Text nodes |
||||
Issue descriptionChrome Version: 69 stable and 71 canary OS: All but iOS What steps will reproduce the problem? (1) Open http://w3c-test.org/html/dom/elements/the-innertext-idl-attribute/multiple-text-nodes.window.html (2) Observe What is the expected result? The test passes. What happens instead? The test fails. Please use labels and text to provide additional information. Test source: http://w3c-test.org/html/dom/elements/the-innertext-idl-attribute/multiple-text-nodes.window.js A <div> has four Text children: "X", "", " ", "Y" Edge, Firefox, Safari work correctly. Found by https://gist.github.com/foolip/32285c7baf22f7ca356c80b56b56a484
,
Oct 12
It seems layout tree doesn't have Text node changed to have characters.
LayoutBlockFlow {HTML} at (0,0) size 800x600
LayoutBlockFlow {BODY} at (8,8) size 784x584
LayoutBlockFlow {DIV} at (0,0) size 784x20
LayoutText {#text} at (0,0) size 11x19
text run at (0,0) width 11: "X"
LayoutText {#text} at (11,0) size 11x19
text run at (11,0) width 11: "Y"
This dump is created by:
<body></body>
<script>
if (window.testRunner) testRunner.waitUntilDone();
const div = document.body.appendChild(document.createElement("div"));
const t1 = div.appendChild(new Text(""));
div.appendChild(new Text(""));
const t2 = div.appendChild(new Text(""));
const t3 = div.appendChild(new Text(""));
setTimeout(() => {
t1.data = "X";
t2.data = " ";
t3.data = "Y";
console.log(div.innerText)
if (window.testRunner) testRunner.notifyDone();
}, 100);
</script>
,
Oct 15
,
Oct 17
WhitespaceAttacher issue?
,
Oct 17
Sounds plausible.
,
Oct 18
The root cause is Text::TextLayoutObjectIsNeeded() returns false for Text node
which is one space, at
if (!context.use_previous_in_flow)
return false;
where |context| is |AttachContext|.
Here is comment of |use_previous_in_flow|:
// True if the previous_in_flow member is up-to-date, even if it is nullptr.
bool use_previous_in_flow = false;
I think we should have layout object for space only Text node if we are don't know about previous flow.
,
Oct 18
Correction about #c6. It is OK to return false for Text::TextLayoutObjectIsNeeded() at first call. We call Text::TextLayoutObjectIsNeeded() from WhitespaceAttacher::ReattachWhitespaceSiblings() via WhitespaceAttacher::DidReattachText() in Text::RebuildTextLayoutTree().
,
Oct 18
The root cause is following line in WhitespaceAttacher::ReattachWhitespaceSiblings()
void WhitespaceAttacher::ReattachWhitespaceSiblings(...) {
...
for (Node* sibling = ...) {
...
// If sibling's layout object status didn't change we don't need to
// continue checking other siblings since their layout object status
// won't change either.
if (!!sibling_layout_object == had_layout_object)
break;
...
}
}
For empty Text node, we don't need to have layout object and Text::ReattachLayoutTreeIfNeeded() always doesn't attach layout object regardless previous layout object in flow.
,
Oct 18
I'm not sure why StringImpl::ContainsOnlyWhitespace() returns true for empty string. I think it should return false for empty string.
,
Oct 18
Some people think it is OK to return true for ContainsOnlyWhitespace() with empty string. Also, from beginning[1], it is so. I found some codes do: str.IsEmpty() || str.ContainsOnlyWhitespace() So, I rename it to ContainsOnlyWhitespaceOrEmpty() to reduce confusion. [1] https://chromium.googlesource.com/chromium/src/+/4bce49199234b5f0eace49418a8927b27e0be83e
,
Oct 18
In review: http://crrev.com/c/1288176
,
Oct 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/473684f7aa84bbc48c49066cbf7e0076006bcba5 commit 473684f7aa84bbc48c49066cbf7e0076006bcba5 Author: Yoshifumi Inoue <yosin@chromium.org> Date: Thu Oct 18 08:06:28 2018 Rename ContainsOnlyWhitespace() to ContainsOnlyWhitespaceOrEmpty() This patch renames ContainsOnlyWhitespace() to ContainsOnlyWhitespaceOrEmpty() to avoid ambiguity of return value of empty string for improving readability. This patch is done by global replace and manual changes fro removing |IsEmpty()| from |str.IsEmpty() || str.ContainsOnlyWhitespaceOrEmpty()| in * third_party/blink/renderer/core/css/properties/css_parsing_utils.cc * third_party/blink/renderer/core/editing/selection_controller.cc This patch is a preparation of the patch[1]. [1] http://crrev.com/c/1288176 Make WhitespaceAttacher to ignore empty Text node Bug: 894674 Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng Change-Id: I144ef97f2b016a1bb39e6c3ef5fc5d850865d009 Reviewed-on: https://chromium-review.googlesource.com/c/1288157 Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Yoshifumi Inoue <yosin@chromium.org> Cr-Commit-Position: refs/heads/master@{#600686} [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/core/css/properties/css_parsing_utils.cc [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/core/dom/character_data.cc [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/core/dom/character_data.h [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/core/dom/slot_assignment_test.cc [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/core/dom/text.cc [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/core/dom/whitespace_attacher.cc [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/core/editing/commands/insert_text_command.cc [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/core/editing/selection_controller.cc [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/core/html/html_object_element.cc [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/core/layout/layout_text.cc [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/core/layout/line/abstract_inline_text_box.cc [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/modules/accessibility/ax_inline_text_box.cc [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/modules/accessibility/ax_layout_object.cc [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/platform/wtf/text/string_impl.cc [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/platform/wtf/text/string_impl.h [modify] https://crrev.com/473684f7aa84bbc48c49066cbf7e0076006bcba5/third_party/blink/renderer/platform/wtf/text/wtf_string.h
,
Oct 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/395e56f1f58492347fdad75595cd85463fd68ee3 commit 395e56f1f58492347fdad75595cd85463fd68ee3 Author: Rune Lillesveen <futhark@chromium.org> Date: Thu Oct 18 10:56:02 2018 Early out for empty text nodes, both reattach and visit. The WhitespaceAttacher should just ignore empty text nodes as if they weren't there in order to consider correct text node siblings for figuring out which whitespace nodes need a LayoutText. Bug: 894674 Change-Id: I27a96779678c972093dec6e5f2e19d6a40939065 Reviewed-on: https://chromium-review.googlesource.com/c/1288351 Commit-Queue: Rune Lillesveen <futhark@chromium.org> Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Cr-Commit-Position: refs/heads/master@{#600724} [delete] https://crrev.com/d5f79a75aec45e299eeab35bdea54477ee924513/third_party/WebKit/LayoutTests/external/wpt/html/dom/elements/the-innertext-idl-attribute/multiple-text-nodes.window-expected.txt [modify] https://crrev.com/395e56f1f58492347fdad75595cd85463fd68ee3/third_party/blink/renderer/core/dom/whitespace_attacher.cc
,
Oct 18
|
||||
►
Sign in to add a comment |
||||
Comment 1 by yosin@chromium.org
, Oct 12Components: -Blink>HTML Blink>Editing>Serialization
Owner: yosin@chromium.org
Status: Started (was: Available)