Closed
Bug 514127
Opened 15 years ago
Closed 15 years ago
"Scroll ignored" area rendered wrong in fennec
Categories
(Core :: Layout, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
fennec | 1.0+ | --- |
People
(Reporter: romaxa, Assigned: tnikkel)
References
Details
Attachments
(2 files, 2 obsolete files)
82.80 KB,
image/png
|
Details | |
17.24 KB,
patch
|
roc
:
review+
pavlov
:
approval1.9.2+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•15 years ago
|
||
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
Reporter | ||
Comment 2•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-07-03+18%3A00%3A02&enddate=2009-07-05+02%3A39%3A04
Reporter | ||
Comment 3•15 years ago
|
||
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
Blocks: 488242
Updated•15 years ago
|
tracking-fennec: --- → ?
Reporter | ||
Comment 5•15 years ago
|
||
Tested mozilla-1.9.2 without http://hg.mozilla.org/mozilla-central/rev/23942ec68af5 change - Works fine.
Assignee | ||
Comment 6•15 years ago
|
||
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.
Assignee | ||
Comment 7•15 years ago
|
||
Could someone try this patch with Fennec and see if it fixes the problem?
Reporter | ||
Comment 8•15 years ago
|
||
Comment on attachment 398092 [details] [diff] [review] patch? It solve problem with MicroB browser, I guess with fennec it should work too.
Reporter | ||
Comment 9•15 years ago
|
||
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.
Assignee | ||
Comment 10•15 years ago
|
||
Flashes caused by this patch?
Updated•15 years ago
|
Attachment #398092 -
Flags: review?(roc)
Comment 11•15 years ago
|
||
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.
Assignee | ||
Comment 12•15 years ago
|
||
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.
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.
Updated•15 years ago
|
tracking-fennec: ? → 1.0+
Assignee | ||
Comment 14•15 years ago
|
||
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?
You're right. You could move it into a helper function in nsLayoutUtils.
Assignee | ||
Comment 16•15 years ago
|
||
Assignee: nobody → tnikkel
Attachment #398092 -
Attachment is obsolete: true
Attachment #400447 -
Flags: review?(roc)
Attachment #398092 -
Flags: review?(roc)
Attachment #400447 -
Flags: review?(roc) → review+
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
Assignee | ||
Comment 18•15 years ago
|
||
...and CanvasFrame -> nsCanvasFrame? Or what is the plan for getting rid of the ns prefix and using the mozilla namespace for frames?
That's a pretty big project that I don't want to start here!
So yeah, renaming to nsCanvasFrame would be desirable.
Assignee | ||
Comment 21•15 years ago
|
||
Just added a mochitest. I'll do that cleanup in a followup later.
Attachment #400447 -
Attachment is obsolete: true
Attachment #400963 -
Flags: review?(roc)
Attachment #400963 -
Flags: review?(roc) → review+
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
Assignee | ||
Comment 23•15 years ago
|
||
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.
Keywords: checkin-needed
Assignee | ||
Updated•15 years ago
|
Flags: wanted1.9.2?
Assignee | ||
Updated•15 years ago
|
Attachment #400963 -
Flags: approval1.9.2?
Updated•15 years ago
|
Attachment #400963 -
Flags: approval1.9.2? → approval1.9.2+
Comment 24•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/aba4d636382d
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Updated•15 years ago
|
Flags: in-testsuite+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: needs 1.9.2 landing
Assignee | ||
Comment 25•15 years ago
|
||
I need to merge this to 1.9.2 before landing on 1.9.2.
Keywords: checkin-needed
Whiteboard: needs 1.9.2 landing
Flags: wanted1.9.2? → blocking1.9.2+
Priority: -- → P2
Comment 26•15 years ago
|
||
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
Assignee | ||
Comment 27•15 years ago
|
||
Looking into it.
Assignee | ||
Comment 28•15 years ago
|
||
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.
Assignee | ||
Comment 29•15 years ago
|
||
Tests are all starred or green, so marking fixed on 1.9.2. http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9170c06ef1e5
status1.9.2:
--- → beta1-fixed
Reporter | ||
Comment 30•15 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 31•15 years ago
|
||
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.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 32•15 years ago
|
||
The cleanup mentioned in comment 17 is filed as bug 520425.
Comment 33•14 years ago
|
||
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).
You need to log in
before you can comment on or make changes to this bug.
Description
•