514127 - "Scroll ignored" area rendered wrong in fennec

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Oleg Romashin (:romaxa)]

Attachment: Picture

Install:
http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/fennec-1.0b4pre.en-US.linux-i686.tar.bz2


./fennec http://linux.org.ru

Scroll down a bit to see how rendered area outside overflow rect.

EXPECTED: background is black, no white painted area
ACTUAL: background is rendered white outside of rendered area

Comment 1

[Oleg Romashin (:romaxa)]

Regression is:
http://ftp.mozilla.org/pub/mozilla.org/xulrunner/nightly/2009/07/2009-07-04-03-mozilla-central/xulrunner-1.9.2a1pre.en-US.linux-i686.tar.bz2
http://ftp.mozilla.org/pub/mozilla.org/xulrunner/nightly/2009/07/2009-07-05-03-mozilla-central/xulrunner-1.9.2a1pre.en-US.linux-i686.tar.bz2

Comment 2

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-07-03+18%3A00%3A02&enddate=2009-07-05+02%3A39%3A04

Comment 3

[Oleg Romashin (:romaxa)]

This bug looks like 100% regression candidate:
http://hg.mozilla.org/mozilla-central/rev/23942ec68af5

Bug 488242. Make iframes with semi-transparent backgrounds work correctly by painting all canvas background colors using a dedicated fallback background color display item. r+sr=roc

Comment 5

[Oleg Romashin (:romaxa)]

Tested mozilla-1.9.2 without 
http://hg.mozilla.org/mozilla-central/rev/23942ec68af5
change - Works fine.

Comment 6

[Timothy Nikkel (:tnikkel)]

From discussing in #mobile it seems Fennec doesn't update the scrollport properly. I think this is probably why bug 488242 is causing the problem; because it draws the background color based on the viewport. So it sounds like updating the scrollport is the proper solution. But if not I think I can change RenderDocument (the guts of drawWindow) to paint the canvas background color over the passed in rect instead of the viewport. That actually might be a good idea regardless.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: patch?

Could someone try this patch with Fennec and see if it fixes the problem?

Comment 8

[Oleg Romashin (:romaxa)]

Comment on attachment 398092 [details] [diff] [review]
patch?

It solve problem with MicroB browser, I guess with fennec it should work too.

Comment 9

[Oleg Romashin (:romaxa)]

I have tested it a bit more on N900 device, and in the beginning of page loading it flashes, probably it possible to see on desktop too, by running fennec with valgrind (make it slower)
with white background and then it works fine.

Comment 10

[Timothy Nikkel (:tnikkel)]

Flashes caused by this patch?

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 398092 [details] [diff] [review]
patch?

This patch seems like a good idea for drawWindow in general. Code that tries to render the entire web content to a canvas will need this patch to work right.

Comment 12

[Timothy Nikkel (:tnikkel)]

One question I wanted to answer before potentially getting this in was what we do if the passed in rect extends outside the document bounds? Is that allowed? This patch will probably change what happens in that case.

Comment 13

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

IMHO drawWindow should draw nothing outside the document bounds.

I think here if RENDER_IGNORE_VIEWPORT_SCROLLING is not set we should keep doing what we're doing --- the canvas background color fills the viewport --- otherwise we set the canvas background color to cover the area of the root scrolled frame ... so we should pass GetRootScrollFrameAsScrollable()->GetScrolledFrame() as the frame.

Comment 14

[Timothy Nikkel (:tnikkel)]

I think what we want is the overflow rect of GetRootScrollFrameAsScrollable()->GetScrolledFrame(), which is a CanvasFrame. The rect of the scrolled frame is just the size of the scrollport and the position marks the scroll position. Or more specifically CanvasFrame::CanvasArea is what we want, which is what the canvas background is drawn over. Without duplicating that code, I guess the best way to access CanvasFrame::CanvasArea is to pull out the definition of CanvasFrame into a header then?

Comment 15

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

You're right. You could move it into a helper function in nsLayoutUtils.

Comment 16

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 17

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

Oh, we should probably have a mochitest for this.

Two things that would be nice to do in followup:
-- rename nsHTMLFrame.h/.cpp to nsCanvasFrame.h/.cpp
-- remove nsICanvasFrame and just use nsCanvasFrame instead

Comment 18

[Timothy Nikkel (:tnikkel)]

...and CanvasFrame -> nsCanvasFrame? Or what is the plan for getting rid of the ns prefix and using the mozilla namespace for frames?

Comment 19

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

That's a pretty big project that I don't want to start here!

Comment 20

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

So yeah, renaming to nsCanvasFrame would be desirable.

Comment 21

[Timothy Nikkel (:tnikkel)]

Attachment: patch v3

Just added a mochitest.

I'll do that cleanup in a followup later.

Comment 22

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

Comment on attachment 400963 [details] [diff] [review]
patch v3

Do you actually have to escape the entire iframe src URL? I seem to recall writing iframe data: URLs without escaping <> and using ' for quotes

Comment 23

[Timothy Nikkel (:tnikkel)]

You're probably right, but I've had problems before, so I just ran the iframe src data URL through a encoder and pasted instead of (possibly) trying to figure out why it wasn't working. With the full unencoded src right there it shouldn't cause any problem.

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/aba4d636382d

Comment 25

[Timothy Nikkel (:tnikkel)]

I need to merge this to 1.9.2 before landing on 1.9.2.

Comment 26

[John Daggett (:jtd)]

This appears to be causing 3.6 Tinderbox orange on Linux:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1253752451.1253759850.26712.gz

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6-Unittest/1253755683.1253757320.31162.gz

Comment 27

[Timothy Nikkel (:tnikkel)]

Looking into it.

Comment 28

[Timothy Nikkel (:tnikkel)]

The failing tests pass on my Linux box, and this is on m-c. I will wait until the Linux test results are in on the next push, if it's still failing I'll backout.

Comment 29

[Timothy Nikkel (:tnikkel)]

Tests are all starred or green, so marking fixed on 1.9.2.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9170c06ef1e5

Comment 30

[Oleg Romashin (:romaxa)]

Problem is still not fixed properly with latest patch.

When I scroll page down, it works fine, but when I scroll the same page back to TOP, white background visible.
http://maemo.org/news/events/maemo_summit_2009/#participants
Reproducible on:
http://ftp.mozilla.org/pub/mozilla.org/xulrunner/nightly/2009-09-24-03-mozilla-1.9.2/
And fennec nightly


With patch https://bug514127.bugzilla.mozilla.org/attachment.cgi?id=398092
It works fine.

Comment 31

[Timothy Nikkel (:tnikkel)]

Filed bug 518777 for this issue. Marking this resolved because it did fix a real problem, and trying to fix this new issue in this bug would just cause confusion as to what is fixed and what is not etc.

Comment 32

[Timothy Nikkel (:tnikkel)]

The cleanup mentioned in comment 17 is filed as bug 520425.

Comment 33

[Henri Sivonen (:hsivonen)]

The test case depends on the implementation details of the old HTML parser on how data: URL parsing maps to event loop tasks (bug 546639).