Last Comment Bug 680279 - Splitting Gmail rich-text list with Enter misplaces the caret
: Splitting Gmail rich-text list with Enter misplaces the caret
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla9
Assigned To: Fabien Cazenave [:kaze]
:
Mentors:
Depends on:
Blocks: 674861
  Show dependency treegraph
 
Reported: 2011-08-18 15:39 PDT by Nickolay_Ponomarev
Modified: 2011-10-25 22:19 PDT (History)
4 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
unaffected
+
fixed
fixed


Attachments
testcase (582 bytes, text/html)
2011-08-19 03:37 PDT, Fabien Cazenave [:kaze]
no flags Details
patch proposal (7.80 KB, patch)
2011-08-19 06:34 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch proposal (7.89 KB, patch)
2011-08-19 14:34 PDT, Fabien Cazenave [:kaze]
ehsan: review+
Details | Diff | Splinter Review
backout - bug674861 (6.67 KB, patch)
2011-08-19 14:54 PDT, Fabien Cazenave [:kaze]
blizzard: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Nickolay_Ponomarev 2011-08-18 15:39:40 PDT
Sorry for the Gmail testcase, I don't have the time to attempt to reduce it.

1. Log in https://mail.google.com/mail/?shva=1#inbox (the desktop 
2. click Compose
3. Create an unordered list by clicking the respective icon
4. type x, Enter, x, Enter, x, creating three list items with the text "x"
5. Position the caret at the end of the first item and press Enter twice

Expected results (roughly):
  <ul><li>x</li></ul>
  <p>{caret}</p>
  <ul>..the rest of the list</ul>

Actual results:
  <ul><li>x</li></ul>
  <ul><li>x</li><li>x</li></ul>
  <p>{caret}</p>

Works: 2011-08-03-03-07-53-mozilla-central.app
Fails: 2011-08-04-03-07-32-mozilla-central.app
and still on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a1) Gecko/20110818 Firefox/9.0a1

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3735fb1cd5ef&tochange=be4b064f1159

Bug 674861 is the only obviously related item I could find, although I understand it was supposed to change the behaviour in a different case.

(When I initially tried to reproduce on a clean Firefox profile, the issue was reproducible with one account that uses the Gmail "Preview (dense)" theme and was not reproducible with another account. Now I can reproduce with any account.)
Comment 1 Fabien Cazenave [:kaze] 2011-08-19 03:27:57 PDT
Hmm, I hope it’s not a regression caused by my patch on bug 674861. Looking at it.
Comment 2 Fabien Cazenave [:kaze] 2011-08-19 03:37:39 PDT
Created attachment 554354 [details]
testcase
Comment 3 Fabien Cazenave [:kaze] 2011-08-19 04:17:16 PDT
Found the regression, rushing to submit a patch today so it can be shipped with Firefox 8.

Thanks for your report!
Comment 4 Fabien Cazenave [:kaze] 2011-08-19 06:34:44 PDT
Created attachment 554393 [details] [diff] [review]
patch proposal

Fixes the regression introduced in bug 674861.
New tests have been added to make sure that lists are properly splitted (i.e. that a paragraph is created at the right place) when allowed.

Ehsan, is it still time to get this patch in Firefox 8?
Comment 5 Fabien Cazenave [:kaze] 2011-08-19 08:30:31 PDT
http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=31d671f9f0f7
Comment 6 Fabien Cazenave [:kaze] 2011-08-19 09:59:47 PDT
Ugh, tests failing. Looking at it. :-(
Comment 7 :Ehsan Akhgari 2011-08-19 12:24:38 PDT
Comment on attachment 554393 [details] [diff] [review]
patch proposal

Can you please re-request the review when you fix the test failures?
Comment 8 :Ehsan Akhgari 2011-08-19 12:25:44 PDT
(In reply to Fabien Cazenave (:kazé) from comment #4)
> Ehsan, is it still time to get this patch in Firefox 8?

I would really like to fix this in Firefox 8 by backing out bug 674861.  Can you please attach a patch which reverts bug 674861 on mozilla-aurora and ask approval-aurora on it?
Comment 9 Fabien Cazenave [:kaze] 2011-08-19 14:34:33 PDT
Created attachment 554553 [details] [diff] [review]
patch proposal

unit tests now work properly
Comment 10 Fabien Cazenave [:kaze] 2011-08-19 14:54:43 PDT
Created attachment 554558 [details] [diff] [review]
backout - bug674861

For Firefox 8, the two following patches should be applied in this order:
 • attachment 550096 [details] [diff] [review]attachment 554553 [details] [diff] [review] 

Or, I can make a single patch instead of these two ones. Let me know.
Comment 11 :Ehsan Akhgari 2011-08-19 15:10:31 PDT
(In reply to Fabien Cazenave (:kazé) from comment #10)
> Created attachment 554558 [details] [diff] [review]
> backout - bug674861
> 
> For Firefox 8, the two following patches should be applied in this order:
>  • attachment 550096 [details] [diff] [review]
>  • attachment 554553 [details] [diff] [review] 
> 
> Or, I can make a single patch instead of these two ones. Let me know.

No, this should be fine.  Thanks!
Comment 12 :Ehsan Akhgari 2011-08-19 15:14:19 PDT
Pushed to inbound.
Comment 14 Nickolay_Ponomarev 2011-08-22 13:20:14 PDT
Thanks! Fixed for me as well on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a1) Gecko/20110822 Firefox/9.0a1
Comment 15 Christopher Blizzard (:blizzard) 2011-08-23 14:57:05 PDT
Comment on attachment 554558 [details] [diff] [review]
backout - bug674861

Approved for Aurora (Update 8.)  Please land this as soon as possible, thanks!
Comment 16 :Ehsan Akhgari 2011-09-12 13:23:02 PDT
Bug 674861 backed out from Aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/b71f847ed644
Comment 17 christian 2011-10-25 22:19:50 PDT
---------------------------------[ Triage Comment ]---------------------------------

Even though this is fixed we'll track it to make sure it doesn't bounce.

Note You need to log in before you can comment on or make changes to this bug.