Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Description • 11 years ago

STR
 (1) Compose email
 (2) Type long body, many lines.  Let's say 10.
 (3) Unfocus textarea

It looks like a bunch of the message has been deleted, when in actuality it's just clipped out by a smaller-than-available-space textarea.  Very surprising and disturbing at first.

b? -> radar

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 1 • 11 years ago

Hrm.  Scroll lines have disappeared when hitting return.  If the indicators were there I think it would be a little less disturbing.  No lines are actually removed from the content of the email.

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/361d9359f4f3
Gaia   0abc95774d0bbdfe314fa588e09fc92cac3e6427
BuildID 20130130113326
Version 18.0
Unagi

	Andrew Sutherland [:asuth] (he/him)
	Comment 2 • 11 years ago

From https://bugzilla.mozilla.org/show_bug.cgi?id=798695#c2:
"I just realized that my quick hack with setting the height of the area only counts newlines, and does not compensate for line-wrapping.  I'm not sure if we can use scrollHeight in a textarea and just set the CSS height to deal with things, but that is quite possibly the better solution."

That bug was sortof about something else so it got fixed, but scrollHeight is the right call.  We just need to do that on 'input' events and we will suck less.  I'll flag myself to follow up on this; others should feel free to steal if I don't get to it first.

	Andrew Overholt [:overholt]
	Comment 3 • 11 years ago

Andrew, do you think this is a recent regression?  Bug 798695 is pretty old.

	Alex Keybl [:akeybl]
	Comment 4 • 11 years ago

We're down to only the most critical bugs at this point. This is not a blocker, given that criteria (no real data loss).

	Andrew Sutherland [:asuth] (he/him)
	Comment 5 • 11 years ago

(In reply to Andrew Overholt [:overholt] from comment #3)
> Andrew, do you think this is a recent regression?  Bug 798695 is pretty old.

Not really a regression.  The size of the textarea has been broken a long time; I think we did intentionally change styling per instructions from VD that has probably made this suck more by making the bounds of the container invisible.  There may have been a platform regression in terms of the textarea's scrollbars, but I couldn't really say.

	Andrew Sutherland [:asuth] (he/him)
	Comment 6 • 11 years ago

It turns out input_areas.css had a "max-height: 10rem" in there that had massively regressed us. since that overrode even my insufficient "rows" setting logic.

I've tested this fix on Firefox nightly and on-device.  I can type stuff and the box keeps going and I can scroll to see everything.

We do a very naive scrollHeight propagation; assuming the underlying logic is smart about when it dirties its scrollHeight and the style.height setter is smart about ignoring redundant writes, we are good.  If either of those are not true, this may generate bad load.  However, because of the text-wrapping thing, there isn't a tremendously easy solution for us.  If we had XUL's onoverflow or something like that, we could maybe be less dumb.

	Jim Porter (:squib)
	Comment 7 • 11 years ago

There is an overflow event for HTML (but only in Firefox; Webkit has a similar event called overflowchanged). I don't think it's standardized, but maybe we could use it and fall back to a less-efficient method, assuming we even care about making this work on not-Firefox.

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 8 • 11 years ago

2012-11-14 09:31:34 was when Bug 798695 was fixed.

** 2012_12_27_23 shows a scroll bar when composing more than 10 lines in hotmail/gmail
Mercurial-Information: 
"releases/mozilla-b2g18" revision="f58409e312c8"
"integration/gaia-nightly" path="gaia" revision="dc794f5ee13e"
Github:
"releases/gecko.git" revision="1975ab93b55bf07844f58dff1d35e2087b00d1ea"
"releases/gaia.git" revision="73270c32928f070e49b265ea75ff4062ddb82d67"

** 2012_12_28_07 does not show the scroll bar.
Mercurial-Information: 
"releases/mozilla-b2g18" revision="f58409e312c8"
"integration/gaia-nightly" revision="23144a961998"
Github:
"releases/gecko.git" revision="1975ab93b55bf07844f58dff1d35e2087b00d1ea"
"releases/gaia.git" revision="05b802ed09f3459899fd2210e3472c9afbd1a07b"

	Andrew Sutherland [:asuth] (he/him)
	Comment 9 • 11 years ago

Comment on attachment 712593 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/8051

(I need to do another rev of the patch given the feedback; trying a new flag thing)

	Andrew Sutherland [:asuth] (he/him)
	Comment 10 • 11 years ago

If someone else wants to take over the patch to try out the overflow event, that would be ideal.

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 11 • 11 years ago

noming for reconsideration for v1.1; this was set - for tef?  

There is no scroll bar for the body section of a composed email, which may confuse end users.

	Alex Keybl [:akeybl]
	Comment 12 • 11 years ago

Please don't renominate bugs without new information. This is already tracking.

	Kevin Grandon :kgrandon Assignee
	Comment 13 • 11 years ago

This is an alternative approach at forcing the textarea to take up the remaining space on the screen. (It will always be a minimum of 10rem, but will grow to fill up whatever is not taken by the address rows).

This is mainly a pure CSS implementation.

	Kevin Grandon :kgrandon Assignee
	Comment 14 • 11 years ago

Comment on attachment 733676 [details]
Pull request - alternative approach

><html>
><head>
><meta http-equiv="Refresh" content="1;url=https://github.com/mozilla-b2g/gaia/pull/9000">
></head>
><body>
><a href="https://github.com/mozilla-b2g/gaia/pull/9000">Redirecting to github.</a>
></body>
></html>

	Kevin Grandon :kgrandon Assignee
	Comment 16 • 11 years ago

I have been pretty busy lately. Should I free up, I'll try to take another stab at this one. Otherwise, anyone is welcome to snag it in the meantime.

	Dylan Oliver [:doliver]
	Comment 17 • 11 years ago

Triage: This bug has been added to the v1.2 backlog but is not blocking release

	Andrew Sutherland [:asuth] (he/him)
	Comment 18 • 11 years ago

Comment on attachment 712593 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/8051

(the feedback flag was just for my radar purposes; since this is in our pivotal tracker instance for visibility, it's not needed anymore)

	Marcus Cavanaugh [:mcav]
	Comment 19 • 11 years ago

After rebasing Andrew's patch (since there was a file reorg in the intervening time), his patch seems to address this properly. :jimporter reported issues with the patch's behavior in b2g-desktop; while I haven't tested that yet, it seems to work fine for me on device. I uploaded a new PR just for testing convenience, but it's identical to asuth's patch.

If I can get b2g-desktop running, I'll report back here with my results.

	Marcus Cavanaugh [:mcav]
	Comment 20 • 11 years ago

Comment on attachment 806289 [details]
Rebased PR (same as asuth's attachment)

I can't replicate the exact scenario in b2g-desktop due to a keyboard error (this.__DOM_IMPL__ is undefined), but since it appears to work on-device and in nightly, I'm marking for review anyway. Andrew, if you have any luck with b2g-desktop, let me know. I may have something wrong with my setup, but I've tried both the prebuilt B2G.app as well as one from mozilla-central.

	Andrew Sutherland [:asuth] (he/him)
	Comment 21 • 11 years ago

Comment on attachment 806289 [details]
Rebased PR (same as asuth's attachment)

This still works in the base case, but we get bit by a bug in Firefox OS's form-handling related to textareas larger than the display area remaining after the keyboard has gobbled up most of the display.  This happens when replying to a message, for example.

I filed bug 942749 on this; see it for more details and an initial attempt at a fix that was somewhat stymied by two different self-built b2g-desktop (for symbols) crashing at startup during JS GC's.

We could probably hack around that bug by manually fighting forms.js for scrolling, but that's inadvisable; we should fix that bug first and then we can check this again and land it.

	Kevin Grandon :kgrandon Assignee
	Comment 23 • 11 years ago

Saw this got unassigned. I haven't ran any tests yet, but going to try this out.

	Kevin Grandon :kgrandon Assignee
	Comment 24 • 11 years ago

Assigning to myself again if no one has objections.

	Andrew Sutherland [:asuth] (he/him)
	Comment 25 • 11 years ago

(In reply to Kevin Grandon :kgrandon from comment #24)
> Assigning to myself again if no one has objections.

Glad to have you look into it!  Just keep in mind that because of the scrollIntoView system issue on bug 942749 (which afflicts contenteditable as well), fixing this bug without fixing that bug actually makes things worse in general.  So if you could please fix that one too ;)

	Andrew Sutherland [:asuth] (he/him)
	Comment 26 • 11 years ago

I ran with this on a self-built b2g-desktop with your scroll fix patch and with "white-space: pre-wrap;" added to .cmp-body-text (essential!) and this looks pretty good.

The big question to answer is what we want to do about long lines.  With just that one change, a lone line like one containing a full git hash exceeds the available line length[1] causes the entire page to be able to horizontally scroll.  It might make sense to wrap the contenteditable so that it scrolls horizontally in its own box but is still operating in the page's vertical scrolling context.  I was able to wrap cmp-body-text in a div with just "overflow-x: auto;" and that did make things seem a little less stupid since it avoided having all the compose headers sliding off the screen with the body.

This will likely need a JS marionette test to ensure that we roundtrip the newlines through local drafts correctly.  The best case is probably: reply to a message, sanity check state, type some stuff in with some long lines and some newlines, verify, save draft, load draft, verify.  Maybe wait a few days for this bit since I have outstanding marionette cleanup work.

1: at least on Firefox nightly on desktop; for CSS play I had to switch to it because b2g-desktop only wants to work with a profile once and then I have to nuke it.  Not sure what's up with that.  I think linux b2g-desktop also regressed badly for me on my current crappy ATI drivers, so that also was not fun.

	Kevin Grandon :kgrandon Assignee
	Comment 27 • 11 years ago

Comment on attachment 8345040 [details] [review]
Github pull request

Hi Andrew - I'm feeling ok about this patch now. I was a little concerned with the HTML escaping/unescaping, but I couldn't find any problems with the implementation - perhaps you can :)

	Kevin Grandon :kgrandon Assignee
	Comment 28 • 11 years ago

Oops - I did not investigate the long-line issue. I will do so now.

	Kevin Grandon :kgrandon Assignee
	Comment 29 • 11 years ago

(In reply to Kevin Grandon :kgrandon from comment #28)
> Oops - I did not investigate the long-line issue. I will do so now.

Ok - this has been addressed now with some overflow css properties on the body text div.

	Andrew Sutherland [:asuth] (he/him)
	Comment 30 • 11 years ago

Comment on attachment 8345040 [details] [review]
Github pull request

Almost there!  Thanks very much for adding the integration test and dealing with the horizontal scroll issue.  I think we'll be good to go with the below fixed:

- Our security hygiene policy with innerHTML in e-mail that we reached with the security team/:freddyb is that we avoid innerHTML unless we have to use it, and that we use our bleach.js library when we have to.  The wisdom of this is arguably supported by your patch having a typo and transforming single quotes to " instead of the intended entity ;)

It's fairly straightforward for us to use document.createTextNode/document.createElement('br') in alternation from a scan or split on \n.  (And then invert it by walking text nodes and br elements.)  So I think we should do that.

- Somewhat related to the former, right now if you type multiple spaces in the compose area in succession since we haven't set "white-space: pre-wrap;", contenteditable creates   entities for us.  So if you type "   foo" and send the e-mail, the recipient gets "   foo" in their e-mail  (According to bug 592592, we shouldn't insert   when "white-space" is pre-wrap.)

It seems like generating an assertion on this needs us to trigger ComposeCard._saveStateToComposer() and then verify the value of composeCardInstance.composer.body.text or something like that.  Alternately we could cause the message to be sent to the fake-server, receive it, then check that the div we cram it in matches our expected string value.  (We use pre-wrap there so white-space is significant and we just have newlines.)

We definitely want an integration or unit test to cover this.  (Note that we do not want to be producing \u00a0 characters in our output either!)

- At least on Firefox nightly (Dec 16 linux x64 running your gaia branch with a DEBUG=1 build), when I type the first character on the last line (which is empty before I type the character) when there are at least 5 populated lines, everything shifts up slightly.  It's like the height of that last line does not include the required space below the baseline, so once something gets typed, the layout engine realizes it needs a little more space.

To reproduce this, you should be able to type 1/2/3/4/5 each on their own line.  Then go to type 6 on the last line.  Jumpytown/

- It's not clear why you are replacing use of sendKeys with direct innerHTML clobbering.  http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-sendkeys.js seems to go to a tremendous amount of work to generate actual keypress events that should net us the desired effect (and cover the multiple spaces case too), so it seems better to get that realism in our test.  Please switch back to sendKeys and having the

	Andrew Sutherland [:asuth] (he/him)
	Comment 31 • 11 years ago

(In reply to Andrew Sutherland (:asuth) from comment #30)
> Please switch back to sendKeys and having the

Completed: Please switch back to sendKeys and having the string expectation/etc. exist in the plaintext for line 'line1\nline2'.  More specifically, we want the string value to be what the format-flowed interpretation of the message body is.  (So if you had a string with a line longer than 72/76/80 characters, even though the e-mail in RFC2822 form would be wrapped, the we would not have the wrapping in any of our typing/checking constants.)

	Kevin Grandon :kgrandon Assignee
	Comment 32 • 11 years ago

Awesome review, thanks! I tried sendKeys, but for some reason I was having problems with contenteditable and innerHTML seemed to quickly solve the issue. I will try sendKeys again and investigate.

I am at a work week this week, then we have off for the holidays. I don't want to leave this bug sitting for too long, so if anyone wants to pick up the patch and run with it, feel free to. Otherwise I will try to grab this in early 2014.

	Kevin Grandon :kgrandon Assignee
	Comment 33 • 10 years ago

Comment on attachment 8345040 [details] [review]
Github pull request

Updated the pull request, and here's where I'm at with this:

- Added the white-space definition to prevent conversion to  . Updated the test in the local_draft_tests.js file to reflect this, and am using it for more general-purpose format preservation testing. Let me know if you want me to move this, or capture any additional cases. I'm also not entirely sure of how to compare it to a plaintext result like you asked for in Comment 31.

- I was not able to reproduce jumpiness issue on the 6th line - can you verify on a device? It sounds like a gecko bug, but I'm wondering if this is related to fonts installed on the system? I've currently manually installed the B2G fonts on my system, and I think the relevant font is Feura Sans. Do you have the proper fonts installed?

- I have reverted to using sendKeys in the test. I was having trouble using it with contenteditable elements, but I forgot I had to click() first =|

- Changed from custom HTML escaping to DOM node iteration. I haven't messed with text content like this in a while - so there is probably something wrong here. Let me know if you see something I missed and I'll add a test for it.

	Kevin Grandon :kgrandon Assignee
	Comment 35 • 10 years ago

Any chance we can prioritize this for 1.4? The current usability of the email compose area leaves a lot to be desired.

	Kevin Grandon :kgrandon Assignee
	Comment 36 • 10 years ago

Review ping? Had to rebase this today, and would rather avoid having to spend time doing that. Any chance we could move forward on this, or find someone else to review? If something is wrong and it'd be faster for you to steal and finish, that'd be fine too.

	Andrew Sutherland [:asuth] (he/him)
	Comment 37 • 10 years ago

Review on this is now imminent; deepest apologies.  Especially once we moved this to the 1.4 feature bucket it got deferred a little since it depended on the marionette test bustage being fixed (which :gaye did, hooray!) but following that work-week I was on vacation for a week and then lost power for ~3 days in the great PA snow/ice storm once I recovered from jet-lag.  I'll steal as appropriate.

	Andrew Sutherland [:asuth] (he/him)
	Comment 38 • 10 years ago

About to land after verifying Marionette is happy.  It includes some minor fixes/cleanups but nothing substantiative.  Just wanted to note what the mystery turned out to be:

(In reply to Kevin Grandon :kgrandon from comment #33)
> - I was not able to reproduce jumpiness issue on the 6th line - can you
> verify on a device? It sounds like a gecko bug, but I'm wondering if this is
> related to fonts installed on the system? I've currently manually installed
> the B2G fonts on my system, and I think the relevant font is Feura Sans. Do
> you have the proper fonts installed?

This just reproduced everywhere (nightly, b2g-desktop, device) for me, although obviously things can change in 6 weeks ;).  It turns out it was a result of "font-size: 1.7rem;" interacting with "line-height: 1.9rem;"

	Kevin Grandon :kgrandon Assignee
	Comment 39 • 10 years ago

(In reply to Andrew Sutherland (:asuth) from comment #38)
> About to land after verifying Marionette is happy.  It includes some minor
> fixes/cleanups but nothing substantiative.  Just wanted to note what the
> mystery turned out to be:
> 
> (In reply to Kevin Grandon :kgrandon from comment #33)
> > - I was not able to reproduce jumpiness issue on the 6th line - can you
> > verify on a device? It sounds like a gecko bug, but I'm wondering if this is
> > related to fonts installed on the system? I've currently manually installed
> > the B2G fonts on my system, and I think the relevant font is Feura Sans. Do
> > you have the proper fonts installed?
> 
> This just reproduced everywhere (nightly, b2g-desktop, device) for me,
> although obviously things can change in 6 weeks ;).  It turns out it was a
> result of "font-size: 1.7rem;" interacting with "line-height: 1.9rem;"

Nice find. Do you want me to update anything in the PR?

	Kevin Grandon :kgrandon Assignee
	Comment 40 • 10 years ago

Also I just rebased the PR and got a green build.

	Andrew Sutherland [:asuth] (he/him)
	Comment 41 • 10 years ago

landed on gaia/master:
https://github.com/mozilla-b2g/gaia/pull/16345
https://github.com/mozilla-b2g/gaia/commit/0697063c6546d20b0873ac414b212be64b692784

Thus ends the great saga of the absurdly unusable composition UI!  Thank you Kevin!


The review fixes ended up folding in more slipstream fixes than I really wanted, but I did a lot of manual testing on this (probably more than we've done in quite some time based on how many glitches I found/had to fix) so I'm okay with this.  Here's the inventory:

- Filter out the "bogus" <br type="_moz"> being inserted by the editing infrastructure once any newlines were inserted.

- Change HTML generation to avoid inserting extra '\n' at the end.

- Change marionette test to use the compose card's JS logic for creating a string from the state of the composer rather than trying to look at the innerHTML/textContent.

- Fix the canForward issue (bug 943776), disabling the unit test that depended on this broken behaviour, and leaving bug 943776 to fix that once we can properly inject synthetic messages.

- Fix a canReplyAll bug that was uncovered once the above error was corrected.

- Remove min-height on the compose text editor area to address a visual problem when replying to an HTML message.  (Excess whitespace resulted because we required the text area to be 10 rem high).  This then required adding magical clicking logic to the HTML area and the container body based on clickY versus the position of the contenteditable area so that clicking could still result in positioning the cursor.  Attachments show up above the contenteditable so there are no horrible edge cases there.  I also changed the margins while in there.

- Workaround a bug in the iframe-shim code that caused explosions when creating a compose card with HTML contents without a message_reader card on the stack.  This is because of the horrible hacks for coordinate space compensation for zooming/pinching in the message_reader mode that I really shouldn't have r+'d.  I moved things around to stop the explosions, but the evil is still there.

- The line-height/font-height thing already mentioned.

	Kevin Grandon :kgrandon Assignee
	Comment 42 • 10 years ago

asuth+++++ thanks for finishing this/landing this! I also would've been totally fine with you taking the commit authorship - as you probably wrote more of the final commit than I did :)

Anyway - I'm stoked that we'll be able to properly compose emails now. The first patch on this bug was 1+ year ago, glad we've finally got it figured out :)