Closed
Bug 506826
Opened 15 years ago
Closed 14 years ago
Implement -moz-element(): using arbitrary elements as the source for CSS backgrounds
Categories
(Core :: Layout: Images, Video, and HTML Frames, enhancement)
Core
Layout: Images, Video, and HTML Frames
Tracking
()
RESOLVED
FIXED
mozilla2.0b4
People
(Reporter: rkawaguchi, Assigned: mstange)
References
Details
(Keywords: dev-doc-complete)
Attachments
(9 files, 14 obsolete files)
233.15 KB,
patch
|
Details | Diff | Splinter Review | |
3.40 KB,
patch
|
Details | Diff | Splinter Review | |
3.68 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
12.96 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
7.29 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
78.04 KB,
patch
|
Details | Diff | Splinter Review | |
5.90 KB,
patch
|
Details | Diff | Splinter Review | |
29.56 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
260.77 KB,
patch
|
dbaron
:
approval2.0+
|
Details | Diff | Splinter Review |
Implement the followings to allow use of an arbitrary element as the source for CSS background. 1) A new CSS image type, -moz-element(#foo). Normally 'foo' is the ID of an element in the document to which the stylesheet applies. 2) A new DOM API, window.mozSetImageElement("foo", node). The node is used instead of the element with ID foo when -moz-element(#foo) is used. For more details discussion: http://weblogs.mozillazine.org/roc/archives/2008/07/the_latest_feat.html http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/d58cbafa83471800/b4b5ab1849ffe3af?lnk=gst&q=proposal%3A+using+arbitrary+elements+as+the+source+for+CSS+backgrounds
Reporter | ||
Comment 1•15 years ago
|
||
This patch is worked on top of the patch v4 for bug 113577. This patch brings forward changes in roc's svg-integration repo to the latest trunk and implements "-moz-element()" CSS function to be used with "background" or "background-image" property. (mozSetImageElement() is not implemented yet.) This passes all the reftests except for one. (I need to tweak DrawPaintServer()).
Reporter | ||
Comment 2•15 years ago
|
||
Reftests for patch v1. (I brought them forward from roc's repo and replaced url(#id) with -moz-element(#id)).
Reporter | ||
Comment 3•15 years ago
|
||
Comment on attachment 390976 [details] [diff] [review] patch v1 This patch also brings forward the feature to use gradient and pattern SVG frames as the source of CSS backgrounds.
Reporter | ||
Comment 4•15 years ago
|
||
I was thinking how I should introduce the image snapping logic (bug 458487) into paint server rendering since a half of the image snapping code is hidden inside imgFrame, which we are discouraged to directly access. Some options would be: (1) Copy and paste the image snapping code from imgFrame::Draw() to nsSVGIntegrationUtils::DrawPaintServer(); the idea of duplicating over 100 lines of code does not sound so good. (2) Move the image snapping code from imgFrame::Draw() to somewhere else that can be accessed from both imglib2 and layout components. (3) Renders a paint server frame as an imgIContainer image and simply toss it to nsLayoutUtils::DrawImage(), which internally calls imgFrame::Draw(). An extra step of drawing a paint server on the surface of a temporary imgIContainer is not too bad, I think, because we anyway have to draw it on a temporary surface when tiling it. I implemented the option (3) in patch v2 because it makes the most of the existing code base. But, the option (3) is a kind of cheating and may be a very bad idea. Any advice on this issue? The patch v2 fails 5 reftests (all moz-element() related); the causes are the same: an unexpected scaling.
Reporter | ||
Updated•15 years ago
|
Updated•15 years ago
|
Version: unspecified → Trunk
Reporter | ||
Comment 5•15 years ago
|
||
I still need to clean up DrawPaintServer() and fix a leak and a few reftests. Anyway, It's very close to finish. I am hoping that I can finish this in a week after going back to Japan.
Attachment #390976 -
Attachment is obsolete: true
Attachment #390977 -
Attachment is obsolete: true
Attachment #391236 -
Attachment is obsolete: true
Assignee | ||
Comment 6•15 years ago
|
||
Ryo, do you think you'll have time to finish this patch, or do you want somebody else to take over?
Comment 7•15 years ago
|
||
Markus, I'm sorry for this late reply. I will have some free time towards the end of this month, and am planning to work on this patch. However, if this patch needs be finished sometime soon, it's no problem for me that someone else takes it over.
Comment 8•14 years ago
|
||
Will this be a means to use videos as a background, too? There are some pretty fancy examples of use of video as background listed here http://abduzeedo.com/web-inspiration-video-backgrounds, though these are all using flash, of course.
Assignee | ||
Comment 9•14 years ago
|
||
I'd like to work on this because I need it for the new Mac theme, see bug 547805. I've looked at the v2 and v3 patches (attachment 391236 [details] [diff] [review] and attachment 399463 [details] [diff] [review]) and have some questions: - What syntax have we decided on? v2 restricts the argument of -moz-element to #elementid, but v3 reuses url() parsing and thus accepts much more. The discussion in the newsgroup (see comment 0) seems to have come to the conclusion that we want the v2 approach. - What to do with the duplicated image snapping code? This problem is described in comment 4, and while Ryo went for option (3) in the v2 patch, the v3 patch uses option (1). Another option I thought of would be to move all the code from imgFrame::Draw to DrawImageInternal in nsLayoutUtils.cpp, remove the Draw method on imgFrame and imgIContainer and instead add a GetSurfaceForDrawing method to them which returns the gfxASurface that imgFrame::Draw would have used. Then we'd only have to fix up the one other caller of imgIContainer::Draw, nsBaseDragService. And GetSurfaceForDrawing would need some special indication for the single-color case so that we keep that optimization.
(In reply to comment #9) > - What syntax have we decided on? v2 restricts the argument of -moz-element to > #elementid, but v3 reuses url() parsing and thus accepts much more. The > discussion in the newsgroup (see comment 0) seems to have come to the > conclusion that we want the v2 approach. Yeah, that's fine. > - What to do with the duplicated image snapping code? This problem is > described in comment 4, and while Ryo went for option (3) in the v2 patch, > the v3 patch uses option (1). > Another option I thought of would be to move all the code from > imgFrame::Draw to DrawImageInternal in nsLayoutUtils.cpp, remove the Draw > method on imgFrame and imgIContainer and instead add a GetSurfaceForDrawing > method to them which returns the gfxASurface that imgFrame::Draw would have > used. That won't work well with SVG images. I think we need either 1 or 2. I don't quite know how this will work with bug 547805. There, you want to align the rendering of -moz-element() based on the position of the referenced element, and these patches don't do that (and in many use cases we wouldn't want to do that). Maybe we will need an extension value for background-position, say -moz-align-with-source, that positions the center of a -moz-element() image at the center of its source element, if there is one? Eventually I would like to extend this so it can be used for TabCandy, accelerated using layers. Let's not go there immediately, but I'd like to make sure the API doesn't prevent that. I think right now we're in good shape.
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #10) > > - What to do with the duplicated image snapping code? This problem is > > described in comment 4, and while Ryo went for option (3) in the v2 patch, > > the v3 patch uses option (1). > > Another option I thought of would be to move all the code from > > imgFrame::Draw to DrawImageInternal in nsLayoutUtils.cpp, remove the Draw > > method on imgFrame and imgIContainer and instead add a GetSurfaceForDrawing > > method to them which returns the gfxASurface that imgFrame::Draw would have > > used. > > That won't work well with SVG images. Why not? I've looked at imgContainerSVG::Draw in http://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/file/07ca124f2309/svg_images - we construct an empty gfxASurface and use presShell->RenderDocument to draw into it, so couldn't we just return that surface instead and draw it in nsLayoutUtils? Or do you mean that SVG images don't want the pixel snapping that imgFrame::Draw does? > I don't quite know how this will work with bug 547805. There, you want to align > the rendering of -moz-element() based on the position of the referenced > element Well, not really. I wasn't going to reference an element, but an image outside the document set by document.mozSetImageElement. And I think background-attachment: fixed will take care of the positioning nicely. > Eventually I would like to extend this so it can be used for TabCandy, > accelerated using layers. Not sure what this will look like. Are you planning to create a separate layer for backgrounds that use -moz-element and then reflect the relation between that layer and the referenced element's layer in the layer tree? Or just reusing the retained layer of the referenced element when rendering the background?
(In reply to comment #11) > Why not? I've looked at imgContainerSVG::Draw in > http://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/file/07ca124f2309/svg_images > - we construct an empty gfxASurface and use presShell->RenderDocument to draw > into it, so couldn't we just return that surface instead and draw it in > nsLayoutUtils? Or do you mean that SVG images don't want the pixel snapping > that imgFrame::Draw does? I haven't read those patches yet, so dholbert should comment. But for the non-tiled case, we shouldn't need a temporary surface. Rather than stretching a surface to fill the image area, instead SVG images are drawn to correctly fill the image area. > > I don't quite know how this will work with bug 547805. There, you want to align > > the rendering of -moz-element() based on the position of the referenced > > element > > Well, not really. I wasn't going to reference an element, but an image outside > the document set by document.mozSetImageElement. And I think > background-attachment: fixed will take care of the positioning nicely. Good idea, background-attachment:fixed will work fine. > > Eventually I would like to extend this so it can be used for TabCandy, > > accelerated using layers. > > Not sure what this will look like. Are you planning to create a separate layer > for backgrounds that use -moz-element and then reflect the relation between > that layer and the referenced element's layer in the layer tree? Yes. When we reference an element with a layer, we should create a layer for the -moz-element background and for the referenced element (if possible) and hook them up in the layer tree so the retained layer subtree for the referenced element is used directly in the -moz-element background. That would let us accelerate everything with the GPU.
Assignee | ||
Comment 13•14 years ago
|
||
Okay. For option (2), what place is there that can be accessed from both imglib and from layout? Do I need to create that place first? Or should I just go with code duplication for now?
Comment 14•14 years ago
|
||
Remember the evangelism follow up: Add a test to Modernizr and tip off the author of http://caniuse.com/ In fact, an evangelism tracking bug to make sure Modernizr tests for everything Firefox supports might be a good idea...
Lars: Indeed. Do you want to file that bug? You might even fix it :-) Markus: we could put shared snapping logic in gfx maybe?
Assignee | ||
Comment 16•14 years ago
|
||
Attachment #448606 -
Flags: review?(dbaron)
Assignee | ||
Comment 17•14 years ago
|
||
Comment on attachment 448606 [details] [diff] [review] part 1, v1: style system Wait, I think this leaks strings.
Attachment #448606 -
Flags: review?(dbaron)
Probably the nsStyleImage::mElementId. However, style structs don't get destructors run, so instead of making nsStyleImage::mElementId own a string, you should make it point to the memory in the style rule.
Assignee | ||
Comment 19•14 years ago
|
||
(In reply to comment #18) > However, style structs don't get destructors run Not sure what you mean, I see a destructor called ~nsStyleImage(). > so instead of making nsStyleImage::mElementId own a string, > you should make it point to the memory in the style rule. Is that the memory of the nsCSSValue that gets passed to SetStyleImage? That's what I'm using in this patch now. Is that nsCSSValue guaranteed to outlive the nsStyleImage object filled from it?
Attachment #448606 -
Attachment is obsolete: true
Attachment #448764 -
Flags: review?(dbaron)
Assignee | ||
Comment 20•14 years ago
|
||
(In reply to comment #15) > Markus: we could put shared snapping logic in gfx maybe? Moving it into gfxUtils seems to work, thanks.
Assignee | ||
Comment 21•14 years ago
|
||
Ryo's latest patch makes dynamic updates to out-of-document canvases work by having nsHTMLCanvasElement dispatch a "redraw" event that nsSVGRenderingObserver listens for. Is that a good approach? These events aren't standard so I'm not sure if it's a good idea to expose them to web content.
I don't think we should be firing DOM events here. We should probably just add a private C++ API to nsHTMLCanvasElement that lets you add an observer that will be called back on updates.
Assignee | ||
Comment 23•14 years ago
|
||
The previous patch was missing the implementations of nsStyleImage::IsOpaque(), IsComplete() and operator==.
Attachment #448764 -
Attachment is obsolete: true
Attachment #449487 -
Flags: review?(dbaron)
Attachment #448764 -
Flags: review?(dbaron)
Assignee | ||
Comment 24•14 years ago
|
||
Comment on attachment 449487 [details] [diff] [review] part 1, v3 Still not done... I need to add nsChangeHint_UpdateEffects in CalcDifference when the referenced element has changed. I'll request review when everything runs and passes tests.
Attachment #449487 -
Flags: review?(dbaron)
Assignee | ||
Comment 25•14 years ago
|
||
Like the previous version, but with changes to CalcDifference that also add nsChangeHint_UpdateEffects when -moz-element changes. This needs changes to MaxDifference and maxHint nsStyleContext::CalcStyleDifference.
Assignee: nobody → mstange
Attachment #449487 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #451960 -
Flags: review?(dbaron)
Assignee | ||
Comment 26•14 years ago
|
||
Attachment #451961 -
Flags: review?(roc)
Assignee | ||
Comment 27•14 years ago
|
||
This is the heart of the operation. It needs lots of things from the dependent bugs.
Attachment #451962 -
Flags: review?(roc)
Assignee | ||
Comment 28•14 years ago
|
||
Compute the right size, call Acquire/ReleaseReferenceFrame at the right times, and delegate drawing to nsLayoutUtils::DrawPixelSnapped or nsSVGIntegrationUtils::DrawPaintServer.
Attachment #451963 -
Flags: review?(roc)
Assignee | ||
Comment 29•14 years ago
|
||
These are the reftests from Ryo's patch, adapted and culled until they passed. There are several instance where I compare scaled things to unscaled things at the same position, so the scaled version is blurred. In order to get rid of the blurring I apply an SVG threshold filter.
Attachment #451964 -
Flags: review?(roc)
Assignee | ||
Comment 30•14 years ago
|
||
Some more reftests I made. These test dynamic changes, multiple -moz-element background images, rendering sharpness and how fractional sizes are dealt with.
Attachment #451965 -
Flags: review?(roc)
Attachment #451960 -
Flags: review?(dbaron) → review-
Comment on attachment 451960 [details] [diff] [review] part 1, v4: parsing >+#define VARIANT_IMAGE (VARIANT_HUO | VARIANT_GRADIENT | \ >+ VARIANT_IMAGE_RECT | VARIANT_ELEMENT) I think it goes against the existing pattern to include VARIANT_INHERIT inside VARIANT_IMAGE. I'd rather you drop the INHERIT (H) and make the callers write VARIANT_IMAGE | VARIANT_INHERIT. >+ // If we detect a syntax error, we must match the opening parenthesis of the >+ // function with the closing parenthesis and skip all the tokens in between. This seems like documenting the obvious, since it comes right above SkipUntil(')'); I'd suggest removing the comment. nsStyleBackground::CalcDifference is wrong since it needs to check the background layers first, and loop through all layers before returning NS_STYLE_HINT_VISUAL, so that an early return doesn't return NS_STYLE_HINT_VISUAL when nsChangeHint_UpdateEffects is also needed. nsStyleImage::GetElementId should return |const PRUnichar*| rather than |const nsString|, which creates multiple unneeded temporaries. I'd like to look at the new version of nsStyleBackground::CalcDifference, so marking review-.
Updated•14 years ago
|
Keywords: dev-doc-needed
Attachment #451961 -
Flags: review?(roc) → review+
+ * @param aNominalSize how big the "image" should be, in appunits Name this aPaintServerSize and document it as well as you did in DrawableFromPaintServer? PaintFrameCallback doesn't repeat when aRepeat is true. Isn't that a bug?
Factor out the calls to nsLayoutUtils::GetGraphicsFilterForFrame in ImageRenderer::Draw? + if (mPaintServerFrame) { + mPaintingProperty->ReleaseReferencedFrame(); + } How can this already be non-null? Otherwise looks great!
Attachment #451964 -
Flags: review?(roc) → review+
Comment on attachment 451965 [details] [diff] [review] part 6: more reftests Change the 1.1 zoom factor to 1.2. We want 60/zoom to be an integer.
Attachment #451965 -
Flags: review?(roc) → review+
So with this patch, referencing a <canvas>, <video> or <img> that has a frame will include borders and padding in the rendering, but if it doesn't have a frame we won't get those. CSS scaling might also be applied if there's a frame, but not if there isn't. Some people might complain about that difference, but I guess it makes sense.
Assignee | ||
Comment 36•14 years ago
|
||
Attachment #451960 -
Attachment is obsolete: true
Attachment #463964 -
Flags: review?(dbaron)
Assignee | ||
Comment 37•14 years ago
|
||
updated to trunk
Attachment #451961 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #463965 -
Attachment is patch: true
Attachment #463965 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 38•14 years ago
|
||
Attachment #463966 -
Flags: review?(roc)
Assignee | ||
Comment 39•14 years ago
|
||
(In reply to comment #32) > + * @param aNominalSize how big the "image" should be, in appunits > > Name this aPaintServerSize and document it as well as you did in > DrawableFromPaintServer? Done. > PaintFrameCallback doesn't repeat when aRepeat is true. Isn't that a bug? It didn't have to; repeating is handled by gfxCallbackDrawable. I got rid of the aRepeat argument for gfxDrawingCallbacks in bug 572680.
Attachment #451962 -
Attachment is obsolete: true
Attachment #463967 -
Flags: review?(roc)
Attachment #451962 -
Flags: review?(roc)
Assignee | ||
Comment 40•14 years ago
|
||
(In reply to comment #33) > Factor out the calls to nsLayoutUtils::GetGraphicsFilterForFrame in > ImageRenderer::Draw? Done. > + if (mPaintServerFrame) { > + mPaintingProperty->ReleaseReferencedFrame(); > + } > > How can this already be non-null? It can't. Removed the check.
Attachment #451963 -
Attachment is obsolete: true
Attachment #463968 -
Flags: review?(roc)
Attachment #451963 -
Flags: review?(roc)
Assignee | ||
Comment 41•14 years ago
|
||
Attachment #463969 -
Flags: review?(roc)
Assignee | ||
Comment 42•14 years ago
|
||
Attachment #451964 -
Attachment is obsolete: true
Attachment #451965 -
Attachment is obsolete: true
Attachment #463966 -
Flags: review?(roc) → review+
Attachment #463967 -
Flags: review?(roc) → review+
Attachment #463968 -
Flags: review?(roc) → review+
Comment on attachment 463969 [details] [diff] [review] part 5: invalidate observers This is great! I think you need an additional change to add InvalidateDirectRenderingObservers to WebGLContext::Invalidate.
Attachment #463969 -
Flags: review?(roc) → review+
Assignee | ||
Comment 44•14 years ago
|
||
with that change
Attachment #463969 -
Attachment is obsolete: true
Comment on attachment 463964 [details] [diff] [review] part 1, v5: parsing I think I'd be more comfortable if nsStyleImage::mElementId owned a copy of the string rather than pointing into the style rule's data; I think pointing into the style rule's data is ok if nsStyleImage are always in style structs, but given the current configuration (and the lack of comments documenting a change) I'm guessing you didn't audit all uses of nsStyleImage and the strings they're passed to make sure the lifetimes are ok. So I'd prefer making mElementId PRUnichar* (no const, but keep the const in GetElementId), cloning the string in SetElementId, and freeing it in SetNull. (It might be worth filing a followup bug to see if we can make nsStyleImage stop copying and refcounting, though.) nsStyleBackground::CalcDifference is wrong; it needs to loop through all layers before deciding to return NS_STYLE_HINT_VISUAL. Also, in CalcDifference, could you make moreLayers and lessLayers pointers rather than references? r=dbaron with those changes, although I'd sort of like to look at the new CalcDifference.
Attachment #463964 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 46•14 years ago
|
||
Attachment #463964 -
Attachment is obsolete: true
Attachment #465331 -
Flags: review?(dbaron)
Comment on attachment 465331 [details] [diff] [review] part 1, v6: parsing nsStyleImage::GetElementId should still return |const PRUnichar*|; you don't want the caller messing with the data that the nsStyleImage owns. r=dbaron with that
Attachment #465331 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 48•14 years ago
|
||
Requesting approval on all patches in this bug, bug 572680, bug 572689 and bug 572688. From roc's email: > It adds significant new capabilities to the platform. It demos well. It will be > useful for theming and personas. It matches and surpasses Webkit's CSS canvas > backgrounds feature. The code will rot if we don't land it. It doesn't do > anything scary to our existing code. And Markus is quite capable of fixing any > issues that come up.
Attachment #465376 -
Flags: approval2.0?
Attachment #465376 -
Flags: approval2.0? → approval2.0+
Whiteboard: [needs landing]
Assignee | ||
Comment 49•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/5d5752f83c61 http://hg.mozilla.org/mozilla-central/rev/4c296b963aa4 http://hg.mozilla.org/mozilla-central/rev/1b2d8d134422 http://hg.mozilla.org/mozilla-central/rev/255b19177dd4 http://hg.mozilla.org/mozilla-central/rev/c799b49b3f26 http://hg.mozilla.org/mozilla-central/rev/4a50f3c34d5a http://hg.mozilla.org/mozilla-central/rev/9fd11a17eb1a
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b4
Comment 50•14 years ago
|
||
pushed additional changeset on mstange's request: http://hg.mozilla.org/mozilla-central/rev/27f263f62bfb
Comment 51•14 years ago
|
||
Now documented: https://developer.mozilla.org/en/CSS/-moz-element And listed on the CSS Reference's Mozilla extensions page and Firefox 4 for developers. Do let me know if I missed anything.
Keywords: dev-doc-needed → dev-doc-complete
Comment 52•12 years ago
|
||
I beg to add support for the values in CSS content and cursor. And it is not justice. Also, border-radius. I also ask to add CSS to mask your browser.
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•