Closed Bug 1255276 Opened 4 years ago Closed 4 years ago

setting transform on <g> element makes it disappear and triggers "ASSERTION: InitialOverflowProperty must be set first"

Categories

(Core :: SVG, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jryans, Assigned: dholbert)

References

()

Details

(Keywords: assertion, regression, testcase)

Attachments

(4 files, 5 obsolete files)

STR:

1. Open http://cpettitt.github.io/project/dagre-d3/latest/demo/user-defined.html
2. Highlight the text "house" inside the house shape near the top
3. Double click to edit the `transform="translate(-19.399999618530273,-8.5)` attribute two nodes above the text
4. Change the value to empty value and confirm the change
5. Press Cmd-Z to undo

ER:

The node should return to its original position and the attribute value should be restored.

AR:

The attribute does get restored in the markup view...  but the node disappears entirely from view!
Comment on attachment 8728789 [details]
MozReview Request: Bug 1255276 - Undoing SVG attribute change shouldn't hide node. r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39125/diff/1-2/
Attachment #8728789 - Attachment description: MozReview Request: Bug 1255276 - Undoing SVG attribute change shouldn't hide node. r=pbro → MozReview Request: Bug 1255276 - Undoing SVG attribute change shouldn't hide node. r=pbrosset
Attachment #8728789 - Flags: review?(pbrosset)
Not sure how to test this...  Maybe you have an idea.  AFAICT, the attributes are always present in the DOM with or without my changes...
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [btpp-fix-now]
Comment on attachment 8728789 [details]
MozReview Request: Bug 1255276 - Undoing SVG attribute change shouldn't hide node. r=pbrosset

https://reviewboard.mozilla.org/r/39125/#review35799

::: devtools/client/inspector/markup/markup.js:2888
(Diff revision 2)
> +        undoMods.removeAttribute(attribute.name);

I don't understand why this fixes it, and I don't understand why the problem is happening the first place.

Let me explain what I do know:

undoMods contains a series of modifications we're going to do to the node when the user presses ctrl+Z.

Without your fix, we were pushing 2 modifications to it:
- first, `undoMods.setAttribute(name, oldValue)` when calling `_saveAttribute` at line 2886
- then again, `undoMods.setAttribute(name, oldValue)` when calling `_applyAttributes` line 2888 (which, in turn, calls `_saveAttribute`).

So, we would set the attribute to its old value twice in a row.
Is this what causes the node to disappear?
The weird thing is, as you said, the attribute is in the DOM. If you query it from the console, it is there.
Why is the text not shown?

So, I was thinking, what would happen if we would set the attribute only once when undoing? This seemed like a better fix, right?
So, just remove the call to `_saveAttribute` at line 2886, so that `undoMods` only sets the old value once.
Turns out that doesn't fix anything.
But since I don't understand why the text disappears in the first place, I don't know why.

With your fix in, we now have 3 opeartions in `undoMods`:
- first, `undoMods.setAttribute(name, oldValue)` when calling `_saveAttribute` at line 2886
- then, your fix, line 2888, `undoMods.removeAttribute`,
- and finally, again, `undoMods.setAttribute(name, oldValue)` when calling `_applyAttributes` line 2888 (which, in turn, calls `_saveAttribute`).

So, setting the attribute, removing it, and setting it again fixes the issue?

Maybe I'm missing something terribly obvious, and I apologize if that's the case, but I can't see it.
Attachment #8728789 - Flags: review?(pbrosset)
Patrick and I both worked on creating a minimal test case for this.  Here's my version:

https://jsbin.com/yohop/17/edit?html,js,output

I seem to have angered JSBin, it only works when the output is in a separate frame for me.

So far it feels like some kind of layout or rendering race condition.  If I check Chrome Canary, everything seems to work as expected.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> I seem to have angered JSBin, it only works when the output is in a separate
> frame for me.
Indeed, I have the same problem too, so this URL works fine: https://output.jsbin.com/yohop
:dholbert, it looks like we've stumbled on some kind of edge case in SVG layout where mutating attributes like "transform" from script can cause content to disappear from the screen incorrectly.  See comments 5 and 6 for a reduced test case on JSBin demonstrating the issue.

Do you know who can investigate this?
Flags: needinfo?(dholbert)
[Yeah, this looks like a Core|SVG bug --> morphing.]

I can reproduce, using https://output.jsbin.com/yohop/17 (which is the URL I get from loading comment 5's testcase & clicking arrow)

This works correctly in old builds (e.g. 2014-01-01 Nightly), and mozregression gives this regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c962bde5ac0b&tochange=ac376a4e8174

That includes bug 991545, which is an optimization to make us do less stuff when the "transform" attribute changes on SVG content. Looks like that optimization must not have been quite correct.
Blocks: 991545
Component: Developer Tools: Inspector → SVG
Flags: needinfo?(dholbert)
Keywords: regression
Product: Firefox → Core
I'll poke at this a bit and see if I can figure out what's broken...  [stealing from jryans]
Assignee: jryans → dholbert
Whiteboard: [btpp-fix-now]
When I reproduce the bug in a debug build (using the jsbin URL as well as the original URL from comment 0), I get this assertion-failure:

###!!! ASSERTION: InitialOverflowProperty must be set first.: 'frame->Properties().Get( nsIFrame::DebugInitialOverflowPropertyApplied())', file layout/base/RestyleTracker.h, line 133
Attached file testcase 1
Keywords: assertion
Summary: Undoing attribute change fails for SVG node → setting transform on <g> element makes it disappear and triggers "ASSERTION: InitialOverflowProperty must be set first"
Keywords: testcase
(For the record, the assertion that we're tripping here (comment 10) was added in bug 984226.)
The problem here actually goes back to an older patch being not-quite-correct, on Bug 861188:
https://hg.mozilla.org/mozilla-central/diff/f1e5eb8741ee/content/svg/content/src/SVGTransformableElement.cpp#l65

In that bug, we changed to treat "attribute modified" notifications as frame-reconstruct triggers, iff there was an existing nonempty transform:
>      if (aModType == nsIDOMMutationEvent::ADDITION ||
> -        aModType == nsIDOMMutationEvent::REMOVAL) {
> +        aModType == nsIDOMMutationEvent::REMOVAL ||
> +        (aModType == nsIDOMMutationEvent::MODIFICATION &&
> +         !(mTransforms && mTransforms->HasTransform()))) {
>        // Reconstruct the frame tree to handle stacking context changes:

*However* - the check for an existing nonempty transform there (the 2nd-to-last line quoted above) is actually broken, because this code runs *after* we've already populated mTransforms with the value that's currently being set.

So in the attached testcase, when we trigger the bug -- going from transform="" to transform="translate(20,20)" -- this condition "mTransforms && mTransforms->HasTransform()" evaluates to true, because mTransforms->mBaseVal already has the "translate(20,20)" transform included.

So we skip this frame-reconstruction scenario (which we really should be triggering, because we're really adding a transform where there was previously none) and take the optimized update-existing-transform fast-path. And that makes us spam the assertion noted above because there's no existing transform, I think.
Depends on: 861188
So really, we'd like to expand the "MODIFICATION" check quoted in comment 13 to catch cases where:
 (1) we're setting the transform attribute from something nonempty to something empty.
 (2) we're setting the transform attribute from something empty to something nonempty.

This logic currently handles (1) (it catches that scenario), but it does not handle (2).

(Also: for the purposes of my hand-waving above, nonempty-but-bogus values like transform="foobar" should be considered "empty", since they produce an empty transform list.)
This is just a refactoring to make it easier to catch new types of "attribute added" / "attribute removed" scenarios in this function (which I think is what we need here).

This patch shouldn't affect behavior at all -- it just refactors existing code.
Attachment #8728789 - Attachment is obsolete: true
Attachment #8729730 - Flags: review?(longsonr)
Attached patch part 2, WIP (hacky) (obsolete) — Splinter Review
This is a bit hacky (hence "wip") but it seems to fix the bug.

Basically, we detect the "transform attribute is changing from empty to nonempty" scenario in nsSVGElement::ParseAttribute and set some state, which we later check for in SVGTransformableElement::GetAttributeChangeHint (and handle it like an attribute-addition).

Robert, can you think of anything better that we could do here?

(Ignore the hacky C-style cast in SVGTransformableElement::GetAttributeChangeHint, too -- I'm confident I can find a way to work around the need for that. It's just a stopgap for the moment to get this building/running.)
Attachment #8729733 - Flags: feedback?(longsonr)
Here's a less hacky version of part 2. Still curious if you can think of any better ways to handle this.

(I figured out the casting thing that was biting me earlier -- it was just a const-ness issue -- calling a non-const method from a const method. clang gave me a really un-helpful build error which made it more confusing than it actually was. Anyway, I'm using a cleaner const_cast hackaround in this version, which I'm happier with.)
Attachment #8729733 - Attachment is obsolete: true
Attachment #8729733 - Flags: feedback?(longsonr)
Attachment #8729753 - Flags: feedback?
Attachment #8729730 - Flags: review?(longsonr) → review+
(In reply to Daniel Holbert [:dholbert] from comment #17)
> Created attachment 8729753 [details] [diff] [review]
> part 2: track when transform attribute goes from empty to nonempty
> 
> Here's a less hacky version of part 2. Still curious if you can think of any
> better ways to handle this.
> 

Part 2 won't catch SMIL animation changes will it?

You could store wasUnset on the transformList itself I guess as an additional bool.
actually we can't do my suggestion as we might not have a transform list if the transform is SMIL animateMotion. So a bool on SVGTransformablElement or a property.
or somehow get whatever calculates aModType to set it to ADDITION instead of MODIFICATION somehow.
the code to set blank transforms as no transform i.e. bug 86118 was a workaround to a bug in svg-edit that has since been fixed so if we wanted we could revert that and go back to an empy transform being a create i.e. remove the HasTransform checks from everywhere.
> Part 2 won't catch SMIL animation changes will it?

SMIL doesn't seem to have this bug, for some reason -- here's a testcase demonstrating that (no assertions or disappearing content for me).
(In reply to Robert Longson from comment #20)
> or somehow get whatever calculates aModType to set it to ADDITION instead of
> MODIFICATION somehow.

FWIW: this is sort of what we do for SMIL, which is why this works there.

Specifically: in nsSVGAnimatedTransformList::SetAnimValue, we cache the "was previously set" state here:
> 82   bool prevSet = HasTransform() || aElement->GetAnimateMotionTransform();

Then, we set the value. And then, we determine our modType:
> 115   if(prevSet) {
> 116     modType = nsIDOMMutationEvent::MODIFICATION;
> 117   } else {
> 118     modType = nsIDOMMutationEvent::ADDITION;
> 119   }
> 120   aElement->DidAnimateTransformList(modType);

So testcase 2 ends up triggering DidAnimateTransformList (and GetAttributeChangeHint inside of that) with modType==nsIDOMMutationEvent::ADDITION, not MODIFICATION.
BTW, "whatever calculates aModType " is the following code, in Element::MaybeCheckSameAttrVal():

> 2217       bool valueMatches = aValue.EqualsAsStrings(*info.mValue);
> 2218       if (valueMatches && aPrefix == info.mName->GetPrefix()) {
> 2219         return true;
> 2220       }
> 2221       modification = true;
> 2222     }
> 2223   }
> 2224   *aModType = modification ?
> 2225     static_cast<uint8_t>(nsIDOMMutationEvent::MODIFICATION) :
> 2226     static_cast<uint8_t>(nsIDOMMutationEvent::ADDITION);
http://mxr.mozilla.org/mozilla-central/source/dom/base/Element.cpp?rev=7212aefa6d71#2217

Basically, it's just checking (a) do we have some existing value in this attribute, and (b) does the old value match the new value. And it uses this to determine whether to do nothing, vs. treat this as an addition or modification.

(This code gets invoked when Element::SetAttr() calls OnlyNotifySameValueSet(), which calls MaybeCheckSameAttrVal().  Element::SetAttr has a local-variable which gets passed in as an outparam .)

So -- this is way too abstract of a place to care about the details of TransformList implementations & whether our old value happens to be bogus or not.

The only way to implement Comment 21 (I think) would be to modify Element::ParseAttribute (called a bit later in Element::SetAttr), to make it accept |modType| as an outparam and be able to customize it depending on what we parse & what the old value was.  Then, we could modify nsSVGElement::ParseAttribute to change |modType| in this scenario with transform attribute.

That might be doable, but that seems like a pretty broad-reaching change. (I'm also not sure if we have dependencies about modType being "correct" from a did-the-attribute-exist-at-all-previously perspective...)
Either comment 21 or what you've already done then.
I'm currently preparing a new patch which adds a new member-bool (alongside its one existing member-bool) to nsSVGAnimatedTransformList, BTW.

The new bool tracks whether "HasTransform()" returned true before the most recent tweak to our base value.
Here's an updated "part 1", with only a whitespace tweak (adding a newline, to make it easier to insert into an "if" condition in a readable way).

Carrying forward r=longsonr.
Attachment #8729730 - Attachment is obsolete: true
Attachment #8730380 - Flags: review+
Attachment #8729753 - Attachment is obsolete: true
Attachment #8729753 - Flags: feedback?
Comment on attachment 8730400 [details] [diff] [review]
part 2 v3 (now with documentation): Add "HadTransformBeforeLastBaseValTweak()" accessor

I think Change is better than Tweak. Tweak is something unpleasant that happens to noses ;-)

r=longsonr with that changed(i.e. tweaked) throughout
Attachment #8730400 - Flags: review?(longsonr) → review+
Sounds good -- I'll fix that. :) Thanks for the review!
https://hg.mozilla.org/mozilla-central/rev/0b31695a67bf
https://hg.mozilla.org/mozilla-central/rev/9483041fedee
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.