340908 - Hook up SVG foreignobject to new invalidation architecture

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Now that the necessary invalidation hooks have landed, we just need to connect the dots to get foreignobject content to invalidate itself safely.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Here it is. We accumulate the damage region locally during reflow so we don't have to do any fancy stuff on every invalidate in the reflowing content.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This seems to work nicely; you can really browse inside foreignobject content now, edit text, etc.

There are of course some remaining issues:
-- scrolling doesn't really work
-- it's sloooooow when you're rotated (combination of things, I think: cairo clip masks making every operation roughly twice as slow, natural difficulties of rotating images etc, child native widgets being in random places unrelated to the actual content and all having to be repainted all the time)
-- it's not really clear how combobox dropdowns should work in rotated content. Probably we should go all-out and use our transparent widget code to draw a true rotated dropdown, that'd wow the crowd.
-- Seams are really obvious in rotated content. The Google home page is a case in point.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

Testcase ... browser widgety thing which you can transform

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: better testcase

This one makes better use of screen real estate.

Comment 5

[Takeshi Kurosawa :takenspc]

(In reply to comment #4)
> Created an attachment (id=224951) [edit]
> better testcase
> 
> This one makes better use of screen real estate.
> 

It doesn't look like the testcase :-).

When it implemented completly, it will become the strongest feature of Gecko !

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

oops

Comment 7

[tor]

Comment on attachment 224946 [details] [diff] [review]
fix

Unfortunally you'll need to update this patch now that the nsISVGRendererRegion removal has happened.

Why do you call PostReflowCommand() in DidModifySVGObservable?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #7)
> (From update of attachment 224946 [details] [diff] [review] [edit])
> Unfortunally you'll need to update this patch now that the nsISVGRendererRegion
> removal has happened.

Yeah, I know. I considered delaying my review for that and decided that would be unconscionable :-).

> Why do you call PostReflowCommand() in DidModifySVGObservable?

To ensure that the inner block (eventually) gets reflowed after our size has changed.

Comment 9

[tor]

(In reply to comment #8)
> > Why do you call PostReflowCommand() in DidModifySVGObservable?
> 
> To ensure that the inner block (eventually) gets reflowed after our size has
> changed.

Size updates go through AttributeChanged, which you have calling PostReflowCommand() as I'd expect.  The only thing a foreignObject frame should be getting modify messages for is a filter change, which doesn't seem like it should need to reflow the fO content, as the size isn't changing.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 224946 [details] [diff] [review]
fix

Looks nice!

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch

Updated to trunk + comment.

Comment 12

[tor]

Comment on attachment 225251 [details] [diff] [review]
updated patch

I think you'll need a call to UpdateGraphic in NotifyCanvasTMChanged.

You removed the call to UpdateCoveredRegion in DoReflow, which is the method that sets mRect.  UpdateCoveredRegion probably needs to be called in InitialUpdate and AttributeChanged.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated

Comment 14

[tor]

Comment on attachment 225335 [details] [diff] [review]
updated

With an UpdateCoveredRegion in NotifyCanvasTMChanged (hopefully soon to be gone), r=tor.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.