547654 - rendering problems when mouse scrolling email messages when Lightning addon is installed

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Adil Allawi]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a2pre) Gecko/20100221 Minefield/3.7a2pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.2pre) Gecko/20100221 Lightning/1.0b2pre Lanikai/3.1b1pre ID:20100221061116

I am using latest nightly Thunderbird 3.1 beta with Lightning 1.0 beta. When I scroll an email with the mouse wheel (two-finger scroll on my MacBook I see lines appearing in the scrolled part of the email.

If I scroll by dragging the scroll bar, the display problem does not happen.
If I disable Lightning then the display problem does not happen.

Reproducible: Always

Steps to Reproduce:
1. View a message in a new tab or in the message panel below the list of emails.
2. Scroll with the mouse (using two fingers on the trackpad in my case).
3. Look at the scrolled part of the email.
Actual Results:  
Lines appear in the scrolled part of the email.

Expected Results:  
normal display

Comment 1

[Adil Allawi]

Attachment: View of scrolled email message showing the display problem

Comment 2

[Nathan Tuggy (:tuggyne)]

Moving tentatively to Calendar/Lightning for more triage/troubleshooting.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

Not sure why this is happening. You say you are using Thunderbird 3.1 beta with Lightning 1.0b1? This shouldn't be possible.

Please switch to Lightning 1.0b2pre

Comment 4

[Adil Allawi]

I switched to latest nightly lightning 1.0b2pre and Lanikai 3.1b2pre. Problem still happens but less frequently. 

I hacked around with chromebug and found that if I remove the XUL markup for the "Today Pane" button in the status bar the problem goes away. The only difference that I notice is that the "Today Pane" button makes the status bar two pixels deeper.

Specifically, I deleted this markup:

<statusbarpanel id="calendar-show-todaypane-panel" pack="center"><toolbarbutton id="calendar-status-todaypane-button" doubleimage="true" type="checkbox" label="Today Pane" tooltiptext="Show Today pane" observes="calendar_toggle_todaypane_command" command="calendar_toggle_todaypane_command" oncommand="TodayPane.toggleVisibility(event)" checkState="0" checked="false"></toolbarbutton></statusbarpanel>

For more information 
- my system is 10.6 and the computer is a 17" MacBook Pro. This problem only happens when scrolling down using the mouse wheel or two-finger trackpad scroll. There is no problem when scrolling up. Also this only happens when scrolling an email in the message panel of the main mail window. it does not happen when I view an email in a new window.

I hope this information helps.

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

This might be a mac specific issue, I can't reproduce this on Linux.

Martin or Daniel, can you confirm?

Comment 6

[Adil Allawi]

Found the source of this issue. In line 133 of lightning.css #calendar-status-todaypane-button, #calendar-status-todaypane-button[checked="true"] are defined to have a vertical padding of 0.2pt. This raises the status but up by a fraction of a pixel. I think it is this fraction of pixel that causes the rendering errors. Redefining the vertical padding to zero or a round number of pixels fixes the problem.

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Adil, do you think you could create a patch that fixes the issue for pinstripe and winstripe? See https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_diff_and_patch_files.3F

Our repository url is http://hg.mozilla.org/comm-central

Comment 8

[Adil Allawi]

Attachment: Fixing padding and margin of Today Pane button in Status bar

Removed the fractional width from the Today Pane button css in Winstripe and Pinstripe themes as it causes smooth scolling issues on the Mac. This width was originally introduced in bug 456362 based on the look of the button in Linux. On the Mac there is no need for any padding while Windows needs a 1px padding. And both Mac and Windows themes need a 2px top margin to stop the button from hitting the top of the status bar. So it is possible there will be the need to add a Linux-specific version of this css without the margin if this causes issues in Linux.

Comment 9

[Adil Allawi]

Attachment: Screenshot of padding and margin in Windows and Mac with the patch

Comment 10

[Adil Allawi]

Attachment: Fix padding and margin of Today Pane button in Status bar

Updated patch - previous patch had a typo (oops).

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 434183 [details] [diff] [review]
Fix padding and margin of Today Pane button in Status bar

Adil, thanks for preparing a patch! Sorry for not getting back to you on IRC, was quite busy. Feel free to ping me again or email if you have further questions.

I'll take a look at this soon and tell you how it looks on linux. If there is any trouble, we can solve this after bug 471378 is fixed.

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Linux Screenshot

On Linux the button is very close to the bottom of the statusbar. How does windows/mac look if you decrease the top margin to 1px? That should make it look sufficiently centered on linux. You could also experiment with the xul attributes, i.e adding either align=center or pack=center to the parent box and removing the margin. This should vertically center the toolbarbutton.

Comment 13

[Adil Allawi]

Attachment: Added 1px to the bottom of the margin.

Who would have thought that positioning a button would be so finicky :) Decreasing the top margin would not work as it would hit the top edge of the status bar on Windows and Mac, but squeezing in a pixel at the bottom should work for all three platforms. Please try the attached patch.

BTW: this is only the beginning of the issues with this button. The Mac UI for the button has no proper icons created for it and references non existant images. I commented on bug 502095 with these problems as it seems the best place to track them.

Comment 15

[Mark Banner (:standard8)]

Phillip: this is a serious issue, especially on mac, as it makes it hard to read emails when scrolling. I think it needs to block the version of Lightning that comes out alongside TB 3.1...

Note that from bug 540466, we think this is the regression bug, though I'm not sure if this will help much:

> http://hg.mozilla.org/mozilla-central/rev/ff084019260e looks like a pretty
> likely cause to me:
> 
> Bug 352093. Part 15: Rework scrolling so that we don't need a dedicated native
> widget for scrollframes. r=dbaron,joshmoz,karlt,jmathies

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're working around a core engine bug ... if you can come up with some kind of reduced testcase, we should fix the core bug.

Comment 17

[Adil Allawi]

Robert: You are right there could be a small css change and the issue can reappear. I tried to create a simple test case, however the issue only happens with certain emails at certain heights and cannot be reproduced reliably. I create a test case that works then I resize the window and it disappears. Trying to force the message pane height through css will work for one email but not another so I am not sure what to do to reduce this.

Comment 18

[Adil Allawi]

Attachment: Test case for repeating rendering issue

That was not easy. I managed to repeat the bug reliably on the "Welcome to Shredder" page that draws on startup. Copy the attached localstore.rdf to your profile folder and the window size should be just right to make the scroll of the Welcome screen display the rendering issue.

Comment 20

[Adil Allawi]

Attachment: Temporary patch to fix core scroll update issue

I isolated the scrolling bug down to an invalidate issue on the Mac - basically there is an error in the calculation of the invalidate rect of one pixel. 

I proved this with this patch. It calls [mView setNeedsDisplayInRect:rect] in nsChildView::Scroll with a rect based on the area to be updated. When I increased the size of the update rect by one pixel the problem stops happening.

Comment 21

[Philipp Kewisch [:Fallen][📱📆]]

I've tested the new 1px patch on Linux. Comparing with how it was before, the statubar looks a bit unproportional this way.

I think we should either go with the previous version (attachment 434183 [details] [diff] [review]), or even just let the button hit the top edge on windows/mac.

Comment 22

[Adil Allawi]

spun off the core layout issue into bug 555195 so that we can track it independently.

Comment 23

[Adil Allawi]

Attachment: Patch to Today Pane button CSS

I think I have got all the requirements fixed here.
- Rounded the padding to the nearest pixel to prevent triggering a core layout rendering issue.
- Reduced 1 pixel from the height of the button on Winstripe and Pinstripe
- Raised button label by one pixel to stop the 'y' in 'Today touching the bottom of the button.
- Added a margin to the button on Pinstripe and Winstripe so that it does not hit edges of the status bar.

Comment 24

[Adil Allawi]

Attachment: Screenshot of padding and margin in Windows and Mac

Comment 25

[Adil Allawi]

Attachment: Patch to Today Pane button CSS

Apologies for resending the patch - I squeezed an additional 1px height saving on pinstripe.
- Rounded the padding to the nearest pixel to prevent triggering a core layout
rendering issue.
- Reduced 1 pixel from the height of the button on Winstripe and Pinstripe
- Raised button label by one pixel to stop the 'y' in 'Today touching the
bottom of the button.
- Added a margin to the button on Pinstripe and Winstripe so that it does not
hit edges of the status bar.

Comment 26

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Linux Screenshot

This is how it looks on Linux. The statusbar still looks larger and a bit unproportional on the left side with the icons. Can't we just make the button stick to the top as it was before?

I'll leave the UI decision to Bryan though, I'm not an expert on that :-)

Comment 27

[Adil Allawi]

Attachment: Today button on Mac and Win with minimal height

So you can review the difference, I reduced the height of the today pane button to its minimum allowing it to touch the top and bottom of the status bar. Enclosed is the screenshot of this. The height was reduced by 2 pixels - one from the top and bottom margins.

Comment 28

[Adil Allawi]

Attachment: Patch with the minimal height for the today pane button

Here is the patch for the previous screenshot with one pixel stripped from the top and bottom margin.

Comment 30

[Mark Banner (:standard8)]

(In reply to comment #16)
> You're working around a core engine bug ... if you can come up with some kind
> of reduced testcase, we should fix the core bug.

Roc: I tried but I failed. I think this needs a good amount of xul and css knowledge and exactly what to get to overlap what and by how much. To be honest, I think this needs a core dev in that area to take a look at current Thunderbird & Lightning to be able to investigate & debug, I'm not sure we've got enough available expertise to look at it.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Adil already got a good reduced testcase in bug 555195, I just haven't gotten around to fixing the bug yet.

Comment 32

[Adil Allawi]

To be specific - this patch works around the core layout bug without creating any new display issues or patching any code outside of the lightning component. I think it would be worth checking this issue in if you wanted Lightning to be released before the layout engine is patched.

Comment 33

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

Comment on attachment 437812 [details]
Linux Screenshot

I'm ok with the extra top padding, I think this is better than extra padding at the bottom.  If we tried to align them center I think we'd get a very odd and uneven floating look.  So lets stick with this.

Comment 34

[cmtalbert]

(In reply to comment #32)
> To be specific - this patch works around the core layout bug without creating
> any new display issues or patching any code outside of the lightning component.
> I think it would be worth checking this issue in if you wanted Lightning to be
> released before the layout engine is patched.
I made these changes to my nightly lightning installation and so far, it works great on the mac.  My only nit on the patch is that you ought to call out that these changes are to work around a specific Gecko bug so we can remove these changes once the bug is fixed.

Comment 35

[Adil Allawi]

I do not think that is necessary. Even thought the reason for the patch is to work around a Gecko bug, the changes are an improvement to the CSS for the Mac and Windows as well. The original CSS was designed to work well for Linux see my comment #8 and comment #13 above.

Comment 36

[cmtalbert]

(In reply to comment #35)
> I do not think that is necessary. Even thought the reason for the patch is to
> work around a Gecko bug, the changes are an improvement to the CSS for the Mac
> and Windows as well. The original CSS was designed to work well for Linux see
> my comment #8 and comment #13 above.
Sounds fine.  Thanks again for the patch.

Comment 37

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 437843 [details] [diff] [review]
Patch with the minimal height for the today pane button

This looks fine on linux, r=philipp

Comment 38

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/bd2fc742e3e1>
-> FIXED

Comment 39

[Adil Allawi]

Not sure how this works but shouldn't this patch also be applied to the comm-1.9.2 branch as well?

Comment 40

[Philipp Kewisch [:Fallen][📱📆]]

Right now, Lightning is still building from comm-central. I will transplant all calendar patches applied after the comm-1.9.2 branching soon and then have the buildbots changed.

Comment 41

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/c06e5c955a8d>
-> FIXED