New issue
Advanced search Search tips

Issue 859410 link

Starred by 1 user
Status: Fixed
Owner:
yosin@chromium.org
Closed: Aug 27
Cc:
editing-dev@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Element#innerText for not being rendered element should not have newline for BR

Project Member Reported by yosin@chromium.org, Jul 2 
Element#innerText for not being rendered element should not have newline for BR element to follow the specs[1][2].

This is caused by Element::innerText() calls Node::textContent() with
emitting newline for BR option.

String Node::textContent(bool convert_brs_to_newlines = false) const

String Element::innerText() {
...
  if (!GetLayoutObject() && !HasDisplayContentsStyle())
    return textContent(true);
...
}


[1] https://html.spec.whatwg.org/multipage/dom.html#the-innertext-idl-attribute
[2] https://dom.spec.whatwg.org/#dom-node-textcontent

Comment 1 by yosin@chromium.org, Jul 2

Components: UI>Browser>ReaderMode
Labels: -Pri-3 M-70 Pri-1
Add UI>Browser>ReaderMode to components since the dom-distiller issue[1] is a
block of fixing this issue.

[1] https://github.com/chromium/dom-distiller/issues/10 WebTextTest.testGenerateOutputBRElements should have spec complaint test expectaion
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 2 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1adc476524936a5687a5257dc2080aa8d7423828

commit 1adc476524936a5687a5257dc2080aa8d7423828
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Mon Jul 02 09:17:27 2018

Utilize w3c test harness in fast/dom/inner-text-with-no-renderer.html

This patch changes "inner-text-with-no-renderer.html" to utilize w3c test
harness for ease of maintenance.

This patch is a preparation of the patch[1].

[1] http://crrev.com/c/1114673 Make Element#innerText specification compliant

Bug:  859410 
Change-Id: I108b1225a72517d1afa7402f2c184d7b3e5b1043
Reviewed-on: https://chromium-review.googlesource.com/1122035
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571849}
[delete] https://crrev.com/6fdf89d459260449b6de4c942d1999f27b27302c/third_party/WebKit/LayoutTests/fast/dom/inner-text-with-no-renderer-expected.txt
[modify] https://crrev.com/1adc476524936a5687a5257dc2080aa8d7423828/third_party/WebKit/LayoutTests/fast/dom/inner-text-with-no-renderer.html
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 2 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d06b421696496929039edf01232ff856b6762433

commit d06b421696496929039edf01232ff856b6762433
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Mon Jul 02 09:19:11 2018

Utilize w3c test harness in editing/pasteboard/newlines-around-floating-or-positioned.html

This patch changes "newlines-around-floating-or-positioned.html" to utilize
w3c test harness for ease of maintenance.

This patch is a preparation of the patch[1].

[1] http://crrev.com/c/1114673 Make Element#innerText specification compliant

Bug:  859410 
Change-Id: Ic582771158dae7d2eb936d1581aa31bdb7b9cd63
Reviewed-on: https://chromium-review.googlesource.com/1122032
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571851}
[delete] https://crrev.com/f3902a8b461d42ed85addad2d4047421ed28632d/third_party/WebKit/LayoutTests/editing/pasteboard/newlines-around-floating-or-positioned-expected.txt
[modify] https://crrev.com/d06b421696496929039edf01232ff856b6762433/third_party/WebKit/LayoutTests/editing/pasteboard/newlines-around-floating-or-positioned.html
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 3 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a1bab9cc7a7874a4a50cd02f4b6dc2e445823d62

commit a1bab9cc7a7874a4a50cd02f4b6dc2e445823d62
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Tue Jul 03 02:07:32 2018

Utilize w3c test harness in fast/dom/row-inner-text.html

This patch changes "row-inner-text.html" to utilize w3c test harness for ease of
maintenance.

This patch is a preparation of the patch[1].

[1] http://crrev.com/c/1114673 Make Element#innerText specification compliant

Bug:  859410 
Change-Id: I2efbd96aca89a2c4e5b63d62dcc3dc2f2968d8e0
Reviewed-on: https://chromium-review.googlesource.com/1122138
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572085}
[modify] https://crrev.com/a1bab9cc7a7874a4a50cd02f4b6dc2e445823d62/third_party/WebKit/LayoutTests/fast/dom/row-inner-text.html
[delete] https://crrev.com/665298df3e20cb930d7f05797125ab3d0d9d3698/third_party/WebKit/LayoutTests/platform/linux/fast/dom/row-inner-text-expected.png
[delete] https://crrev.com/665298df3e20cb930d7f05797125ab3d0d9d3698/third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/dom/row-inner-text-expected.png
[delete] https://crrev.com/665298df3e20cb930d7f05797125ab3d0d9d3698/third_party/WebKit/LayoutTests/platform/mac/fast/dom/row-inner-text-expected.png
[delete] https://crrev.com/665298df3e20cb930d7f05797125ab3d0d9d3698/third_party/WebKit/LayoutTests/platform/mac/fast/dom/row-inner-text-expected.txt
[delete] https://crrev.com/665298df3e20cb930d7f05797125ab3d0d9d3698/third_party/WebKit/LayoutTests/platform/win/fast/dom/row-inner-text-expected.png
[delete] https://crrev.com/665298df3e20cb930d7f05797125ab3d0d9d3698/third_party/WebKit/LayoutTests/platform/win/fast/dom/row-inner-text-expected.txt
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 3 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/39096e3e136f555b3c4b960d29b8237020c911db

commit 39096e3e136f555b3c4b960d29b8237020c911db
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Tue Jul 03 04:52:29 2018

Utilize w3c test harness in fast/dom/NodeList/childNodes-reverse-iteration.html

This patch changes "childNodes-reverse-iteration.html" to utilize w3c test
harness for ease of maintenance.

This patch is a preparation of the patch[1].

[1] http://crrev.com/c/1114673 Make Element#innerText specification compliant

Bug:  859410 
Change-Id: Ie4f834134639bd2b21838764c032183319d62a73
Reviewed-on: https://chromium-review.googlesource.com/1122073
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572111}
[delete] https://crrev.com/8a700fb27e8b6e9c69dcafc8cf1d8632e630faf9/third_party/WebKit/LayoutTests/fast/dom/NodeList/childNodes-reverse-iteration-expected.txt
[modify] https://crrev.com/39096e3e136f555b3c4b960d29b8237020c911db/third_party/WebKit/LayoutTests/fast/dom/NodeList/childNodes-reverse-iteration.html
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 4 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8262c5befe2ef2256db95c0a828d55dc81f02951

commit 8262c5befe2ef2256db95c0a828d55dc81f02951
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Wed Jul 04 04:57:45 2018

Introduce NGPhysicalTextFragment::IsGeneratedText()

This patch introduces |NGPhysicalTextFragment::IsGeneratedText()| with
|NGTextType::kGeneratedText| to distinguish between normal text and generated
text, e.g. hyphen and ellipsis, for sorting fragments with |StartOffset()|.

This patch is a preparation of the patch[1].

[1] http://crrev.com/c/1114673 Make Element#innerText specification compliant

Bug:  859410 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Ic9879985d1b5dd4cab921ce042ebc393043e3166
Reviewed-on: https://chromium-review.googlesource.com/1122061
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572478}
[modify] https://crrev.com/8262c5befe2ef2256db95c0a828d55dc81f02951/third_party/blink/renderer/core/layout/ng/inline/ng_physical_text_fragment.h
[modify] https://crrev.com/8262c5befe2ef2256db95c0a828d55dc81f02951/third_party/blink/renderer/core/layout/ng/inline/ng_physical_text_fragment_test.cc
[modify] https://crrev.com/8262c5befe2ef2256db95c0a828d55dc81f02951/third_party/blink/renderer/core/layout/ng/inline/ng_text_fragment_builder.cc
[modify] https://crrev.com/8262c5befe2ef2256db95c0a828d55dc81f02951/third_party/blink/renderer/core/layout/ng/inline/ng_text_fragment_builder.h
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 14 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/81db97a9a7f000ed4133a9891781658945853ae3

commit 81db97a9a7f000ed4133a9891781658945853ae3
Author: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Date: Tue Aug 14 00:10:11 2018

Roll DOM Distiller JavaScript distribution package

Diff since last roll:
https://chromium.googlesource.com/chromium/dom-distiller/+/9596033e36..ccfe233400

Picked up changes:
https://chromium.googlesource.com/chromium/dom-distiller/+log/9596033e36..ccfe233400
ccfe233 Fix EmbedExtractorTest.testDivCaption
8825eaf In tests, render elements before getting their innerText
8c9af2c Make generateOutput(textOnly=true) standard compliant

Bug:  651764 , 859410 , 873291 , 873298 
Change-Id: I66bf2c9a433751597fc4c7153ce5955e28dad064
Reviewed-on: https://chromium-review.googlesource.com/1171847
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Commit-Queue: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582763}
[modify] https://crrev.com/81db97a9a7f000ed4133a9891781658945853ae3/DEPS
[modify] https://crrev.com/81db97a9a7f000ed4133a9891781658945853ae3/third_party/dom_distiller_js/README.chromium
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 24 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/72d438ee5563db3087ea1f6630834343bdb8fb65

commit 72d438ee5563db3087ea1f6630834343bdb8fb65
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Fri Aug 24 08:17:29 2018

Implement Element#innerText to conform the spec

NOT READY FOR COMMIT
To commit this patch, I need to do:
 - Rebaseline 9500+ layout files for linux, mac, win, since existing
 implementation doesn't conform the spec[1]
 - Fix DOM distiller bug[3]
  It depends on textContent(true) to have newline for <br>
  https://github.com/chromium/dom-distiller/issues/10
  WebTextTest.testGenerateOutputBRElements should have spec complaint test
  expectation
 - Fix CrSettingsSiteDetailsPermissionTest.All
   change expectations to have <option>s


This patch implements Element#innerText to conform the spec[1].
Pass rate of WPT is changed from 78 failures to 6 failures for 213 test cases.

The design doc is https://goo.gl/VW9xxe.

The differences of current implementations are:
 - No more leading/training newlines
 - No more trailing whitespaces
 - At most two newlines between sequences of <p> and <div>.
 - Contents of <select>, <optgroup> and <option> in result.
 - No newline for <br> for disconnected element.

Note: Handling of <select>, <optgroup> and <option> aren't conformed with the
spec[1] since the spec[1] requires to implement Element#innerText specific
CSS handling, ::first-line, ::first-letter, text-transform etc, for contents
of <option>. I filed the issue[2].


[1] https://html.spec.whatwg.org/multipage/dom.html#the-innertext-idl-attribute
[2] https://github.com/whatwg/html/issues/3797 innerText for <select>, <optgroup> and <option>

TBR=alexmos@chromium.org
TBR=dpapad@chromium.org
TBR=dmazzoni@chromium.org
TBR=skyostil@chromium.org

Bug:  651764 ,  859410 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I48a02db0347d8ebd189f3ef608b31a4a93d89e84
Reviewed-on: https://chromium-review.googlesource.com/1114673
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585756}
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/chrome/browser/chrome_security_exploit_browsertest.cc
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/chrome/test/data/webui/settings/site_details_permission_tests.js
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/content/test/data/accessibility/aria/aria-textbox-with-rich-text-expected-mac.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/content/test/data/accessibility/aria/aria-textbox-with-rich-text-expected-win.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/content/test/data/accessibility/html/contenteditable-descendants-expected-blink.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/content/test/data/accessibility/html/contenteditable-descendants-expected-mac.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/content/test/data/accessibility/html/contenteditable-descendants-expected-win.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/content/test/data/accessibility/html/contenteditable-descendants-with-selection-expected-blink.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/content/test/data/accessibility/html/contenteditable-descendants-with-selection-expected-mac.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/content/test/data/accessibility/html/contenteditable-descendants-with-selection-expected-win.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/content/test/data/accessibility/html/contenteditable-with-embedded-contenteditables-expected-blink.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/content/test/data/accessibility/html/contenteditable-with-embedded-contenteditables-expected-mac.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/content/test/data/accessibility/html/contenteditable-with-embedded-contenteditables-expected-win.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/headless/test/data/protocol/sanity/renderer-in-cross-origin-object-expected.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/WebKit/LayoutTests/accessibility/menu-list-sends-change-notification-expected.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/WebKit/LayoutTests/accessibility/multiselect-list-reports-active-option-expected.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/WebKit/LayoutTests/editing/pasteboard/innerText-inline-table.html
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/WebKit/LayoutTests/editing/pasteboard/newlines-around-floating-or-positioned.html
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/WebKit/LayoutTests/external/wpt/html/dom/elements/the-innertext-idl-attribute/getter-expected.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/WebKit/LayoutTests/fast/dom/inner-text-001-expected.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/WebKit/LayoutTests/fast/dom/inner-text-first-letter.html
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/WebKit/LayoutTests/fast/dom/inner-text-with-no-renderer.html
[delete] https://crrev.com/40f1a7556c25c113bf1f7fcd9b36cfe686051bc7/third_party/WebKit/LayoutTests/fast/forms/select/menulist-no-renderer-for-unexpected-children-expected.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/WebKit/LayoutTests/fast/forms/select/menulist-no-renderer-for-unexpected-children.html
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/external/wpt/html/dom/elements/the-innertext-idl-attribute/getter-expected.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/WebKit/LayoutTests/http/tests/devtools/editor/text-editor-indent-autodetection-expected.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/WebKit/LayoutTests/http/tests/devtools/help/release-note-expected.txt
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/WebKit/LayoutTests/http/tests/devtools/network/network-cookies-pane.js
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/blink/renderer/core/dom/element.cc
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/blink/renderer/core/dom/element.h
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/blink/renderer/core/editing/BUILD.gn
[add] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/blink/renderer/core/editing/element_inner_text.cc
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/blink/renderer/core/editing/ime/input_method_controller_test.cc
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/blink/renderer/core/editing/iterators/text_iterator_behavior.h
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/blink/renderer/core/exported/web_document.cc
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/blink/renderer/core/html/forms/html_option_element.cc
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/blink/renderer/core/html/forms/html_option_element.h
[modify] https://crrev.com/72d438ee5563db3087ea1f6630834343bdb8fb65/third_party/blink/renderer/core/layout/ng/inline/ng_physical_text_fragment.h

Comment 10 by yosin@chromium.org, Aug 27

Status: Fixed (was: Started)
New implementation of Element#innerText no longer emits newline for <br>.
►

Sign in to add a comment