668163 - Broken overlay on top of Bing maps

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is broken across all platforms. George, would someone of your team be able to run a regression test? Would be kinda helpful.

Comment 2

[Alice0775 White]

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

Comment 3

[George Carstoiu]

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

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 5

[Alice0775 White]

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.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 7

[Alice0775 White]

Attachment: reduced sample

Comment 8

[Boris Zbarsky [:bzbarsky]]

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).

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

When you move around the map on the given Garmin connect page, you can see the moving clipping at the right and bottom side.

Comment 10

[Jonathan Watt [:jwatt]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.
======

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://www.w3.org/TR/2011/REC-CSS2-20110607/visudet.html#inline-replaced-width is more explicit about the behavior, too.

Comment 14

[Jonathan Watt [:jwatt]]

(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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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....

Comment 18

[Boris Zbarsky [:bzbarsky]]

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....

Comment 19

[Jonathan Watt [:jwatt]]

(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.

Comment 20

[Jonathan Watt [:jwatt]]

Err, didn't quite edit that reply as I meant to, but you get the idea.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> 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....

Comment 22

[Jonathan Watt [:jwatt]]

(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.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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).

Comment 24

[Jonathan Watt [:jwatt]]

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?

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: example of improved case

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.)

Comment 26

[Jonathan Watt [:jwatt]]

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.

Comment 27

[Jonathan Watt [:jwatt]]

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.

Comment 28

[Asa Dotzler [:asa]]

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.)

Comment 29

[u279076]

(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?

Comment 30

[u279076]

Attachment: Screenshot

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 32

[u279076]

(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.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2]]

No, at least not for me. It's still broken.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 36

[u279076]

(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.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think we should approach contacts at Bing to get them to fix the site.

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Though I'd also be ok with a patch that maps *explicit* height and width attributes into style.

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

jwatt, would you be able to write that patch?

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 41

[Jonathan Watt [:jwatt]]

Okay, great. I can do this, but not until next week.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: back out bug 611099 on mozilla-beta

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).

Comment 43

[LegNeato]

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?

Comment 44

[Christopher Blizzard (:blizzard)]

Adding dev-doc-needed since we're likely backing out something that's documented.

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 46

[Jet Villegas (inactive)]

Assigned to jwatt.

Comment 47

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0

Comment 48

[LegNeato]

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 49

[LegNeato]

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

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 51

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out on mozilla-aurora for Firefox 9:
https://hg.mozilla.org/releases/mozilla-aurora/rev/91108b393572

Comment 52

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 54

[Alex Keybl [:akeybl]]

[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?

Comment 55

[Daniel Holbert [:dholbert]]

(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.

Comment 56

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I merged the necessary backouts to Aurora 10; see bug 611099 comment 20.

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 58

[Jonathan Watt [:jwatt]]

I should have a patch for this this week.

Comment 59

[Jonathan Watt [:jwatt]]

Attachment: map <svg> width/height into style

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.

Comment 60

[Daniel Holbert [:dholbert]]

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?

Comment 61

[Jonathan Watt [:jwatt]]

Oops, I 'rm'ed them by mistake. Thanks!

Comment 62

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 63

[Jonathan Watt [:jwatt]]

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?

Comment 64

[Jonathan Watt [:jwatt]]

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.

Comment 65

[Jonathan Watt [:jwatt]]

Attachment: map <svg> width/height into style

I did change this a bit, so requesting review again.

Comment 66

[Jonathan Watt [:jwatt]]

(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.

Comment 67

[Jonathan Watt [:jwatt]]

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.

Comment 68

[Daniel Holbert [:dholbert]]

(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)

Comment 69

[Boris Zbarsky [:bzbarsky]]

> 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 70

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 71

[Jonathan Watt [:jwatt]]

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

Comment 72

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/966b11b4940d

Comment 73

[j.j.]

Removing misleading keywords. This needs to land on Aurora 11, no?

Comment 74

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Beta is fine, so re-adding probably accidentally removed keyword. But yes, we have to fix it in Aurora.

Comment 75

[Alex Keybl [:akeybl]]

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.

Comment 76

[Jonathan Watt [:jwatt]]

Thanks. Pushed to Aurora in:

https://hg.mozilla.org/releases/mozilla-aurora/rev/d2a10fe0d30f

Comment 77

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 78

[Eric Shepherd [:sheppy]]

This actually had not yet been documented.