Tall <canvas> element fails to scroll with the rest of the page

RESOLVED FIXED in mozilla2.0b12

Status

()

defect
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: drh, Assigned: Ehsan)

Tracking

({regression})

Trunk
mozilla2.0b12
All
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker], )

Attachments

(1 attachment, 3 obsolete attachments)

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.
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Component: General → Graphics
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → thebes
Hardware: x86 → All
Version: unspecified → Trunk
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
Whiteboard: [hardblocker]
blocking2.0: ? → betaN+
Matt, do you mind taking a look at this?
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?
Blocks: 633936
Whiteboard: [hardblocker] → [hardblocker][needs owner]
It appears that this regressed in changeset 31b46f48f714 (bug 621778).
Blocks: 621778
Assignee: nobody → matt.woodrow+bugzilla
Whiteboard: [hardblocker][needs owner] → [hardblocker]
No longer blocks: 633936
I can't reproduce this on the latest nightly with OpenGL acceleration enabled.
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.
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. :/
I can repro in a new profile!
So, apparently, if you make the window wide enough, this doesn't happen any more.
Seems like as long as the canvas height is below 8192 (0x2000), things work just fine.
Assignee: matt.woodrow+bugzilla → ehsan
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 ...
Posted patch Patch (v1) (obsolete) — Splinter Review
There will also be tests, but not before I get lunch!  ;-)
Attachment #512269 - Flags: review?(matt.woodrow+bugzilla)
(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 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.
Posted patch Patch (v2) (obsolete) — Splinter Review
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 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+
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)
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).
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 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+
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][needs landing]
(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.  :-)
s/flag/glad/
Posted patch For check-in (obsolete) — Splinter Review
Attachment #512308 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/f3d13890fbe3
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [hardblocker][has patch][needs landing] → [hardblocker]
Target Milestone: --- → mozilla2.0b12
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 → ---
We do indeed, OpenGL is the only accelerated backend that can handle canvases that are larger than the max texture size.
After a discussion we decided against allowing textures that were larger than the maximum texture size on windows.
So, does this mean that this bug is irrelevant on Windows?
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?
(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.
Ehsan, bug 633936 covers the Windows fail.
Attachment #512403 - Attachment is obsolete: true
Attachment #512549 - Flags: review?
Attachment #512549 - Flags: review? → review?(joe)
Attachment #512549 - Flags: review?(joe) → review+
http://hg.mozilla.org/mozilla-central/rev/b6dc8964dea8
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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.
Depends on: 634581
Depends on: 774689
You need to log in before you can comment on or make changes to this bug.