No need to mutate declarationblock and notify if the property to be removed doesn't exist at all

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks 1 bug)

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
We currently always make declaration block mutable and notify the owner about its being changed when we remove a property. However, it is wasteful when the property to be removed doesn't exist at all.

One free optimization we can do for this case is to check the old value in nsDOMCSSDeclaration::RemoveProperty and return directly rather than calling into RemovePropertyInternal when the old value is empty.

There are some other callsites of RemovePropertyInternal, but we cannot check property existence for free in those cases, so probably it's not worth it.

Making this block bug 1420423 because I see such wasteful removals reach the root element which can trigger a whole document restyle. (After fixing this, bug 1435122 would become the only thing triggers such restyle in my local profiling.)
(Assignee)

Updated

a year ago
Attachment #8947702 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

a year ago
Looks like it doesn't work. Probably worth investigating why.
What part doesn't work?  (Are there cases when properties can validly have empty string as value, btw?)
It's also not 100% clear what the MutationObserver behavior for the "style" attribute should be in this case.... I really wish the CSSOM spec were sane.  :(
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> What part doesn't work?  (Are there cases when properties can validly have
> empty string as value, btw?)

Probably shorthands, from the try run.
(Assignee)

Comment 6

a year ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> What part doesn't work?  (Are there cases when properties can validly have
> empty string as value, btw?)

There are test failures on the try push (which you can see from the MozReview). I don't currently have idea on what's going on, but from a brief look of the message, I'm guessing it may be related to shorthands.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> It's also not 100% clear what the MutationObserver behavior for the "style"
> attribute should be in this case.... I really wish the CSSOM spec were sane.
> :(

When you are removing a non-existing property, the style is not going to change at all, so I suppose MutationObserver shouldn't be triggered either...

But maybe this would make removeProperty and setProperty with empty value behave inconsistent?
> Probably shorthands, from the try run.

Ah, indeed.

> so I suppose MutationObserver shouldn't be triggered either...

MutationObserver _is_ triggered if you do element.setAttribute("foo", element.getAttribute("foo")), fwiw.  So it _really_ depends on what the spec says in terms of when the attr set happens on CSSOM mutations...

We should definitely have setProperty(x, "") and removeProperty(prop) have identical behavior.
(Assignee)

Comment 8

a year ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> > so I suppose MutationObserver shouldn't be triggered either...
> 
> MutationObserver _is_ triggered if you do element.setAttribute("foo",
> element.getAttribute("foo")), fwiw.  So it _really_ depends on what the spec
> says in terms of when the attr set happens on CSSOM mutations...

So... CSSOM doesn't seem to be using the wording of DOM spec, but for removeProperty, it says:
* If property is a shorthand property, for each longhand property longhand that property maps to, invoke removeProperty() with longhand as argument.
* Otherwise, if property is a case-sensitive match for a property name of a CSS declaration in the declarations, remove that CSS declaration. 

So it sounds like there should be no mutation posted to anything in case the property doesn't exist. But the current spec sounds like it doesn't queue any mutation record anyway, which we probably should fix. I can probably try to write a PR for it.

And in terms of consistency, currently if the style attribute doesn't exist at all, we don't queue mutation record either. Maybe this change would make us more consistent to ourselves at least.

> We should definitely have setProperty(x, "") and removeProperty(prop) have
> identical behavior.

Yeah, the spec says:
* If value is the empty string, invoke removeProperty() with property as argument and terminate this algorithm. 

So I guess the change should probably be done in RemovePropertyInternal... Probably we can have declaration blocks return whether anything is really removed, and in case there isn't, we pass aNotify=false into SetInlineStyleDeclaration.
(Assignee)

Comment 9

a year ago
> So it sounds like there should be no mutation posted to anything in case the property doesn't exist. But the current spec sounds like it doesn't queue any mutation record anyway, which we probably should fix. I can probably try to write a PR for it.

Submitted w3c/csswg-drafts#2268.
> But the current spec sounds like it doesn't queue any mutation record anyway

If only things were so simple.  https://drafts.csswg.org/cssom/#the-elementcssinlinestyle-interface says:

  Mutating the declarations must set the style content attribute on the context object
  to the serialization of the declarations.

So technically it's maybe doing an attribute set for every shorthand in the longhand, with a MutationRecord for each one.  Pretty sure no one does that.

I filed https://github.com/w3c/csswg-drafts/issues/1559 back in June last year on the spec being completely nuts around this stuff...

I would be happy if no-op changes to the decl block did not queue mutation records, assuming those no-ops are easy to detect.  Certainly for the "remove a prop that's not there" case that would be good.
(Assignee)

Comment 11

a year ago
Hmmm, fixing the performance issue is probably easier than fixing the spec-conformance issue...

The mutation record is created in AttributeWillChange :/
(Assignee)

Comment 12

a year ago
Since solving the performance issue for bug 1420423 doesn't require fixing the mutation observer change, I'm going to ignore that part for now.
(Assignee)

Comment 13

a year ago
For fixing the mutation observer behavior, I think what we probably should do is to
1. make nsMutationReceiver::AttributeWillChange put the new mutation record aside as a pending one, rather than appending it directly, then
2. add nsMutationReceiver::AttributeChanged which checks and eventually appends the pending mutation record.

This way, if an attribute isn't actually changed, we wouldn't queue any mutation record.
(Assignee)

Updated

a year ago
Attachment #8947702 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Blocks: 1197705
Priority: -- → P1

Comment 15

a year ago
mozreview-review
Comment on attachment 8948304 [details]
Bug 1435139 - Don't call SetCSSDeclaration when removing non-existing property.

https://reviewboard.mozilla.org/r/217786/#review224716

OK.  So this will stop dispatching mutation observer notifications where we now dispatch them...

This is probably OK, assuming we sort out the mutation observer bit in the spec to match and write some web platform tests (in a followup). r=me

::: layout/style/Declaration.h:130
(Diff revision 1)
>    void ValueAppended(nsCSSPropertyID aProperty);
>  
>    void GetPropertyValue(const nsAString& aProperty, nsAString& aValue) const;
>    void GetPropertyValueByID(nsCSSPropertyID aPropID, nsAString& aValue) const;
>    bool GetPropertyIsImportant(const nsAString& aProperty) const;
> -  void RemoveProperty(const nsAString& aProperty);
> +  bool RemoveProperty(const nsAString& aProperty);

Please document the return value.

::: layout/style/Declaration.h:161
(Diff revision 1)
>    /**
>     * Removes a custom property declaration from this object.
>     *
>     * @param aName The variable name (i.e., without the "--" prefix).
>     */
> -  void RemoveVariable(const nsAString& aName);
> +  bool RemoveVariable(const nsAString& aName);

Again, document return value.

::: layout/style/DeclarationBlock.h:135
(Diff revision 1)
>    inline void GetPropertyValue(const nsAString& aProperty,
>                                 nsAString& aValue) const;
>    inline void GetPropertyValueByID(nsCSSPropertyID aPropID,
>                                     nsAString& aValue) const;
>    inline bool GetPropertyIsImportant(const nsAString& aProperty) const;
> -  inline void RemoveProperty(const nsAString& aProperty);
> +  inline bool RemoveProperty(const nsAString& aProperty);

Document return value.

::: layout/style/ServoDeclarationBlock.h:69
(Diff revision 1)
>    }
>  
>    void GetPropertyValue(const nsAString& aProperty, nsAString& aValue) const;
>    void GetPropertyValueByID(nsCSSPropertyID aPropID, nsAString& aValue) const;
>    bool GetPropertyIsImportant(const nsAString& aProperty) const;
> -  void RemoveProperty(const nsAString& aProperty);
> +  bool RemoveProperty(const nsAString& aProperty);

Document.
Attachment #8948304 - Flags: review?(bzbarsky) → review+
I'm sorry for the lag here.  I thought I submitted this yesterday night, but mozreview managed to time itself out submitting the publish... :(  It's been extremely slow and laggy for me recently (multi-second waits on basic UI operations like changing the review dropdown to r+).  :(
(Assignee)

Comment 17

a year ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> Comment on attachment 8948304 [details]
> Bug 1435139 - Don't call SetCSSDeclaration when removing non-existing
> property.
> 
> https://reviewboard.mozilla.org/r/217786/#review224716
> 
> OK.  So this will stop dispatching mutation observer notifications where we
> now dispatch them...
> 
> This is probably OK, assuming we sort out the mutation observer bit in the
> spec to match and write some web platform tests (in a followup). r=me

No, it doesn't change that behavior at all. The record is currently done in GetDeclarationBlock rather than the setter. See my comment in bug 1197705.
(Assignee)

Comment 18

a year ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #16)
> I'm sorry for the lag here.  I thought I submitted this yesterday night, but
> mozreview managed to time itself out submitting the publish... :(  It's been
> extremely slow and laggy for me recently (multi-second waits on basic UI
> operations like changing the review dropdown to r+).  :(

There may be some bug in Firefox which is making MozReview slower. I think I observed once when MozReview got extremely slow and eventually irresponsible at all, while other people told me it works fine for them and it worked after I restarted the browser.
Could be.  But network panel is claiming things like "we sent the PUT and the server took 5s to respond"...
(Assignee)

Comment 20

a year ago
Posted file Servo PR
Comment hidden (mozreview-request)

Comment 22

a year ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/03d139086ace
Don't call SetCSSDeclaration when removing non-existing property. r=bz

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/03d139086ace
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Duplicate of this bug: 845205
You need to log in before you can comment on or make changes to this bug.