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)

enhancement
Not set
normal

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
Attached patch patch v1 (obsolete) — Splinter Review
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()).
Attached patch reftests v1 (obsolete) — Splinter Review
Reftests for patch v1.

(I brought them forward from roc's repo and replaced url(#id) with -moz-element(#id)).
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.
Attached patch patch v2 (obsolete) — Splinter Review
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.
Blocks: 453063
Version: unspecified → Trunk
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
No longer blocks: 453063
Ryo, do you think you'll have time to finish this patch, or do you want somebody else to take over?
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.
Blocks: 547805
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.
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.
(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.
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?
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?
Attached patch part 1, v1: style system (obsolete) — Splinter Review
Attachment #448606 - Flags: review?(dbaron)
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.
Attached patch part 1, v2: style system (obsolete) — Splinter Review
(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)
(In reply to comment #15)
> Markus: we could put shared snapping logic in gfx maybe?

Moving it into gfxUtils seems to work, thanks.
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.
Attached patch part 1, v3 (obsolete) — Splinter Review
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)
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)
Depends on: 572680
Depends on: 572688
Depends on: 572689
Depends on: 572697
Attached patch part 1, v4: parsing (obsolete) — Splinter Review
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)
This is the heart of the operation. It needs lots of things from the dependent bugs.
Attachment #451962 - Flags: review?(roc)
Attached patch part 4: -moz-element drawing (obsolete) — Splinter Review
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)
Attached patch part 5: Ryo's reftests (obsolete) — Splinter Review
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)
Attached patch part 6: more reftests (obsolete) — Splinter Review
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)
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-.
Keywords: dev-doc-needed
+   * @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!
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.
Blocks: 276431
Attached patch part 1, v5: parsing (obsolete) — Splinter Review
Attachment #451960 - Attachment is obsolete: true
Attachment #463964 - Flags: review?(dbaron)
updated to trunk
Attachment #451961 - Attachment is obsolete: true
Attachment #463965 - Attachment is patch: true
Attachment #463965 - Attachment mime type: application/octet-stream → text/plain
(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)
(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)
Attached patch part 5: invalidate observers (obsolete) — Splinter Review
Attachment #463969 - Flags: review?(roc)
Attached patch part 6: reftestsSplinter Review
Attachment #451964 - Attachment is obsolete: true
Attachment #451965 - Attachment is obsolete: true
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+
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+
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+
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?
pushed additional changeset on mstange's request: http://hg.mozilla.org/mozilla-central/rev/27f263f62bfb
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.
Depends on: 591141
Depends on: 591832
Depends on: 592086
Depends on: 601999
Depends on: 587336
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.
Depends on: 591304
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.