The attributeChangedCallback is fired twice for the *first* style attribute change

RESOLVED FIXED in Firefox 63

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: edgar, Assigned: smaug)

Tracking

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

Reporter

Description

2 years ago
(See Bug #1419322 comment #14)

This bug is filed for tracking
- There are two attributeChangedCallback fired for the *first* style attribute change [2].

[2] https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5663
Assignee

Updated

Last year
Assignee: nobody → bugs
Assignee

Updated

Last year
No longer blocks: 1419322
Assignee

Updated

Last year
Duplicate of this bug: 1419322
Assignee

Updated

Last year
Duplicate of this bug: 1419308
Assignee

Comment 3

Last year
remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/b10b233a562ca28287e8255d8d5e215bade8906d
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=b10b233a562ca28287e8255d8d5e215bade8906d
remote: recorded changegroup in replication log in 0.107s
Assignee

Comment 4

Last year
This makes style attribute handling from DOM point of view less weird.
I didn't add BeforeSetAttr call, but that is something to consider.

The basic idea is that the initial GetCSSDeclaration (now called GetOrCreateCSSDeclaration) call doesn't anymore do that magical SetAttr(notify=false) call, but just creates declaration if needed.
Then it is up to the caller to call SetCSSDeclaration, either passing the old or new declaration, depending on whether everything succeeded.

Hopefully I managed to remove all the .inis for now passing tests...
remote: 
remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/75e19b83e2f83b1067fb5e9c1eed211e5d1e2f43
remote:   https://hg.mozilla.org/try/rev/75da8c403d214252ef8b3ad1af5484e9a1e2b6e1
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=75da8c403d214252ef8b3ad1af5484e9a1e2b6e1
remote: recorded changegroup in replication log in 0.097s
Attachment #8982333 - Attachment is obsolete: true
Attachment #8982468 - Flags: review?(bzbarsky)
Comment on attachment 8982468 [details] [diff] [review]
sane_style_attribute_handling_2.diff

This could really use a commit message describing what's actually getting changed...

The key part that needs to be documented in the commit message is that now we force cloning of the declaration on set if there are mutation observers, so that we can guarantee that we can produce the old value from GetAttr() even after the declaration has been mutated (because the mutation happens on a clone).  We used to only do this for mutation events, because those needed the old value much later down the line, and we handled mutation observers (and custom element data?) via firing AttributeWillChange before the actual declaration mutations, right?

If I understand this part correctly, have you measured the performance impact of this?   Does introducing a mutation observer somewhere in the document now slow down all inline style sets in that document?  By how much?  Does it do that in other UAs?

It feels like we should get this part sorted out before we worry about the details of the patch....
Flags: needinfo?(bugs)
Assignee

Comment 6

Last year
ok, it does regress performance when document has mutation observers.
The tricky thing is that right now too many MutationRecords are created - they are created even if style isn't changed at all.
"[Removing non-existing property or setting invalid value on CSS declaration block shouldn't queue mutation record]"
And also, obviously currently attributeChanged handling is all wrong.

the patch does seem to improve performance when there aren't any kind of mutation observers/listeners.


(Blink is a lot faster when setting style.foo, style.color as an example.)
> ok, it does regress performance when document has mutation observers.

How much?
Assignee

Comment 8

Last year
Too much,  30% or so. And the speed up without mutation observers is just couple of %.
Stylo seems to be very malloc heavy when dealing with DeclarationBlock's.

Blink is 2.5x faster than current Gecko.
Flags: needinfo?(bugs)
Assignee

Comment 9

Last year
Posted file a perf testcase (obsolete) —
Assignee

Updated

Last year
Attachment #8982468 - Flags: review?(bzbarsky)
Assignee

Comment 10

Last year
Posted file a perf testcase
Fixed a bug in the testcase's withattrmutationobserver part.
Attachment #8982654 - Attachment is obsolete: true
Assignee

Updated

Last year
Depends on: 1468665
Assignee

Comment 11

Last year
This is annoyingly complicated, but doesn't regress performance.
Attachment #8982468 - Attachment is obsolete: true
Assignee

Comment 13

Last year
+ missing null check

remote:   https://hg.mozilla.org/try/rev/89e768d85c69ed81541e6d9b8e6967cadb2f05cc
remote:   https://hg.mozilla.org/try/rev/d5b6f0c1a9d0ab87b21e33d5d49e39a6977d0db1
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5b6f0c1a9d0ab87b21e33d5d49e39a6977d0db1
remote: recorded changegroup in replication log in 0.085s
Attachment #8985442 - Attachment is obsolete: true
Assignee

Comment 14

Last year
Comment on attachment 8985580 [details] [diff] [review]
sane_style_attribute_handling_complicated_2.diff

So, the idea with this patch is that style code will first call
InlineStyleDeclarationWillChange before style declaration has changed, and SetInlineStyleDeclaration once it has changed.

In order to be able to report old attribute value, InlineStyleDeclarationWillChange reads the value and also calls AttributeWillChange (so that DOMMutationObserser can grab the old value). Later SetInlineStyleDeclaration passes the old value to 
SetAttrAndNotify so that mutation events and attributeChanged callbacks are handled correctly.

Because of performance, declaration can't be cloned for reading the old value. And that is why the recently-added callback is used to detect when declaration is about to change (bug 1466963 and followup bug 1468665).

To keep the expected existing behavior, even if declaration isn't changed, but just a new declaration was created (since there wasn't any), we need to still run all these
willchange/set calls. That is when the code has 'if (created)' checks.

Since there are several declaration implementation and only nsDOMCSSAttributeDeclaration needs the about-to-change callback, GetPropertyChangeClosure is the one to initialize the callback closure, and the struct which is then passes as data to the closure.

Apparently we lost mutation event testing on style attribute when the pref was added, so test_style_attr_listener.html is modified to test both pref values.

This is annoyingly complicated.

One note, currently AttributeWillChange handling is very inconsistent,
https://bugzilla.mozilla.org/show_bug.cgi?id=1468851
The patch has the model I think we should always have - always enter doc update before AttributeWillChange call.

peterv, feel free to forward the review request to someone else (perhaps mrbkap?). This is rather core DOM stuff even though lots of the changes are in layout/style.
(bz is about to have vacation, so not accepting requests)
Attachment #8985580 - Flags: review?(peterv)
Comment on attachment 8985580 [details] [diff] [review]
sane_style_attribute_handling_complicated_2.diff

Review of attachment 8985580 [details] [diff] [review]:
-----------------------------------------------------------------

This really needs a good checkin comment explaining what is changing (comment 14 is a good start :-)).

::: dom/base/Element.cpp
@@ +2101,5 @@
>  
> +void
> +Element::InlineStyleDeclarationWillChange(MutationClosureData* aData)
> +{
> +  MOZ_ASSERT_UNREACHABLE("Element::SetInlineStyleDeclaration");

s/SetInlineStyleDeclaration/InlineStyleDeclarationWillChange/

::: dom/base/Element.h
@@ +323,5 @@
>    void ClearMappedServoStyle() {
>      mAttrsAndChildren.ClearMappedServoStyle();
>    }
>  
> +  virtual void InlineStyleDeclarationWillChange(MutationClosureData* aData);

Needs documenting.
Looks like aData will always be non-null, so I'd prefer a reference instead of a pointer.

@@ +331,3 @@
>     */
>    virtual nsresult SetInlineStyleDeclaration(DeclarationBlock* aDeclaration,
> +                                             MutationClosureData* aData);

Same thing about the reference.

::: dom/base/nsStyledElement.cpp
@@ +69,5 @@
>  
> +void
> +nsStyledElement::InlineStyleDeclarationWillChange(MutationClosureData* aData)
> +{
> +  MOZ_ASSERT(OwnerDoc()->UpdateNestingLevel(),

I prefer |> 0|.

@@ +96,5 @@
> +        aData->mOldValue->SetTo(oldValueStr);
> +      }
> +    } else {
> +      modification =
> +        !!mAttrsAndChildren.GetAttr(nsGkAtoms::style, kNameSpaceID_None);

HasAttr?

::: layout/style/nsDOMCSSAttrDeclaration.cpp
@@ +74,4 @@
>  {
>    NS_ASSERTION(mElement, "Must have Element to set the declaration!");
> +
> +  MOZ_ASSERT_IF(!mIsSMILOverride, aClosureData);

This should be documented next to the SetCSSDeclaration's declaration (looks a lot like a secret handshake with nsDOMCSSAttributeDeclaration::SetSMILValue :-/).

@@ +124,5 @@
>    // cannot fail
>    RefPtr<DeclarationBlock> decl = new DeclarationBlock();
> +  // Mark the declaration dirty so that it can be reused by the caller.
> +  // Normally SetDirty is called later in SetCSSDeclaration.
> +  //XXXsmaug DeclarationBlock's dirty and immutable flag handling is odd.

Not sure this last line is helpful.

::: layout/style/nsDOMCSSAttrDeclaration.h
@@ +64,5 @@
> +  {
> +    aClosure->function = MutationClosureFunction;
> +    aClosure->data = aClosureData;
> +    aClosureData->mClosure = MutationClosureFunction;
> +    aClosureData->mElement = mElement;

There needs to be a comment somewhere on why we need both function on DeclarationBlockMutationClosure and mClosure on MutationClosureData, even though they point to the same function. I think it is because we need to detect that MutationClosureFunction hasn't been called yet, and function doesn't allow us to detect that?
Could it work to have a boolean member signifying that InlineStyleDeclarationWillChange still needs to be called, set to true here and set to false in the MutationClosureData constructor and once it has been called? Since mClosure is always only ever set to MutationClosureFunction if non-null, we could then just call mElement->InlineStyleDeclarationWillChange directly instead of calling through mClosure and MutationClosureFunction (in nsDOMCSSAttributeDeclaration::SetCSSDeclaration and nsDOMCSSDeclaration::SetCssText).

::: layout/style/nsDOMCSSDeclaration.cpp
@@ +128,5 @@
>    ParsingEnvironment servoEnv =
>      GetParsingEnvironment(aSubjectPrincipal);
>    if (!servoEnv.mUrlExtraData) {
> +    if (created) {
> +      SetCSSDeclaration(olddecl, &closureData);

This (and others below) could use a comment explaining why we do this.

::: layout/style/nsDOMCSSDeclaration.h
@@ +173,4 @@
>      eOperation_Modify,
>  
> +    // We are calling GetOrCreateCSSDeclaration so that we can remove a property
> +    // from it. Does not allocates a new declaration if we don't have one yet;

s/allocates/allocate/
Attachment #8985580 - Flags: review?(peterv) → review+
Assignee

Comment 16

Last year
(In reply to Peter Van der Beken [:peterv] from comment #15)
> There needs to be a comment somewhere on why we need both function on
> DeclarationBlockMutationClosure and mClosure on MutationClosureData, even
> though they point to the same function. I think it is because we need to
> detect that MutationClosureFunction hasn't been called yet, and function
> doesn't allow us to detect that?
> Could it work to have a boolean member signifying that
> InlineStyleDeclarationWillChange still needs to be called, set to true here
> and set to false in the MutationClosureData constructor and once it has been
> called? Since mClosure is always only ever set to MutationClosureFunction if
> non-null, we could then just call mElement->InlineStyleDeclarationWillChange
> directly instead of calling through mClosure and MutationClosureFunction (in
> nsDOMCSSAttributeDeclaration::SetCSSDeclaration and
> nsDOMCSSDeclaration::SetCssText).


problem is that SetCSSText doesn't have an element to play with.
Assignee

Comment 17

Last year
Or I guess one could use mElement from the closure data.
Assignee

Comment 18

Last year
remote: View the pushlog for these changes here:
remote:   https://hg.mozilla.org/try/pushloghtml?changeset=d17ede513a5d8ecfc28bdfa5fc6078a3d5c1b15e
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=d17ede513a5d8ecfc28bdfa5fc6078a3d5c1b15e
remote: recorded changegroup in replication log in 0.117s

Comment 19

Last year
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c0ffefb34f
The attributeChangedCallback is fired twice for the *first* style attribute change, r=peterv

Comment 20

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/e8c0ffefb34f
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Duplicate of this bug: 1197705
(In reply to Olli Pettay [:smaug] from comment #17)
> Or I guess one could use mElement from the closure data.

Yeah, exactly. Seemed more elegant than using that mClosure function pointer to essentially record a boolean state.
Assignee

Updated

11 months ago
Depends on: 1475003
You need to log in before you can comment on or make changes to this bug.