308875 - 'space bar' message reading scrolling broken for formatted messages (with remote images)

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Chris Andrichak]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en) AppleWebKit/412.7 (KHTML, like Gecko) Safari/412.5
Build Identifier: version 1.5 Beta 1 (20050908)

Normally (and forever previously) one was able to select a message and hit 'space bar' to page through it's 
contents. Wth 1.5b1 and HTML (at least not seemingly unformatted msgs) hitting 'space' forwards a page, 
and then immediately shows the 'message loading...' status at bottom and then reverts back to the 
begginging of the message. Repeat endlessly.

Reproducible: Always



Expected Results:  
Normally hitting 'space bar' (regardless of focus) results in the messge contents paging down, and to the 
next msg if needed.

Comment 1

[Chris Andrichak]

probably not related (?): 253682

Comment 2

[Chris Andrichak]

Thunderbird seems to be better behaved with respect to this problem in 1.5 beta 2 - I can scroll through 
more e-mails with this space bar. Not completely fixed though.

Comment 3

[Chris Andrichak]

Attachment: Sample e-mail message that has the problem

text attachment is the source of an e-mail that has the space bar scrolling
problem. Maybe the problem is with multi-part mime messages?

Comment 4

[Chris Andrichak]

version 1.5 (20051025) (rc1? 2?)
This problem still exists although it seems to be limited to this situation - the message contains some formatting, and the problem resolves itself when you click in the message pane or read another message and come back to this message. Only on the initial reading of the message is the space bar paging broken, with the "message loading..." showing as it snaps back to the top of the message.

Comment 5

[Dave L]

I have seen the same behavior in SeaMonkey (including the current version (1.1.7)).  I initially thought it was somehow a theme problem, but it occurs with every theme I try.  

My experience is that the problem is linked to HTML mail when retrieving images from the web; I don't see it with plain text email.  I have SeaMonkey set to "Block images and other content from remote sources."  If I open an HTML message with remote content, space bar paging works fine before I approve retrieval of the remote images.  Once I've clicked the button to approve retrieval, hitting the space bar when looking at the start of a message does a single page down then immediately jumps back to the top of the message, over and over each time I hit the space bar; the page up and down keys, however, page through the message normally while remote content is being retrieve.  The space bar behavior of jumping back to the top continues even after the image retrieval appears to be done.  If I then close and re-open the same message , now with the images already retrieved, space bar paging works fine again.  I've verified this behavior with different messages from different sources and the behavior was consistent.

Comment 6

[Dave L]

Two additions to my comment of 2007-12-12:

1) Using SeaMonkey on MS Windows XP Pro, SP2.

2) I've since realized that I don't need to close and re-open the message to fix the scrolling problem, I only need to click to another window and then back to the message.  Once the remote content download is complete, unfocusing and refocusing the message windows allows space bar paging to work.

Comment 7

[Wayne Mery (:wsmwk)]

Chris, do you still see this problem?

I used to see something like this, but not for a long time.
testcase attachment 199205 [details] WFM on Vista, but remote content is not displaying
version 2.0.0.13pre (20080227)
version 3.0a1 (2008050715)

Comment 8

[Chris Andrichak]

with 2.0.14pre I still see this problem. I would probably agree with Dave L in #5 that it seems to be related to external images in messages. 

The easiest way to remedy the problem while reading a message is to just click (give focus to?) anywhere in the message reading pane - then you can space bar through the message again. Maybe this is a focus problem?

Comment 9

[Wayne Mery (:wsmwk)]

Chris, Dave do you see this with current trunk build?

Comment 10

[Chris Andrichak]

yes, still seeing it with the nightlies. Will have to check if the circumstances are still the same. Seems to happen less though.

Comment 11

[Chris Andrichak]

Using the Shredder alpha 3 test builds, this problem seems to be fixed. Might be able to WFM...

Comment 12

[Chris Andrichak]

Attachment: email with problem

spoke too soon - attached is email with problem.

Happens much less often, though.

Comment 14

[Wayne Mery (:wsmwk)]

do you have more examples?

this makes for rather bad and inconsistent user experience. Surprised I'm not finding other bugs reporting this, but maybe they are "hidden" under poor summaries or people are just living with the pain and not reporting it.

Comment 6 seems to be true for me.

Comment 15

[Magnus Melin [:mkmelin]]

I can easily reproduce this on linux, with a newsletter. Seems it must contain remote pictures, and you have to click allow pictures. wanted3+

Comment 16

[David :Bienvenu]

this is highly highly annoying

Comment 17

[Phil Ringnalda (:philor)]

In the long run, all clicking the button does is gDBView.reloadMessage(), like a billion other things do.

Is focus still on the button, so that the described steps are actually "click the button to reload the message with images, then use the space bar to activate the still-present and still-focused button to reload the message with images again and again and again"?

Comment 18

[David :Bienvenu]

> Is focus still on the button, so that the described steps are actually "click
> the button to reload the message with images, then use the space bar to
> activate the still-present and still-focused button to reload the message with
> images again and again and again"?

You may very well be right - the button is not visible once the message is reloaded, but clicking in the message pane makes the problem go away. However, pressing the space bar does scroll the message down, so it's not that the button is completely eating the space.

Comment 19

[ovidiu]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b5pre) Gecko/20090512 Shredder/3.0b3pre

*can repro with newsletter with remote images
*after hit load images, space does go down and up again, and again
*any click (l,r,m) in msg pane solves it

Comment 20

[Jan Horak [:jhorak]]

Attachment: Patch v1: spacebar scroll fix

Simple fix which focus message browser (id: messagepane) after content is reloaded with images.

Comment 21

[Hadar Pedhazur]

From looking at the patch, I think it would work. I can see that the file to be patched lives in ./chrome/messenger.jar in my Beta 2 build.

Is there a documented way for me to patch that much larger file?

Or, if this comment can be updated whenever there is a nightly that has the fix in it, I'll be delighted to test.

Thanks jhorak, this has been an annoyance for years!

Comment 22

[Phil Ringnalda (:philor)]

Comment on attachment 385769 [details] [diff] [review]
Patch v1: spacebar scroll fix 

Sorry for the delay: touching focus always ends in tears, so I tend to hope aliens will destroy the world before I get to it.

("messagepane").focus() definitely isn't where we want it: that focuses the chrome element, so when you hit tab you go back around to the top, quicksearch if it's on the toolbar, rather than going into the message itself. To focus the message itself, you want |window.content.focus()|, but let's have Bryan give a ui-r on whether we want that, or to do what we do for message header button clicks, call RestoreFocusAfterHdrButton() which focuses content in the standalone window, and wants to set focus back to where it was, but just focuses the threadpane because it doesn't know any better, in the 3pane.

Comment 24

[Hadar Pedhazur]

Since someone just posted, I'll just note that in 3.0 Final, this has _not_ been fixed. :-(

Comment 25

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 385769 [details] [diff] [review]
Patch v1: spacebar scroll fix 

Sorry, thought I'd give those aliens a little more time but alas we're all still here.

I believe 
window.content.focus() is what we want.  This seems like a more focused (keyboard) action than the header button code so I think it follows that we want the focus to go to the content for possible tabbing.  ui-r+ with that.

Comment 26

[Chris Andrichak]

(In reply to comment #25)
> window.content.focus() is what we want.  This seems like a more focused
> (keyboard) action than the header button code so I think it follows that we
> want the focus to go to the content for possible tabbing.  ui-r+ with that.

Using this line instead of the one in the patch (and putting in the right place) this seems to work.

I say seems since this is getting hard to test - with 3.1rc2 I see it happen, but it's hard to get it to repeat with the same email, as if the second viewing in the same session fixes the problem. So if I see it in 3.1rc2 and then immediately quit and reopen in a patched trunk build, it doesn't happen.

Otherwise the patch doesn't seem to cause any problems.

Comment 27

[Chris Andrichak]

Attachment: updated diff using code from comment #25

Comment 28

[Mark Banner (:standard8)]

(In reply to comment #26)
> I say seems since this is getting hard to test - with 3.1rc2 I see it happen,
> but it's hard to get it to repeat with the same email, as if the second viewing
> in the same session fixes the problem. So if I see it in 3.1rc2 and then
> immediately quit and reopen in a patched trunk build, it doesn't happen.

To be clear: I believe you mean 3.1rc1 *build 2*. 3.1rc2 hasn't been generated at all yet.

Comment 29

[Chris Andrichak]

(In reply to comment #28)
> > I say seems since this is getting hard to test - with 3.1rc2 I see it happen,
> 
> To be clear: I believe you mean 3.1rc1 *build 2*. 3.1rc2 hasn't been generated
> at all yet.

Yes - 3.1rc1 build2

Comment 30

[Chris Andrichak]

I'm going to say that the patch fixes things for me. Didn't see it at all in my test build. See this now in 3.1, switch to my test build, don't see it. Seems like a simple enough fix that shouldn't regress anything...

Comment 31

[Seungbeom Kim]

I still see the problem in Thunderbird 3.1. Hasn't it been committed into trunk?

Comment 32

[Jan Horak [:jhorak]]

(In reply to comment #30)
> I'm going to say that the patch fixes things for me. Didn't see it at all in my
> test build. See this now in 3.1, switch to my test build, don't see it. Seems
> like a simple enough fix that shouldn't regress anything...
You need to ask for review from philringnalda first to commit into trunk.

Comment 33

[Ludovic Hirlimann [:Usul]]

(In reply to comment #32)
> (In reply to comment #30)
> > I'm going to say that the patch fixes things for me. Didn't see it at all in my
> > test build. See this now in 3.1, switch to my test build, don't see it. Seems
> > like a simple enough fix that shouldn't regress anything...
> You need to ask for review from philringnalda first to commit into trunk.

Maybe setting the flag would help getting the review :-)

Comment 34

[Chris Andrichak]

Still seeing this bug. I know the diff is a bit out of date, but it's only 1 line of code... Any chance on getting this committed?

Comment 35

[Ludovic Hirlimann [:Usul]]

Comment on attachment 447671 [details] [diff] [review]
updated diff using code from comment #25

Carrying the ui-review, requesting review from philor

Comment 36

[Ludovic Hirlimann [:Usul]]

(In reply to comment #34)
> Still seeing this bug. I know the diff is a bit out of date, but it's only 1
> line of code... Any chance on getting this committed?

Needs to get r+ first , just asked for that. Any chances of getting a unit test for this ?

Comment 37

[Magnus Melin [:mkmelin]]

Comment on attachment 447671 [details] [diff] [review]
updated diff using code from comment #25

This still applies

Comment 38

[Mark Banner (:standard8)]

Comment on attachment 447671 [details] [diff] [review]
updated diff using code from comment #25

Thanks for updating the patch Chris.

It looks good and seems to work fine. r=Standard8.

I've also landed this on trunk for you, so it'll be in tomorrow's 3.3 nightly builds:

http://hg.mozilla.org/comm-central/rev/3076c32865d7

Comment 39

[Mark Banner (:standard8)]

(and thanks to jhorak for the original patch)