I have a stackoverflow page for this at: https://stackoverflow.com/questions/52901043/chrome-v70-has-issue-with-innertext-in-td-cells

yosin@, could you see if this relates to your innerText work?

Mark WontFix since this is expected behavior of the spec[1].
Element#innerText adds TAB character(U+0009) for table-cell except for the last table cell in the row.

Step 6 of the spec[1]
If node's computed value of 'display' is 'table-cell', and node's CSS box is not the last 'table-cell' box of its enclosing 'table-row' box, then append a string containing a single U+0009 CHARACTER TABULATION (tab) character to items.

Should we change the spec not to include TAB character for getting innerText just for table-cell?
If so, please file an issue in WHATWG issue repository[2].


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

I contacted Simon Pieters[1] about the original commit that added this spec change[2] about innerText appending TAB character (U+0009) if the table cell is not the last in a row. No response yet

[1] https://github.com/zcorpan
[2] https://github.com/whatwg/html/commit/132a55eb22d419566650c6f2a38c53e68475e7a3

Thanks Brett. Did you send him a direct message or file a github issue? If github, can you link to it here?

The spec tries to represent a table with tabs and newlines. That sounds fine (at least before studying the compat/interop stories, which don't look good at first glance). There are a ton of tests for getting the innerText of a whole table [1] but none that check getting innerText for just a cell. Which raises the possibility that the spec text was not written with retrieving innerText of a lone table-cell in mind (which would be surprising; zcorpan and roc are usually thorough, so I may be missing something)

[1] https://github.com/web-platform-tests/wpt/blob/e078010b4a0dc252fccac8ce7436403d66ff31de/innerText/getter-tests.js

Further, it appears the other 3 engines do not add the tab character even for whole table innerText [2].
yosin@, do you know what the other engines plan to do here? Do you have a link to your Intent to Ship?

[2] https://wpt.fyi/results/html/dom/elements/the-innertext-idl-attribute/getter.html?aligned&label=stable

In the context of [3] below, I'm reopening this issue until we get resolution on (A) what the other engines do, (B) plan to do, and (C) what the spec intended for td.innerText with respect to the trailing tab character.

[3] https://www.chromium.org/blink/platform-predictability says
> If 3+ engines violate a spec in the same way, the bug is probably in the spec (especially when changing the engines would break websites).  Specs are much easier to change than multiple implementations!

links for convenience
 Issue 651764  is where the impl work for this happened
https://github.com/whatwg/compat/issues/5 is where writing the spec was discussed

I sent him a direct email. He is saying this was original behavior from Firefox? Does that mean it was in the W3C standard?

I agree that maybe it should be in place when doing a call to innerText for a table or row element, but I think it should behave like textContent when it is a call to a td cell with no children. Is that fair?

I also checked http://jsfiddle.net/L16x9gpe/ in the latest Firefox. It is not appending the tab characters for those values

yosin@ any thoughts?

I agree to exclude TAB character when innerText applied to TD element only case.
In other words, innerText should return item list for context-node.

brett@, could you file an issue to whatwg issue tracker[1] to get consensus from community?

Once we get agreement, we can change the spec with WPT tests and change Chrome's implementation?

In current spec, following elements have additional characters:
 - <br>: trailing newline; <br>.innerText = "\n"
 - <td>: trailing tab; <td>foo</td>.innerText = "foo\t"
 - <tr>: trailing newline; <tr>...</tr> = "...\n"

Note: block-level element, e.g. <div>, and <p> emits required newlines but they are excluded.


[1] https://github.com/whatwg/html/issues

This is an application breaking change. See browsers tested below using fiddle example.

Chrome Browsers Tested:
Windows 10
  Chrome 68: PASS
  Chrome 69: PASS
  Chrome 70: FAIL
  Chrome 71: FAIL

Other Browsers Tested:
Windows 10
  IE 11: PASS (11.15.16299.0)
  EDGE: PASS (42.17134.1.0)
  Firefox: PASS (62)
  Opera: PASS (56.0.3051.31)
Mac OS
  Safari: PASS (12)
Android v9 (Pixel 2)
  Chrome: PASS 

yosin@ and dgro...@ please have a look at the new spec change issue located here: https://github.com/whatwg/html/issues/4119

Your continued efforts and assistance in this matter is greatly appreciated.

foolip@, as platform predictability expert, could you weigh in on the question below?

Summary: old behavior had no spec but all engines agreed. A spec appeared that demanded different behavior. Blink implemented/shipped the new spec without any Intent (that I could find), which is designed to prevent these types of issues. The new behavior just hit stable and broke a bunch of sites (this many stars after this short time).

I think we should revert the change on stable ASAP, despite whatever the spec currently says. But sites that have already adapted to the new behavior will be broken _yet again_.

This is the opposite of predictability :(

yosin@, foolip@, thoughts on a path forward here, specifically around not waiting for the spec to change before reverting on stable? Do we have any rubric for making these decisions?

dgro...@ I disagree a reversion or new behavior to what other browsers are doing already will break sites again. I believe sites are already beginning to trim the output of innerText, so a trim of the fixed behavior will only return the correct output

Some sites will do something like

if (chrome)
  return td.innerText.chopOffOneCharacter()
else
  return td.innerText

Those sites will break if we revert.

Second, did chrome <= 69 (and other current browsers) ever return trailing whitespace at the end of innerText? I don't know. But if so, some sites might rely on that behavior and for them, trim() is not a good option.

That all said, I still think we should revert.

dgro...@

In my experience and testing, previous versions of both Chrome and other browsers have never returned a trailing whitespace.

I hope the consensus is to revert the change and soon.

For reference, old Chrome (<=69) has a lot of other bugs with whitespaces, like having trailing newlines at the end of nested blocks.

Here's the WPT fixed by the new implementation:

https://chromium.googlesource.com/chromium/src/+/72d438ee5563db3087ea1f6630834343bdb8fb65%5E%21/#F19

Our company's app solution was to use innerText only if using IE, as they didn't implement textContent, so our solution was:

if(ie)
  return td.innerText
else
  return td.textContent

also, in regards to a fix being used to "chop off" a character no matter what that character is, I would warn the developer to use trim instead.

I agree with tro...@ that we should revert and soon without waiting for a long spec change process to complete. 

yosin@ is there a way to expedite a interim solution while the working group makes a final decision? Thank you

Following #19: That said, it's still probably better to revert, both due to this issue, and also some other bugs caughted and fixed in M71:

https://chromium-review.googlesource.com/q/owner:yosin+status:merged+innerText

I created the patch to revert Element#innerText back to M69 behavior.
http://crrev.com/c/1298716

abdulsyed@, could you approve to merge this change to M70 branch?

For M71+, I'll work to fix the spec with considering interop and make Chrome to follow that.

This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

I talked Simon Pieters[1] F2F at TPAC. He agree to change the spec to match current Firefox behavior == not emit TAB character if innerText is applied to <td>.

We saw the Firefox implementation at glance but we could not find where Firefox not to emit TAB for <td>.


In nsRange.cpp[2]

      switch (f->StyleDisplay()->mDisplay) {
      case StyleDisplay::TableCell:
        if (!IsLastCellOfRow(f)) {
          result.Append('\t');
        }
        break;


static bool
IsLastCellOfRow(nsIFrame* aFrame)
{
  LayoutFrameType type = aFrame->Type();
  if (type != LayoutFrameType::TableCell &&
      type != LayoutFrameType::BCTableCell) {
    return true;
  }
  for (nsIFrame* c = aFrame; c; c = c->GetNextContinuation()) {
    if (c->GetNextSibling()) {
      return false;
    }
  }
  return true;
}




In Firefox, Element#innerText is essentially:

nsGenericHTMLElement::GetInnerText(...) {
...
  if (!IsRendered(*this)) {
    GetTextContentInternal(aValue, aError);
  } else {
    nsRange::GetInnerTextNoFlush(aValue, aError, this);
  }
}




[1] https://github.com/zcorpan
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsRange.cpp?q=nsRange.cpp&redirect_type=direct

Firefox runs innerText algorithm starts from first child node instead of context node itself. This is the reason why Firefox doesn't contain TAB character for <td>foo</td>.innerText


nsRange::GetInnerTextNoFlush(DOMString& aValue, ErrorResult& aError,
                             nsIContent* aContainer)
{
  InnerTextAccumulator result(aValue);

  if (aContainer->IsText()) {
    AppendTransformedText(result, aContainer);
    return;
  }

  nsIContent* currentNode = aContainer;
  TreeTraversalState currentState = AFTER_NODE;

  nsIContent* endNode = aContainer;
  TreeTraversalState endState = AFTER_NODE;

  nsIContent* firstChild = aContainer->GetFirstChild();
  if (firstChild) {
    currentNode = firstChild;  // Start with the first child.
    currentState = AT_NODE;
  }

  
  while (currentNode != endNode || currentState != endState) {
....
}

how critical is this? What are the implications if we wait until M71 to fix this? Do we need to merge this to M71 also?

This issue is marked with 20 stars. So, affects many applications.
We also want to merge into M71 too.

The bug is marked as P3 or Feature. It should not be merged as M71 is in beta. 
Please contact the approriate milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

 Issue 897643  has been merged into this issue.

Mark P1 to avoid auto reject of Merge-Request-71.

follip@, what do you think? Should we revert innerText behavior back to M69 for M70 and M71?

- The spec bug will be fixed soon since Simon recognized this
- M72 will follow new spec
- Some apps have already had workaround.

This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Which CL you're requesting a merge for? And is the change well baked/verified in canary, safe to merge now to M71?

Proposed spec fix and wpt
https://github.com/whatwg/html/pull/4132
https://github.com/web-platform-tests/wpt/pull/13743

>#c33, the patch is http://crrev.com/c/1298716
This is the revert of innerText back to M69 on M70 and M71 branch.
There are lots of layout test changes, 4000+ file, based of new innerText changes.
So, we don't apply this to canary and ToT.
ToT will be changed to follow the proposed spec in #c34.

Approving merge for Cl listed at #33 (http://crrev.com/c/1298716) to M71 branch 3578 based on comments #28 & #35. 

 Pls do check with follip@ before merging to M71 as per comment #31. Thank you.


+abdulsyed@ (M70 Desktop Release TPM) for M70 merge review.

 Issue 898376  has been merged into this issue.

yosin@ - can you please quick summarize this issue as we consider this for M70? We don't have any respins planned yet. 

1) Apart from failing developer tests, what is the user impact for this issue?
2) What are the implications if we target the fix for M71, and not fix this in M70? In other words, is this worth respinning for?
3) How safe is this fix overall? Is there a workaround that users can use in the meantime?

1) Apart from failing developer tests, what is the user impact for this issue?
I'm not sure actual user impact since it depends on web apps.

2) What are the implications if we target the fix for M71, and not fix this in M70? In other words, is this worth respinning for?
I can't answer this since I have no data for impact.

3) How safe is this fix overall? Is there a workaround that users can use in the meantime?
The patch changes innerText to use previous code path, which is used so far == we were living with it more than years. We've not removed old code path yet, so it is easy to switch back

foolip@, could you elaborate question 1 and 2?

 Issue 899704  has been merged into this issue.

My web app didn`t work suddenly. I have had to replace my code to trim the "\t" . It is the only workaround that I have found meanwhile. 

The spec change and the tests have now been merged (see comment 34).

yosin@, given that this is a regression, even if we can't quantify the impact exactly I think we should be conservative and either revert the change that caused this, or, if the change would be small, adapt to the spec changes in comment #34 and merge that to M70. For every web developer that reports a bug, there will be many that don't, so this probably has more impact than it immediately seems like. Does that make sense?

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Please ignore my "if the change would be small" options, I see that the plan is to merge http://crrev.com/c/1298716 and that makes sense I think, it's the least risky approach I can see.

Hi,

We have an application that is effected because of this innerText behaviour change on Chrome 70.
I have gone through this forum , but am unsure if the decision is to provide the fix on the 70 version or the next version,
Please let us know.
Thanks.

It looks like http://crrev.com/c/1298716 is a revert of just the changes to Element::innerText in https://chromium-review.googlesource.com/c/chromium/src/+/1114673/, and looking at both side-by-side it looks straightforward, assuming that nothing that Element::innerText() depends on has changed its behavior in the 10 weeks since the original change.

yosin@, have you confirmed that making the change on master today would pass all tests except the layout tests where a difference is expected?

We need to land something to M71 urgently for this to be even considered for M70. 

I've merged the change to M71 in https://chromium-review.googlesource.com/c/chromium/src/+/1308195.

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

commit 8d5d7ef47ad90ba1926109db68c0c71f8ab5ea7d
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Tue Oct 30 18:22:17 2018

Revert Element#innerText behavior back to M69 state

This patch reverts changes of Element#innerText in http://crrev.com/c/1114673,
since a number of sites are broken by having TAB character for getting
innerText from <td> for aligning Chrome to other browser.

Note: This patch doesn't revert layout test expectations to minimize this patch.

Bug:  897373 
Change-Id: I63c314b38ec4bf4b80c0c01a0407a4d7d787f69e
Reviewed-on: https://chromium-review.googlesource.com/c/1308195
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#406}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/8d5d7ef47ad90ba1926109db68c0c71f8ab5ea7d/third_party/blink/renderer/core/dom/element.cc
[modify] https://crrev.com/8d5d7ef47ad90ba1926109db68c0c71f8ab5ea7d/third_party/blink/renderer/core/editing/BUILD.gn

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

Commit: 8d5d7ef47ad90ba1926109db68c0c71f8ab5ea7d
Author: yosin@chromium.org
Commiter: foolip@chromium.org
Date: 2018-10-30 18:22:17 +0000 UTC

Revert Element#innerText behavior back to M69 state

This patch reverts changes of Element#innerText in http://crrev.com/c/1114673,
since a number of sites are broken by having TAB character for getting
innerText from <td> for aligning Chrome to other browser.

Note: This patch doesn't revert layout test expectations to minimize this patch.

Bug:  897373 
Change-Id: I63c314b38ec4bf4b80c0c01a0407a4d7d787f69e
Reviewed-on: https://chromium-review.googlesource.com/c/1308195
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#406}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}

Hello Chrome team, any idea when this fix will be released to production users ?  Our application was greatly impacted by this change, we are trying to see if we have to rush in a application fix or wait for this patch to be deployed. Please let me know.

Hello Chrome team, 
We would also like to know when this fix will be released to production users ?  Our application was greatly impacted by this change. Please let me know, so that we can decide on whether we need to fix our application code .

Chrome Team,
We are an enterprise solution provider and many of our software products hosted at different client locations are affected by this change. A quick action on your part would be greatly appreciated.

Thanks in advance!

abdulsyed@, with this now merged to M71, at what point can it be merged to M70?

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

commit 1425dba29becf42be33814faeb8ae485a01fcf07
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Fri Nov 02 09:34:59 2018

Make Element#innerText not to emit Tab/Newline for <td>/<tr>/<br> itself

This patch changes |Element#innerText| not to emit Tab/Newline for <td>, <br>,
<tr> itself to follow the spec change[1] for improving interop.

Example:
 - <br>.innerText -> "" (was "\n")
 - <td>abc</td>.innerText -> "abc" (was "abc\t")
 - <tr><td>abc</td></tr>.innerText -> "abc" (was "abc\n")

[1] https://github.com/whatwg/html/pull/4132 Make innerText getter not run the
algorithm on the element itself

Bug:  897373 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I4266ee28df9b44e9044dfa60c2d54257a5f82eeb
Reviewed-on: https://chromium-review.googlesource.com/c/1312436
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604872}
[modify] https://crrev.com/1425dba29becf42be33814faeb8ae485a01fcf07/third_party/WebKit/LayoutTests/external/wpt/html/dom/elements/the-innertext-idl-attribute/getter-expected.txt
[modify] https://crrev.com/1425dba29becf42be33814faeb8ae485a01fcf07/third_party/WebKit/LayoutTests/external/wpt/html/dom/elements/the-innertext-idl-attribute/getter-tests.js
[modify] https://crrev.com/1425dba29becf42be33814faeb8ae485a01fcf07/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/1425dba29becf42be33814faeb8ae485a01fcf07/third_party/blink/renderer/core/editing/element_inner_text.cc

Is CL listed at #58 need a merge to M70 & M71?

Nope, CL from #58 doesn't need to be merged anywhere, ignore that one.

CL from #53 was merged to 71. Now it needs to be merged to 70. yosin / foolip have the merge ready to go here https://chromium-review.googlesource.com/c/chromium/src/+/1298716 but I'm not sure if/how they've verified that the fix worked in 71.

I've verified the fix using http://jsfiddle.net/L16x9gpe/ and "71.0.3578.30 (Official Build) beta (64-bit)" on macOS.

abdulsyed@, is anything else required to merge to M70?

Thank you for verification. Approving merge for M70. 

branch:3538

Thanks for approving to merge into M70!
Merging in progress...

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

Commit: e5ddd94e1fe96cb17e681adb8508e502d1690896
Author: yosin@chromium.org
Commiter: yosin@chromium.org
Date: 2018-11-05 02:27:31 +0000 UTC

Revert Element#innerText behavior back to M69 state

This patch reverts changes of Element#innerText in http://crrev.com/c/1114673,
since a number of sites are broken by having TAB character for getting
innerText from <td> for aligning Chrome to other browser.

Note: This patch doesn't revert layout test expectations to minimize this patch.

TBR=foolp@chromium.org

Bug:  897373 
Change-Id: I63c314b38ec4bf4b80c0c01a0407a4d7d787f69e
Reviewed-on: https://chromium-review.googlesource.com/c/1298716
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#1064}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}

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

commit e5ddd94e1fe96cb17e681adb8508e502d1690896
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Mon Nov 05 02:27:31 2018

Revert Element#innerText behavior back to M69 state

This patch reverts changes of Element#innerText in http://crrev.com/c/1114673,
since a number of sites are broken by having TAB character for getting
innerText from <td> for aligning Chrome to other browser.

Note: This patch doesn't revert layout test expectations to minimize this patch.

TBR=foolp@chromium.org

Bug:  897373 
Change-Id: I63c314b38ec4bf4b80c0c01a0407a4d7d787f69e
Reviewed-on: https://chromium-review.googlesource.com/c/1298716
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#1064}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/e5ddd94e1fe96cb17e681adb8508e502d1690896/third_party/blink/renderer/core/dom/element.cc
[modify] https://crrev.com/e5ddd94e1fe96cb17e681adb8508e502d1690896/third_party/blink/renderer/core/editing/BUILD.gn

Summary
* M69: Non-spec compliant behavior
* M70: Non-spec compliant behavior (merged on Oct 30)
* M71: Non-spec compliant behavior (merged on Nov 5)
* M72: Spec-complaint behavior

 Issue 901276  has been merged into this issue.

abdulsyed@, for everyone following along with this issue, when should they expect a M70 point release with this fix included?

Appreciate the quick fix chrome team, please do let us know when we can expect a M70 version with this fix included.  

Hello, can you please let us know when the M70 will be applied the fix?

Hello, can you please let us know when the M70 will be applied the fix? We are waiting for your reply , so that we can decide if we need to fix this on the application side.

hi everyone - we're planning a respin for this week with the fix. I'll update the thread when we start rolling out. Thanks for everyone's feedback!

Able to reproduce the issue on win-10 using chrome build without fix.

Verified the fix on win-10 using Chrome version #70.0.3538.102 as per the comment #0. Attaching screen cast for reference.
Observed that there are no trailing characters in any cells.
Hence, the fix is working as expected. 
Adding the verified labels.

Thanks...!!

Deleted: 897373.mp4 683 KB

	897373.mp4 683 KB View Download

70.0.3538.102 appears to be live now! Thanks

Just updated to *.102 and there's no trailing characters. Thank you guys!

70.0.3538.102 works perfectly for my application as well! Canary version 72.0.3606.0  on Mac works perfectly as well now.