Closed Bug 1390562 Opened 7 years ago Closed 7 years ago

regression: enter key doesn't insert newline in gmail compose window if "editor.use_div_for_default_newlines" is false

Categories

(Core :: DOM: Editor, defect, P3)

Product:
Core
Component:
DOM: Editor
Version:
56 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- verified
firefox59 --- verified

People

(Reporter: sshock3, Assigned: masayuki)

References

Details

Attachments

(2 files)

Bug 1390562 - part 0: Add automated tests
59 bytes, text/x-review-board-request
m_kato
: review+
ritu
: approval-mozilla-beta-
Details
Bug 1390562 - part 1: HTML editor shouldn't split <div> container at inserting a line break for backward compatibility if defaultParagraphSeparator is "br"
59 bytes, text/x-review-board-request
m_kato
: review+
ritu
: approval-mozilla-beta-
Details 
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170809080026

Steps to reproduce:

Log in to gmail.com.  Hit Compose button.  Type three lines, leaving the middle line blank.  Now go to the end of the first line and hit enter.


Actual results:

The cursor moves to the next line but doesn't insert a new line.


Expected results:

Should insert a new line and move to it.

I've also noticed at times I am moving the cursor around and it gets "stuck" (e.g., have to hit down arrow twice to move down).
Component: Untriaged → Editor
Product: Firefox → Core
Could you reproduce this on the latest Nightly from nightly.mozilla.org?
Flags: needinfo?(sshock3)
I cannot reproduce it in Nightly (57.0a1 (2017-08-21) (64-bit)).
Flags: needinfo?(sshock3)
Thanks, since I cannot reproduce on 56 too, I think that this has already fixed on 56.
You can reopen this bug when this occurs on 56 or 57+.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
I tested and this bug occurs in 56 too.  Re-opening.
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
Version: 55 Branch → 56 Branch
Could you investigate what bug number this is fixed using http://mozilla.github.io/mozregression/ since I cannot reproduce this?
If we can find bug number that this is fixed, we might be able to uplift to 56.
Flags: needinfo?(sshock3)
Which version of nightly corresponds to Firefox 55.0.3.  Also, which one corresponds to Firefox 56.0b7?

I'm a little confused because I tried mozregression with dozens of different versions between now and a few months ago, and could not reproduce the problem.

But I tested several of my own computers, all running official 55.0.3, and they all have the problem.  Also, the other day I installed 56.0b7 and it had the problem.

I thought maybe this has something to do with and extension, but I disabled all extensions and the problem persists.

Lastly, I got on a fresh Windows computer that had never had Firefox on it before, and installed Firefox 55.0.3, and the problem was there.

So I think if I can find the version of nightly that corresponds to 55.0.3 then maybe I could reproduce this.  I would even bisect both sides for you (both when the bug was introduced, and when it was fixed).  But I can't seem to find a broken version while using mozregression.
Flags: needinfo?(sshock3) → needinfo?(m_kato)
(In reply to Phillip from comment #6)
> Which version of nightly corresponds to Firefox 55.0.3.  Also, which one
> corresponds to Firefox 56.0b7?

As long as Comment# 2, this seems to be fixed by 57 cycle.  Since I cannot reproduce this, even if we want to fix this on 56 cycle, we cannot fix on 56.
Flags: needinfo?(m_kato)
Priority: -- → P5
But here's what worries me.  Since I also cannot reproduce this with any nightly build, but I can reproduce it with official builds, (this makes no sense but) maybe somehow this bug only manifests in official builds.

So if we don't figure out what's really going on here, maybe the bug actually exists and will continue to exist even when 57 is officially released.  Maybe the bug has to do with some environmental setting or something (although I did reproduce it on a fresh machine and with no extensions, maybe it is still somehow related to an environmental or a config setting; maybe config settings are different when you install an official release than when you install nightly???)

If you could actually answer my question (how do I get a nightly build that corresponds to 55.0.3 or 56.0b7), then I could install that exact nightly build that corresponds to the official versions I have seen the problem in.  Then depending on whether or not I could reproduce it, we could actually say something useful and have a direction to go.
Flags: needinfo?(m_kato)
i can reproduce the problem with str of comment#0 on 56.0b8.

Regression window in beta channel:
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=05183e252fc3ea344360c3f0c95ed5ea02b15503&tochange=0d22b3809fa05faf076bed6b4f03e8f950109446

Triggered by:
0d22b3809fa0	Julien Cristau — Post Beta 5: Unset EARLY_BETA_OR_EARLIER. a=jcristau

Ugh, I hate the flag EARLY_BETA like that. It impossible to find out regression window.
Status: UNCONFIRMED → NEW
status-firefox55: --- → unaffected
status-firefox56: --- → affected
status-firefox57: --- → ?
Ever confirmed: true
setting editor.use_div_for_default_newlines = true fixes the problem

So, Triggered by: Bug 1357482
Blocks: 1357482
Summary: regression: enter key doesn't insert newline in gmail compose window → regression: enter key doesn't insert newline in gmail compose window if "editor.use_div_for_default_newlines" is false
Flags: needinfo?(m_kato)
Very happy to hear that I can workaround this problem by setting editor.use_div_for_default_newlines to true.  I tried it and indeed that fixes it.

So I guess all the nightly builds I tried worked fine because this setting is true by default in nightly builds.  But in FF 55 (shipping) and FF 56 (beta) it must be off by default.

So there is a workaround, but we still want to make it work correctly even when the setting is off, correct?  Especially since apparently false will remain the default (until 57, is that right?)

Thanks for working on this guys.  So do I understand correctly that the decision is to not bother trying to fix this for 55?  Is that because 56 is close to being released, so you only want to worry about fixing it there?

Thanks again,
Phillip
Hmm, I cannot reproduce this bug with the following steps:

1. Click "COMPOSE" button of Gmail.
2. Type "abc" + Enter + Enter + "def".
3. Click at right space of "abc".
4. Type Enter.

I tested both Firefox 56 and Nightly + "editor.use_div_for_default_newlines, false".

Do I misunderstand the STR or has it already been fixed by Google?
Oh, I succeeded to reproduce this bug with another my gmail account. Odd...
When I do #2 of the STR, the document tree is like this:

<div>
  <div>
    abc[]
    <br>
    <br>
  </div>
  def
  <br>
</div>

Then, typing Enter changes it to:

<div>
  <div>abc</div>
  <div>
    <br>
  </div>
  def
  <br>
</div>

So, it's confused at the container <div> vs following <br>.
On ESR, the changed DOM tree is:

<div>
  <div>
    abc
    <br>
    <br>
    <br>
  </div>
  def
  <br>
</div>

So, we inserted a <br> rather than splitting the container <div>.
Makoto-san, this occurs in release build and non-early beta and in Gmail. Do you still thing that this is P5?
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(m_kato)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #16)
> Makoto-san, this occurs in release build and non-early beta and in Gmail. Do
> you still thing that this is P5?

Ah, this doesn't occur on Nightly, so I mark as P5.  But we should raise priority since use_div_for_default_newlines=true isn't shipped
Flags: needinfo?(m_kato)
Priority: P5 → P3
Comment on attachment 8914756 [details]
Bug 1390562 - part 1: HTML editor shouldn't split <div> container at inserting a line break for backward compatibility if defaultParagraphSeparator is "br"

https://reviewboard.mozilla.org/r/186040/#review191468
Attachment #8914756 - Flags: review?(m_kato) → review+
Comment on attachment 8914755 [details]
Bug 1390562 - part 0: Add automated tests

https://reviewboard.mozilla.org/r/186038/#review191470
Attachment #8914755 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b004d391f787
part 0: Add automated tests r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ef89adcec2ab
part 1: HTML editor shouldn't split <div> container at inserting a line break for backward compatibility if defaultParagraphSeparator is "br" r=m_kato
https://hg.mozilla.org/mozilla-central/rev/b004d391f787
https://hg.mozilla.org/mozilla-central/rev/ef89adcec2ab
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8914755 [details]
Bug 1390562 - part 0: Add automated tests

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1357482 (changed the logic) and bug 1357482 (taking back the older mode for backward compatibility but it wasn't enough).

[User impact if declined]:
Gmail users may see odd Enter key handling. And also some users may meet similar bug in other web apps.

[Is this code covered by automated tests?]:
Yes, this patch is the automated test.

[Has the fix been verified in Nightly?]:
Yes, I confirmed.

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes,

0. Make sure that "editor.use_div_for_default_newlines" is false in about:config.
1. Open Gmail (not GSuite's, I couldn't reproduce this bug with my mozilla.com account).
2. Click "COMPOSE".
3. Unselect "plaintext mode". (I.e., use HTML mail mode.)
4. Type "abc", Enter, Enter, "def".
5. Click right space of "c".
6. Type Enter.

Then, new line should be inserted.

[List of other uplifts needed for the feature/fix]:
Actual fix is the following patch.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Only adding new test.

[String changes made/needed]:
No.
Attachment #8914755 - Flags: approval-mozilla-beta?
Comment on attachment 8914756 [details]
Bug 1390562 - part 1: HTML editor shouldn't split <div> container at inserting a line break for backward compatibility if defaultParagraphSeparator is "br"

Approval Request Comment
[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
This does a partial backing out from the backward compatibility mode. So, taking back our traditional behavior only when it's in backward compatibility mode. (In Nightly, using new standardized behavior in the default setting. This patch does NOT change the behavior of the standardized mode.)

[String changes made/needed]:
No.
Attachment #8914756 - Flags: approval-mozilla-beta?
Comment on attachment 8914755 [details]
Bug 1390562 - part 0: Add automated tests

This is not a new regression. I am trying to limit the risk to 57 quality by limiting uplifts to recent regressions caused by new feature work in 57. Recommend letting this fix ride the 58 train.
Attachment #8914755 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8914756 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Flags: qe-verify+
I managed to reproduce the bug using Firefox 57.0 - build 4 and beta 57.0b12 on Windows 10 x64. 
I retested everything on latest Nightly and beta 58.0b4 using Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64 and couldn't reproduce the bug anymore. 
The questions raised is that considering the fact that this issue couldn't be reproduced on older versions of Nightly, is this verification valid?
Flags: needinfo?(masayuki)
The default value of "editor.use_div_for_default_newlines" of Nightly is true, however, this is reproducible only when it's false (default value of release build). So, if you test this with Nightly, you need to set it to false from about:config and reload the page.
Flags: needinfo?(masayuki)
I retested everything with the pref editor.use_div_for_default_newlines changed to false, but the bug is not reproducing anymore. 
I tested on latest Nightly 59 on Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x32.
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
status-firefox59: --- → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: