1738134 - Google Sheets ignores first Ctrl+Enter newline when entering text in a cell

Status: RESOLVED FIXED

(Web Compatibility :: Desktop, defect)

Description

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

Masayuki, this is a regression in Firefox 94 from HTMLEditUtils::IsVisibleBRElement() bug 1727868. I bisected this regression to this pushlog:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f3a453eb5700042249820b9cb060492c64e5a5c2&tochange=0c79c667c59a344d54f360ffe67b92823790b6e7

Steps to reproduce

1. Create a new Google Spreadsheet: https://docs.google.com/spreadsheets/
2. In a cell, type a string like a.
3. While still editing the cell, press Ctrl+Enter to insert a newline in the cell and type a string like b.
4. Then press Ctrl+Enter again to insert a newline in the cell and type a string like c.

Expected result

The cell should have three lines that look like:

a
b
c

Actual result

The cell has only two lines and looks like:

ab
c

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Probably not going to fix this in 94.x, but I'd be open to a dot release ride along if there's a simple, low-risk patch available.

Comment 2

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

Cannot reproduce this with minimal testcase:

data:text/html,<div contenteditable style="white-space:pre-wrap"><span>a%0A<br></span></div>

nor simple code to implement it: https://jsfiddle.net/d_toybox/1r6ae52o/

, and Ctrl + Enter isn't standard key assign of Gecko to insert a line break. So this could be caused by their script.

Comment 3

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

Er, no. It's indeed a bug of the method. The innerHTML at typing Ctrl + Enter is:

a<br><span style="font-size:13px;color:#000000;font-weight:normal;text-decoration:none;font-family:'Arial';font-style:normal;text-decoration-skip-ink:none;"></span>

It seems that the <br> element is considered as visible due to followed by the <span>. Keep investigating...

Comment 4

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

Previously, it used WSRunScanner::ScanNextVisibleNodeOrBlockBoundary() looking for a following block boundary or visible node.

It treats inline elements as visible even if actually not so.

Now, the method works differently, it ignores all inline elements except replaced elements.

But the new behavior matches with actual layout result. So, it fixed* the mismatch...

Comment 5

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

Hmm, I wonder, the <span> in Google Sheets is visible (width: 0px, height: 15px) so that the preceding <br> element is visible. Why?? I don't see any style to make it visible...

Comment 6

[Alice0775 White]

FWIW,
Login to Google Sheets.
Then Spoof the UA to "Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/95.0.4638.54 Safari/537.36" and reload the spreadsheet.
Then the Ctrl+Enter works as expected.

Comment 7

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

(In reply to Alice0775 White from comment #6)

FWIW,
Login to Google Sheets.
Then Spoof the UA to "Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/95.0.4638.54 Safari/537.36" and reload the spreadsheet.
Then the Ctrl+Enter works as expected.

Thank you, Alice-san, the cell is

<div
  class="cell-input editable"
  tabindex="0"
  role="combobox"
  docs-unhandledkeys="ctrl+shift+z,ctrl+y,ctrl+z"
  id="waffle-rich-text-editor"
  style="background-color: rgb(255, 255, 255);
         font-size: 13px;
         color: rgb(0, 0, 0);
         font-weight: normal;
         font-family: &quot;Arial&quot;;
         font-style: normal;
         text-decoration-skip-ink: none;"
  dir="ltr"
  g_editable="true"
  aria-autocomplete="list"
  aria-label="C6"
  contenteditable="true"
>a<br><span style="font-size:13px;
                   color:#000000;
                   font-weight:normal;
                   text-decoration:none;
                   font-family:'Arial';
                   font-style:normal;
                   text-decoration-skip-ink:none;"></span></div>

And we had not supported white-space:pre-wrap in editor since bug 1724650. So that I guess that they use a specific path for Gecko and that depends on some bugs of Gecko (our editor does not refer CSS in most cases, that makes web developers confused like checking the <br> element is visible or not).

Comment 8

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

The reason why the text is inserted before the <br> element is, editor adjusts caret before invisible <br> element before inserting text.

However, if I write WPT with data:text/html,<div contenteditable style="white-space:pre-wrap">a<br><span style="padding:1px"></span></div>, Chrome also inserts new text before the <br> element. So, restoring the original behavior restores incompatible behavior too. So we should not change the behavior for the other web apps.

Comment 9

[Karl Dubost💡 :karlcow]

I wonder if the webcompat team can fix this with a site intervention.
Let's try to contact first Google.

Comment 10

[Karl Dubost💡 :karlcow]

Google has been contacted.

Comment 11

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

Attachment: Bug 1738134 - Add new tests to check the behavior of `insertText` command when caret is after "invisible" <br> element r=m_kato!

If a <br> element follows a visible thing and there is no visible content
between it and following block boundary (both child's and parent's), it's
called as "invisible" BR element at Mozilla.

Selection API can put caret after it but before the following block boundary
and of course, user may be able to put caret at there if the browser has a
bug like https://bugzilla.mozilla.org/show_bug.cgi?id=1738193.

Even in this case, user can type something if it's in editable content, and
even though this is an edge case, it's safe to get same result in any browsers
since web apps may put caret there accidentally.

The expected results are based on Blink's behavior and Gecko fails the cases
if the following block boundary is of a child block. However, no patch for
Gecko for now.

Comment 12

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

Sorry for the bug spam due to the wrong bug#.

Comment 13

[Phabricator Automation]

Comment on attachment 9248812 [details]
Bug 1738134 - Add new tests to check the behavior of insertText command when caret is after "invisible" <br> element r=m_kato!

Revision D130137 was moved to bug 1738200. Setting attachment 9248812 [details] to obsolete.

Comment 14

[Pascal Chevrel:pascalc]

This is getting late in the cycle for a potential fix to be uplifted to beta, marking as wontfix for 95.

Comment 15

[Karl Dubost💡 :karlcow]

Masayuki-san,
Is it an actionable bug for Core? Or is it just an issue where you would like the webcompat team to do outreach to the site.

Comment 16

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

(In reply to Karl Dubost💡 :karlcow from comment #15)

Masayuki-san,
Is it an actionable bug for Core? Or is it just an issue where you would like the webcompat team to do outreach to the site.

See comment 8, I think that we can do nothing in Core.

Comment 17

[Karl Dubost💡 :karlcow]

Attachment: current-rendering

Thanks Masayuki-san.

On December 2021, on the list, Google said:

We've identified a fix, and it should reach production within a few weeks. There was a Firefox-specific hack that became no longer necessary in some recent build, so we just removed the hack.

Let's test if it has been fixed.

And yes this has been fixed. We can close here.