Closed
Bug 632781
Opened 14 years ago
Closed 14 years ago
Tall <canvas> element fails to scroll with the rest of the page
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla2.0b12
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: drh, Assigned: ehsan.akhgari)
References
()
Details
(Keywords: regression, Whiteboard: [hardblocker])
Attachments
(1 file, 3 obsolete files)
5.15 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11) Gecko/20100101 Firefox/4.0b11
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11) Gecko/20100101 Firefox/4.0b11
The <canvas> element that contains the graph does not scroll together with the rest of the document on the particular page shown above. Similar tall canvas elements work fine on other pages. (I do not know what is special about the particular page shown.) Everything works fine on FF 3.5.4, Chrome, Safari, Opera, IE.
Reproducible: Always
Steps to Reproduce:
1. Visit the URL shown above
2. Scroll down
3.
Actual Results:
The text scrolls but the <canvas> stops scrolling when its top edge hits the top of the browser window.
Expected Results:
The <canvas> should scroll in sync with the text
I am the author of the problem that generates the page that demonstrates the fault. If you think I am misusing or misconfiguring the <canvas>, I would appreciate you letting me know so that I can fix the problem on my end. But I'm thinking this is an FF4 problem as the same code works on *most* pages (just not the one example shown) in FF4 and on all pages in all other browsers including FF3.
Updated•14 years ago
|
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Component: General → Graphics
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → thebes
Hardware: x86 → All
Version: unspecified → Trunk
![]() |
||
Comment 1•14 years ago
|
||
This is a recent regression; regression range is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=66addc5c30ca&tochange=eb105fe0e41c
Disabling accelerated layers fixes things for me.
Markus, you have blake for the most likely culprits in that range...
Keywords: regression
Updated•14 years ago
|
Whiteboard: [hardblocker]
Assignee | ||
Updated•14 years ago
|
blocking2.0: ? → betaN+
Comment 2•14 years ago
|
||
Matt, do you mind taking a look at this?
Comment 3•14 years ago
|
||
At least to give some risk assessment.
Probably a GL layers bug I guess. Perhaps something to do with clipping ... drawing the top-left of the canvas at the visible-region top-left instead of at 0,0?
Updated•14 years ago
|
Whiteboard: [hardblocker] → [hardblocker][needs owner]
Comment 5•14 years ago
|
||
It appears that this regressed in changeset 31b46f48f714 (bug 621778).
Blocks: 621778
Updated•14 years ago
|
Assignee: nobody → matt.woodrow+bugzilla
Updated•14 years ago
|
Whiteboard: [hardblocker][needs owner] → [hardblocker]
Assignee | ||
Comment 6•14 years ago
|
||
I can't reproduce this on the latest nightly with OpenGL acceleration enabled.
Comment 7•14 years ago
|
||
Just downloaded today's Mac nightly and it still happens here. Setting layers.acceleration.disabled=true fixes it for me so that the <canvas> scrolls properly.
Assignee | ||
Comment 8•14 years ago
|
||
I can't reproduce this on OS X 10.6.6 no matter how I try it, so I may not be able to help out here. :/
Assignee | ||
Comment 9•14 years ago
|
||
I can repro in a new profile!
Assignee | ||
Comment 10•14 years ago
|
||
So, apparently, if you make the window wide enough, this doesn't happen any more.
Assignee | ||
Comment 11•14 years ago
|
||
Seems like as long as the canvas height is below 8192 (0x2000), things work just fine.
Assignee | ||
Updated•14 years ago
|
Assignee: matt.woodrow+bugzilla → ehsan
Comment 12•14 years ago
|
||
Why is this betaN+? Just want to make sure it's not something we can fix for final. Seems like it's a limited scope with a defined testcase ...
Assignee | ||
Comment 13•14 years ago
|
||
There will also be tests, but not before I get lunch! ;-)
Attachment #512269 -
Flags: review?(matt.woodrow+bugzilla)
Assignee | ||
Comment 14•14 years ago
|
||
(In reply to comment #12)
> Why is this betaN+? Just want to make sure it's not something we can fix for
> final. Seems like it's a limited scope with a defined testcase ...
I think this only affects non-accelerated canvases. If the only reason we would do that is because of the canvas size, then I agree that this could probably be moved to final (but I'm cheating, I'm looking at the fix too).
Comment 15•14 years ago
|
||
Comment on attachment 512269 [details] [diff] [review]
Patch (v1)
>
> mLayerProgram =
> gl()->UploadSurfaceToTexture(mCanvasSurface,
> drawRect,
> mTexture,
>- true);
>+ true,
>+ drawRect.TopLeft());
> }
The second parameter, aDstRegion, is the region (or rect that gets promoted) of the texture we are uploading to. Since we always want our data to be at the top left of the newly created texture, this should probably be nsIntRect(0, 0, drawRect.width, drawRect.height);
The last parameter, aSrcPoint (where on the source surface the region is located) change looks correct.
Assignee | ||
Comment 16•14 years ago
|
||
Addressed the comment and added two tests, one for very large canvases, the other for regularly sized canvases (for completeness).
Attachment #512269 -
Attachment is obsolete: true
Attachment #512308 -
Flags: review?(matt.woodrow+bugzilla)
Attachment #512269 -
Flags: review?(matt.woodrow+bugzilla)
Comment 17•14 years ago
|
||
Comment on attachment 512308 [details] [diff] [review]
Patch (v2)
I'm not sure if I'm particularly qualified to review the tests part, they look fine to me though.
The rest looks good :)
Attachment #512308 -
Flags: review?(matt.woodrow+bugzilla) → review+
Assignee | ||
Comment 18•14 years ago
|
||
Comment on attachment 512308 [details] [diff] [review]
Patch (v2)
Requesting review on tests from roc to satisfy the review requirements, and get another pair of eyes looking at this.
Attachment #512308 -
Flags: review?(roc)
Updated•14 years ago
|
Whiteboard: [hardblocker] → [hardblocker][has patch]
+ drawRect.TopLeft());
This can't be right. Shouldn't we always pass 0,0 here?
Seems to me we should just be ignoring the visible region here and uploading the entire mBounds always (and mBounds' TopLeft is always 0,0).
Comment 20•14 years ago
|
||
This is the special case where the canvas size is larger than the max texture size.
In this case we only upload the intersection of mBounds with the visible region.
drawRect.TopLeft() is the point on the source surface where we should be uploading from.
Comment 21•14 years ago
|
||
Comment on attachment 512308 [details] [diff] [review]
Patch (v2)
>diff --git a/layout/reftests/bugs/632781-1-ref.html b/layout/reftests/bugs/632781-1-ref.html
>+ <div style="width: 120px; height: 120px; overflow: hidden">
>+ <canvas width="120" height="120" id="c"></canvas>
>+ ctx.fillRect(0, 0, 10000, 10000);
Something is askew here. Maybe you should just use 120, 120 for w/h?
>diff --git a/layout/reftests/bugs/632781-1.html b/layout/reftests/bugs/632781-1.html
Maybe rename this to 632781-verybig.html?
>+ <div id="container" style="width: 100px; height: 100px; padding: 10px; overflow: hidden">
Is there any particular reason you use padding: 10px instead of width: 120 and height: 120?
>diff --git a/layout/reftests/bugs/632781-2.html b/layout/reftests/bugs/632781-2.html
Maybe rename this to 632781-normalsize.html?
As an aside, I tried very hard to use these tests on my local machine, and was *very upset* when the tests didn't work. Despite the fact that I was just using nightlies that *still have this bug.*
Attachment #512308 -
Flags: review+
Attachment #512308 -
Flags: review?(roc) → review+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][needs landing]
Assignee | ||
Comment 22•14 years ago
|
||
(In reply to comment #21)
> Comment on attachment 512308 [details] [diff] [review]
> Patch (v2)
>
>
> >diff --git a/layout/reftests/bugs/632781-1-ref.html b/layout/reftests/bugs/632781-1-ref.html
>
> >+ <div style="width: 120px; height: 120px; overflow: hidden">
> >+ <canvas width="120" height="120" id="c"></canvas>
>
> >+ ctx.fillRect(0, 0, 10000, 10000);
>
> Something is askew here. Maybe you should just use 120, 120 for w/h?
Yep, indeed. I'll change it.
> >diff --git a/layout/reftests/bugs/632781-1.html b/layout/reftests/bugs/632781-1.html
>
> Maybe rename this to 632781-verybig.html?
Sure.
> >+ <div id="container" style="width: 100px; height: 100px; padding: 10px; overflow: hidden">
>
> Is there any particular reason you use padding: 10px instead of width: 120 and
> height: 120?
I'll add a comment as to why I did that.
> >diff --git a/layout/reftests/bugs/632781-2.html b/layout/reftests/bugs/632781-2.html
>
> Maybe rename this to 632781-normalsize.html?
Sure.
> As an aside, I tried very hard to use these tests on my local machine, and was
> *very upset* when the tests didn't work. Despite the fact that I was just using
> nightlies that *still have this bug.*
That's the whole point. I'm flag that I made you very upset there. :-)
Assignee | ||
Comment 23•14 years ago
|
||
s/flag/glad/
Assignee | ||
Comment 24•14 years ago
|
||
Attachment #512308 -
Attachment is obsolete: true
Assignee | ||
Comment 25•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [hardblocker][has patch][needs landing] → [hardblocker]
Target Milestone: --- → mozilla2.0b12
Assignee | ||
Comment 26•14 years ago
|
||
I backed out the patch because of orange on Windows reftest:
http://hg.mozilla.org/mozilla-central/rev/9ed6bef2a9ac
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297790651.1297794178.4385.gz#err0
Judging from the initial inspection, it seems that the canvas in the "verybig" test is white. Maybe it's just too large. Do we have a max size on canvases on Windows?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 27•14 years ago
|
||
We do indeed, OpenGL is the only accelerated backend that can handle canvases that are larger than the max texture size.
Comment 28•14 years ago
|
||
After a discussion we decided against allowing textures that were larger than the maximum texture size on windows.
Assignee | ||
Comment 29•14 years ago
|
||
So, does this mean that this bug is irrelevant on Windows?
Assignee | ||
Comment 30•14 years ago
|
||
To elaborate, IINM this bug only happens when the canvas size is larger than the texture buffer size. If comment 28 is true, this shouldn't be an issue per se, but I don't know how we would render those very large canvases on Windows. Do we just skip them? Is that why the rendering in my reftest is white?
Comment 31•14 years ago
|
||
(In reply to comment #30)
> To elaborate, IINM this bug only happens when the canvas size is larger than
> the texture buffer size. If comment 28 is true, this shouldn't be an issue per
> se, but I don't know how we would render those very large canvases on Windows.
> Do we just skip them? Is that why the rendering in my reftest is white?
Indeed we just fail.
![]() |
||
Comment 32•14 years ago
|
||
Ehsan, bug 633936 covers the Windows fail.
Assignee | ||
Comment 33•14 years ago
|
||
Attachment #512403 -
Attachment is obsolete: true
Attachment #512549 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #512549 -
Flags: review? → review?(joe)
Updated•14 years ago
|
Attachment #512549 -
Flags: review?(joe) → review+
Assignee | ||
Comment 34•14 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 35•14 years ago
|
||
It seems like on Windows XP, layersGPUAccelerated is false (bug 634422). This caused the reftest to fail on Windows XP: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297806979.1297808662.912.gz&fulltext=1.
I marked the test as failing on all versions of Windows: http://hg.mozilla.org/mozilla-central/rev/eeb1dd9e7028.
This probably means that bug 633936 also happens on Windows XP.
You need to log in
before you can comment on or make changes to this bug.
Description
•