Closed Bug 668163 Opened 13 years ago Closed 12 years ago

Broken overlay on top of Bing maps

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla12
Tracking Status
firefox7 + unaffected
firefox8 + unaffected
firefox9 --- unaffected
firefox10 + unaffected
firefox11 --- fixed
firefox12 --- verified

People

(Reporter: whimboo, Assigned: jwatt)

References

()

Details

(Keywords: regression, verified-aurora, verified-beta, Whiteboard: [qa?])

Attachments

(5 files, 1 obsolete file)

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110628 Firefox/7.0a1

Some overlays aren't correctly displayed anymore with the latest Nightly builds at least on OS X. When you open the referenced URL you should see a complete round visualized by a red stroke. In Nightly builds it's broken up into a single short line.

That's a regression compared to Aurora.
This is broken across all platforms. George, would someone of your team be able to run a regression test? Would be kinda helpful.
OS: Mac OS X → All
Hardware: x86 → All
Version: 5 Branch → Trunk
Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/4c05a86b4fe3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110610 Firefox/7.0a1 ID:20110612183729
Fails:
http://hg.mozilla.org/mozilla-central/rev/eab02b0b7c7d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110612 Firefox/7.0a1 ID:20110612185604
pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4c05a86b4fe3&tochange=eab02b0b7c7d
That's strange. Using the mozregression tool, I got the following regression, different from the one in Comment 2.

Regression window:

Last good nightly: 2011-06-12
First bad nightly: 2011-06-13

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e37011790a8a&tochange=bbbc80a2bd8c
Thanks both of you. I have tested the builds around those dates and I can confirm that the regression is between 06-12 and 06-13.

I would assume that one of the check-ins from David Baron have been caused this regression. I will do a bisect to get the exact changeset.
In addition to comment #2, the range is from m-c hourly.

I bisected in local build,
build from 3af9fed4e33a : fails
build from 58fe3ede72f8 : fails
build from c07445f34e92 : works
build from db0bcc6ab526 : works
Triggered by 
58fe3ede72f8	L. David Baron — Remove notion of percentage intrinsic size: remove the single case that (incorrectly) sets percentage intrinsic sizes, and fix all of the tests that depend on our old incorrect behavior. (Bug 611099) r=dholbert Needed to help CSS 2.1 meet Proposed Recommendation entrance criteria.
Thanks Alice. That saves me a lot of time.

David, does it mean the website is broken and should update its code to support the latest agreements for CSS 2.1? Or is it a real failure?
Blocks: 611099
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
Attached file reduced sample
So the basic issue is that the <svg> ends up a different (smaller) size now and clips the picture (this last part is expected), right?

WebKit seems to use 100% of the parent as the width/height of the <svg> here.. but that may be due to some sort of style mapping, not intrinsic size stuff.  Not sure what IE does yet (don't have it here offhand).
When you move around the map on the given Garmin connect page, you can see the moving clipping at the right and bottom side.
So the code is:

<div align="left" style="position: absolute; z-index: 60; top: 0px; left: 0px; width: 1149px; height: 570px;">
  <svg height="100%" width="100%">
</div>

Seems to me that the 300px x 150px fallback should only be happening when the percentages can't be resolved, and since the containing block's dimensions do not depend on its <svg> child, that's not the case here. So this seems like genuine undesirable breakage to me.
Well, it's what the specs call for at the moment....  I agree it's sort of weird; I was never quite clear on why the concept of intrinsic percentage sizing went away.

Maybe what should happen is that if width/height are % for SVG they should get mapped into style?  That would make inline SVG work, at least, though linked SVG still wouldn't work.
I think this is the unfortunate result of SVG saying that the width and height attributes have default values of "100%"; adding them explicitly doesn't do anything.  And I think our new behavior does match the relevant specs -- the percentage values of the width and height attributes, as far as I can tell, are not supposed to do anything when the conditions in the list in http://www.w3.org/TR/2011/PR-SVG11-20110609/coords.html#ViewportSpace are true:

======
The ‘width’ attribute on the outermost svg element establishes the viewport's width, unless the following conditions are met:

    the SVG content is a separately stored resource that is embedded by reference (such as the ‘object’ element in XHTML [XHTML]), or the SVG content is embedded inline within a containing document;
    and the referencing element or containing document is styled using CSS [CSS2] or XSL [XSL];
    and there are CSS-compatible positioning properties ([CSS2], section 9.3) specified on the referencing element (e.g., the ‘object’ element) or on the containing document's outermost svg element that are sufficient to establish the width of the viewport.

Under these conditions, the positioning properties establish the viewport's width.
======
(In reply to comment #12)
> adding them explicitly doesn't do anything.

I'm not sure what you mean here. If I have:

    <div style="width:500px; height:500px; background:red;">
      <div style="width:100%; height:100%; background:lime;"/>
    </div>

then the lime div fills the red div. Seems intuitive. Great.

So if you replace the div with <svg width="100%" height="100%">, why should the <svg> not do the same thing? Making that collapse to the 300px x 150px default seems completely counter-intuitive, and something that the CSS WG should not have broken. When you say "adding them explicitly doesn't do anything", that seems to only be because the CSS WG changed what the spec said should happen, and you implemented the change.

I like the idea of mapping outer-<svg> 'width' and 'height' into style BTW.
> why should the <svg> not do the same thing?

Because per the SVG spec as it stands right now, <svg width="100%" height="100%"> and <svg style="width: 100%; height: 100%"> mean different things.  The latter would behave as your <div> example, and for the same reason...
(In reply to comment #14)
> I like the idea of mapping outer-<svg> 'width' and 'height' into style BTW.

I'd be ok with doing that *if* it doesn't apply to the "default" values that SVG says are there (and if the spec says to do it).

The fact that our old behavior did apply to such default values was one of the most confusing things about using SVG in HTML -- for example, SVG elements that *do* have a viewBox used to have that viewBox's intrinsic ratio ignored when they were inside a container with a fixed height (such that percentage heights were meaningful), but the intrinsic ratio was honored if they were in an auto-height container.
But fundamentally, the problem is that people want the <svg>'s height/width attributes to do double duty.  They want to use them to express an intrinsic size for the SVG (akin to the actual size data in a raster image) and to somehow do something dealing with style (only when the SVG is inline?) like the width/height attributes of <html:img>.

Unfortunately, those are not necessarily quite compatible....
For what it's worth, implementing mapping height/width to style only if the attributes are explicitly set would be pretty trivial for us.  Getting the SVG working group to sign off on that, I'm not sure.  I'm also not sure whether it generally gives the right behavior, because I've completely lost track of the interactions of viewbox and styles and these attributes....
(In reply to comment #15)
> Because per the SVG spec as it stands right now, <svg width="100%"
> height="100%"> and <svg style="width: 100%; height: 100%"> mean different
> things.  The latter would behave as your <div> example, and for the same
> reason...

Sure, but the former, a common case, used to /behave/ the same too, although, yes, the details as to why were different.

I'm concerned about the fact that something that used to work intuitively without knowledge of the arcane spec details of sizing has now turned into a bang-your-head-on-the-desk why-on-earth-is-this-not-doing-what-it-so-obviously-should-do case for authors that they so often have to deal with.

(In reply to comment #16)
> (In reply to comment #14)
> > I like the idea of mapping outer-<svg> 'width' and 'height' into style BTW.
> 
> I'd be ok with doing that *if* it doesn't apply to the "default" values that
> SVG says are there (and if the spec says to do it).

Ok, maybe. And what's the plan in the meantime? Ok. Seems to me that the removal of percentage intrinsic width/height needs to wait until that time then.
Err, didn't quite edit that reply as I meant to, but you get the idea.
> Sure, but the former, a common case, used to /behave/ the same too

Actually, no.  It sometimes behaved the same and sometimes not....

> without knowledge of the arcane spec details of sizing

Those details are sort of forced in the the situation by the problem described in comment 17....
(In reply to comment #21)
> > Sure, but the former, a common case, used to /behave/ the same too
> 
> Actually, no.  It sometimes behaved the same and sometimes not....

The "common case" I was referring to is the one at hand, specifically 100% x 100% without a viewBox inline in a CB that doesn't rely on its content's dimensions.

> > without knowledge of the arcane spec details of sizing
> 
> Those details are sort of forced in the the situation by the problem
> described in comment 17....

In general the details were worked out to pretty much hide the details from authors and make things work intuitively most of the time. Yes, the problem mentioned in comment 17 is real, but it seems to me that the ugly details are now less hidden than before.
(In reply to comment #22)
> (In reply to comment #21)
> > > Sure, but the former, a common case, used to /behave/ the same too
> > 
> > Actually, no.  It sometimes behaved the same and sometimes not....
> 
> The "common case" I was referring to is the one at hand, specifically 100% x
> 100% without a viewBox inline in a CB that doesn't rely on its content's
> dimensions.

To some extent, there's a tradeoff here.  A large number of with-viewBox cases  behave much *more* sensibly now, and those cases were largely a disaster before (and exposed arcane spec details).
So apparently I asked you for details of these cases here:

http://lists.w3.org/Archives/Public/www-style/2010Nov/0154.html

but didn't get a reply. Would you be able to expand on some specific cases you're seeing people trying to use and getting confused?
Prior to this fix, these two examples were substantially different -- there was a huge extra space around the second example.  (Similar things happen in tables, but they're even more confusing.)
Thanks for the testcase. Having a concrete example to focus on helps.

For the benefit of anyone interested in this bug who isn't so familiar with the details of the SVG/CSS specs, first let me summarize what's going on in the two examples in your testcase...

The two examples in the testcase are the same, except that whereas in the first example the containing block for the <svg> does not have a fixed height, the <svg> in the second example does.

The first of the two examples (containing block without a fixed height) behaves the same both before and after the changes to remove support for percentage intrinsic dimensions from replaced elements. The used values for the width and height end up being 400px and 100px respectively. Both pre- and post-change the height can't be resolved directly, so the 4:1 aspect ration in the viewBox is used to calculate the used height from the 400px computed width, resulting in a used height of 100px. The details of how this happens pre- and post-change are subtly different though. Pre-change the reason that the intrinsic height can't be resolved is because, although the 100% default is recognized as an intrinsic dimension and essentially trumps the CSS 'height' property's computed value of 'auto', the <svg>'s containing block does not have a fixed height, thus there's nothing to resolve the 100% against. Post-change, the 100% default value for the height attribute isn't recognized, so again the aspect ratio is used to resolve the height as pre-change.

The second example (where the containing block has a fixed height independent of its content) behaves differently pre- and post-change. Pre-change the default value of 100% for the height attribute is recognized as its intrinsic height, and resolved against the 200px height of the <svg>'s containing block, to 200px. Post-change, percentage intrinsic dimensions no longer exist, so the CSS 'height' property's computed value of 'auto' means the 4:1 aspect ratio defined by the viewBox is used to resolve the used height from the 400px width, which gives a used value of 100px.
So essentially what the CSS change seems to have been motivated by is inline-<svg> in a fixed dimension containing block where the author wants to say "compute this dimension, then size the other dimension proportionally".

Does that sound right?

I agree it's desirable to make this "proportional sizing" possible in fixed size containing blocks. However, to support this previously unworking use case we don't need to - and shouldn't IMO - unnecessarily break existing content where inline-<svg> has /explicitly/ set the width/height /attribute(s)/ to percentage values in fixed size containing blocks. (Which is arguably a more common use case than proportional sizing anyway.)

Besides breaking existing content, to me, having inline <svg height="X%"> behave differently to <svg style="height:X%"> in anything other than rare edge cases is unacceptably confusing for authors. This wasn't really the case before...the whole "<svg> is a replaced element and its width/height are 'intrinsic' dimensions" hack in the SVG spec only exists because mapping the attributes into style isn't straightforward, and because, with support for percentage intrinsic dimensions, setting the width/height attributes happens to appear to behave the same as setting the width/height CSS properties in permutations of content that authors are likely to come up with in the real world. There are edge cases (mostly contrived) where this is not the case (and I didn't like that), but the vast majority of authors would never notice SVG displaying differently to how it would display if the attributes were mapped into style before the change that caused this bug. Unfortunately, with percentage intrinsic dimensions removed, that's no longer the case.

I'm not necessarily against fixing the "proportional sizing in fixed size containing block" case by dropping support for percentage intrinsic dimensions. I just think it should be done at the same time as mapping /explicit/ width/height attributes into style to avoid breaking content and adding "setting the width/height attributes does different things to setting the width/height properties" insanity. I'm still mulling over what the SVG WG will need to change to handle mapping into style though.

With regards to the /default/ values, I agree that 100% is not a good default for inline-<svg>, and mapping the default 100% values into style would recreate the arguably broken behavior that the change to the CSS spec is trying to fix.
Would be nice if QA could see if this is still a problem on Bing Maps. 

Jwatt and Dbaron, is there anything we could or should do here for Firefox 7 (on Aurora today which uplifts to Beta in less than a week.)
Keywords: qawanted
(In reply to Asa Dotzler [:asa] from comment #28)
> Would be nice if QA could see if this is still a problem on Bing Maps. 
> 
> Jwatt and Dbaron, is there anything we could or should do here for Firefox 7
> (on Aurora today which uplifts to Beta in less than a week.)

Mozilla/5.0 (X11; Linux x86_64; rv:7.0a2) Gecko/20110811 Firefox/7.0a2

I don't get the map -- all other content loads -- is the problem as originally reported?
Attached image Screenshot
It's still there and broken with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a2) Gecko/20110811 Firefox/7.0a2
Keywords: qawanted
(In reply to Henrik Skupin (:whimboo) from comment #31)
> It's still there and broken with Mozilla/5.0 (Macintosh; Intel Mac OS X
> 10.6; rv:7.0a2) Gecko/20110811 Firefox/7.0a2

Henrik can you try again? When I tried at 2:56pm today it was not working. However, I just tried it now (some 9 hours later) and the map overlay works (exactly the same build -- in fact the same session) as before. I'm wondering if something server side (or Bing side) fixed this issue.
No, at least not for me. It's still broken.
Since the merge last week this regression is now even present in Firefox Beta. How can we get further progress? What are alternatives? Anything we can do in the next 4 weeks to prevent shipping it to our release audience?
(In reply to Henrik Skupin (:whimboo) from comment #35)
> Since the merge last week this regression is now even present in Firefox
> Beta. How can we get further progress? What are alternatives? Anything we
> can do in the next 4 weeks to prevent shipping it to our release audience?

Adding status-firefox7:affected, hopefully this raises the attention level. I agree, this should be fixed for Firefox 7 if at all possible.
I think we should approach contacts at Bing to get them to fix the site.
Though I'd also be ok with a patch that maps *explicit* height and width attributes into style.
jwatt, would you be able to write that patch?
(In reply to David Baron [:dbaron] from comment #37)
> I think we should approach contacts at Bing to get them to fix the site.

Actually, never mind this; I think jwatt is right, and we should just map width/height into style.

Mapping width/height into style should be safe behavior-wise because its effects on behavior amount to a partial backout of bug 611099 and because it should make us match IE9 (which appears to do the same), and it should be safe code-wise because it's effectively just adding two attributes to a list that we already have.
Okay, great. I can do this, but not until next week.
Given the date, I don't think trying to get that fix in for beta is realistic anymore, so here's a backout patch series (backing out all of bug 611099) for beta.  It passes try (for a limited set of tests; I didn't run everything).
Attachment #557672 - Flags: approval-mozilla-beta?
Comment on attachment 557672 [details]
back out bug 611099 on mozilla-beta

Backout approved for mozilla-beta. This should probably be backed out everywhere, right?
Attachment #557672 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Adding dev-doc-needed since we're likely backing out something that's documented.
Keywords: dev-doc-needed
Backed out on beta.  Still need to get to aurora; hoping jwatt comes up with a patch for central soon so it won't be a problem there.
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=9b664712d5f4
Assigned to jwatt.
Assignee: nobody → jwatt
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0
Whiteboard: [qa?]
We backed out bug 611099 from beta8, so Firefox 8 is unaffected by this bug (as was 7 because the backout was there as well)
Comment on attachment 557672 [details]
back out bug 611099 on mozilla-beta

Approving it for bookkeeping, even though it was transplanted as part of the source migration
Attachment #557672 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Christian Legnitto [:LegNeato] from comment #49)
> Comment on attachment 557672 [details]
> back out bug 611099 on mozilla-beta
> 
> Approving it for bookkeeping, even though it was transplanted as part of the
> source migration

That was on beta, but I'm going to assume you're ok with aurora too.
Verified fixed with the back-out on aurora and beta:

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a2) Gecko/20111004 Firefox/9.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101 Firefox/8.0
Blocks: 694863
Blocks: 694991
[Triage Comment]
This doesn't appear to have been backed out of m-c in time for the Aurora cut over. Is there any reason we didn't back out there?
(In reply to Alex Keybl [:akeybl] from comment #54)
> [Triage Comment]
> This doesn't appear to have been backed out of m-c in time for the Aurora
> cut over

AFAIK, there was never a m-c backout, and the fact that it missed aurora is known (and needs followup work -- see bug 611099 comment 17, bug 611099 comment 18).

RE whimboo's "verified-aurora" keyword removal -- IIUC, this *was* originally fixed-by-backout & verified on at-the-time-aurora, but it's not fixed on *current* aurora (since there was never a m-c backout).  It seems that "verified-aurora" is a temporally-ambiguous keyword, sadly. :-/  I trust that whimboo knows what he's doing -- just thought it was worth clarifying.
Re-adding verified-aurora keyword because it works again with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a2) Gecko/20111204 Firefox/10.0a2. Not sure how to handle the keyword in the future as long as the underlying issue is not fixed and we will get the broken behavior again and again on the aurora branch.
Keywords: verified-aurora
I should have a patch for this this week.
Status: NEW → ASSIGNED
This is actually really trivial since nsSVGElement::UpdateContentStyleRule only maps set attributes as it turns out (thanks bz).

I'm still working on overhauling the 'sizing' tests directory to delete and simplify that stuff now that our implementation strategy is a lot simpler. This patch should be enough to stop us from having to keep backing out bug 611099 though.
Attachment #581300 - Flags: review?(dbaron)
Comment on attachment 581300 [details] [diff] [review]
map <svg> width/height into style

>--- a/layout/reftests/svg/sizing/reftest.list
>+++ b/layout/reftests/svg/sizing/reftest.list
>@@ -298,17 +298,13 @@ random-if(Android) == object--auto-auto-
>-# These two don't have a whole lot of point anymore now that the meaning
>-# of percentages has changed.
>-== dynamic--inline-resize-cb-height.xhtml       standalone-sanity-height-150px.svg
>-== dynamic--inline-resize-cb-width.xhtml        standalone-sanity-width-300px.svg

Presumably we'd want to "hg rm" those test files, too, right?
Oops, I 'rm'ed them by mistake. Thanks!
Comment on attachment 581300 [details] [diff] [review]
map <svg> width/height into style

r=dbaron, but remove the try syntax from the commit message

Also, it seems like this maps values that are valid CSS but not valid SVG (auto, -moz-fit-content, etc. -- anything width takes other than lengths and percentages).  Maybe that's not a problem, though.


Isn't this going to require modifying a bunch of other testcases?  (I'd expect it to allow backing out many of the testcase changes done for bug 611099 -- though maybe it doesn't require it.  Even so, we probably should revert the ones we can.)
Attachment #581300 - Flags: review?(dbaron) → review+
I have one outstanding problem that I've not figured out. Right now I'm getting a failure of the assertion "Why did this not get handled while processing mRestyleRoots?" in CollectRestyles:

https://hg.mozilla.org/mozilla-central/annotate/f98c57415d8d/layout/base/RestyleTracker.cpp#l109

This happens when running layout/svg/crashtests/386566-1.svg

REFTEST TEST-START | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/svg/crashtests/386566-1.svg | 1508 / 1990 (75%)
###!!! ASSERTION: Why did this not get handled while processing mRestyleRoots?: '!element->HasFlag(collector->tracker->RootBit()) || (element->GetFlattenedTreeParent() && (!element->GetFlattenedTreeParent()->GetPrimaryFrame()|| element->GetFlattenedTreeParent()->GetPrimaryFrame()->IsLeaf())) || (aData.mChangeHint & nsChangeHint_ReconstructFrame)', file layout/base/RestyleTracker.cpp, line 121
mozilla::css::CollectRestyles [RestyleTracker.cpp:109]
nsBaseHashtable<nsISupportsHashKey, mozilla::css::RestyleTracker::RestyleData, mozilla::css::RestyleTracker::RestyleData>::s_EnumStub [nsBaseHashtable.h:415]
PL_DHashTableEnumerate [pldhash.cpp:755]
nsBaseHashtable<nsISupportsHashKey, mozilla::css::RestyleTracker::RestyleData, mozilla::css::RestyleTracker::RestyleData>::Enumerate [nsBaseHashtable.h:240]
mozilla::css::RestyleTracker::DoProcessRestyles [nsBaseHashtable.h:245]
nsCSSFrameConstructor::ProcessPendingRestyles [nsCSSFrameConstructor.cpp:11670]
PresShell::FlushPendingNotifications [nsPresShell.cpp:4068]
nsRefreshDriver::Notify [nsRefreshDriver.cpp:397]
nsTimerImpl::Fire [nsTimerImpl.cpp:437]
nsTimerEvent::Run [nsTimerImpl.cpp:524]
nsThread::ProcessNextEvent [nsThread.cpp:660]
NS_ProcessNextEvent_P [nsThreadUtils.cpp:245]
mozilla::ipc::MessagePump::Run [MessagePump.cpp:111]
MessageLoop::RunInternal [message_loop.cc:208]
MessageLoop::Run [message_loop.cc:487]
nsBaseAppShell::Run [nsBaseAppShell.cpp:191]
nsAppStartup::Run [nsAppStartup.cpp:221]
XRE_main [nsAppRunner.cpp:3525]

This failure seems to be purely a result of adding 'width' and 'height' to IsAttributeMapped.

Would you have any pointers bz?
Jet wondered if the testcase can be made to assert without my patch if the setAttribute call sets an attribute that is currently mapped into style. Indeed it can. The assert fails using current m-c builds simply by making the following change to the testcase:

-  document.documentElement.setAttribute('height', "5px");
+  document.documentElement.setAttribute('stroke', "hotpink");

So mapping 'height' into style has simply uncovered an existing bug.

Perhaps for the purposes of avoiding backing out bug 611099 again we should just disable that test and check in my patch.
I did change this a bit, so requesting review again.
Attachment #581300 - Attachment is obsolete: true
Attachment #583218 - Flags: review?(dbaron)
(In reply to David Baron [:dbaron] from comment #62)
> Also, it seems like this maps values that are valid CSS but not valid SVG
> (auto, -moz-fit-content, etc. -- anything width takes other than lengths and
> percentages).  Maybe that's not a problem, though.

Probably not a big deal, and it's something we'll want to do long term, but since the spec doesn't actually say to yet I've changed the patch to prevent that.

> Isn't this going to require modifying a bunch of other testcases?  (I'd
> expect it to allow backing out many of the testcase changes done for bug
> 611099 -- though maybe it doesn't require it.  Even so, we probably should
> revert the ones we can.)

It doesn't require it after the changes you made in bug 611099, but yes, it should be done and I'm working on that. I'll do that in a separate followup bug though since there's no need to hold up this one.
Please also indicate during review whether I should do something to ignore the assert in layout/svg/crashtests/386566-1.svg before checking in, or whether we should do something else.
(side note: the IE TestDrive helicopter demo at
  http://ie.microsoft.com/testdrive/Performance/Helicopter/Default.xhtml
is also broken by this bug, in wide browser-windows, and jwatt says his patch fixes it)
> So mapping 'height' into style has simply uncovered an existing bug.

So what seems to happen is the following:

1)  Any style computation on any SVG element always triggers an animation restyle on that element (see nsSVGElement::WalkContentStyleRules).

2)  Having a pending XBL binding causes us to not create child frames for an element.

So we end up with pending animation restyles for the rect and the <svg> that we post while restyling it.  Then we process animation restyles starting at the animation restyle root (the <svg>).  This has no child frames because of the XBL thing, so we don't process the animation restyle for the rect.

It's not clear to me why we have that animation restyle request to start with... but we could probably handle the XBL case a bit better.
Comment on attachment 583218 [details] [diff] [review]
map <svg> width/height into style

r=dbaron, though I didn't delve into the GetAnimationLength()->HasBaseVal() magic.

Given the analysis above, I think it's ok to annotate the test as asserting, as long as you make sure there's a bug filed on the issue (and stick the bug number in the reftest.list).
Attachment #583218 - Flags: review?(dbaron) → review+
Blocks: 713626
Boris: thanks a lot for the debugging!

David: Thanks for review! I filed bug 713626 for the uncovered assertion failure, and I filed (and took) bug 713627 for simplifying the 'sizing' tests.

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/966b11b4940d
https://hg.mozilla.org/mozilla-central/rev/966b11b4940d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Removing misleading keywords. This needs to land on Aurora 11, no?
Beta is fine, so re-adding probably accidentally removed keyword. But yes, we have to fix it in Aurora.
Keywords: verified-beta
Comment on attachment 583218 [details] [diff] [review]
map <svg> width/height into style

[Triage Comment]
Sounds like this was fixed everywhere but FF11. Let's fix there as well.
Attachment #583218 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified on Aurora with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0a2) Gecko/20120108 Firefox/11.0a2

Also marking as verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0a1) Gecko/20120102 Firefox/12.0a1. I don't think a manual test is necessary here.
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: in-litmus-
Keywords: verified-aurora
This actually had not yet been documented.
Keywords: dev-doc-needed
Depends on: 736431
Depends on: 764851
Depends on: 766956
You need to log in before you can comment on or make changes to this bug.