Ting-Yu Chou [:ting] (away) Assignee
	Description • 10 years ago

+++ This bug was initially created as a clone of Bug #1074783 +++

These two rules from sound_manager.css:

  #volume section div:nth-last-child(2) {
    border-radius: 0 0.6rem 0.6rem 0;
    /* Last visible bar */
  }
  #volume[data-channel="bt_sco"] section div:nth-last-child(2) {
    border-radius: 0;
  }

sets #windows div the flag NODE_HAS_SLOW_SELECTOR when CallscreenWindow inserts view to it, and causes a subtree restyle when insert or append to the div afterward. From the profile of bug 1074783 comment 25, it takes ~40ms on b2g process main thread when launch app.

We should be able to limit the scope of sound_manager.css.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 2 • 10 years ago

Comment on attachment 8510136 [details]
PR

Need a second opinion to say if this is a good fix, or we should simply rewrite the CSS.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 3 • 10 years ago

Comment on attachment 8510136 [details]
PR

This is intuitive, but I failed to understand why #volume roles affects #windows because #volume lives inside #system-overlay which is parallel to #windows.

Any further information?

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 4 • 10 years ago

dbaron, sorry for keep bothering, but I need your help to clarify this:

There must be some reasons that nthChildGenericMatches() set the flag NODE_HAS_SLOW_SELECTOR before knowing whether the element matches the rule, and I guess it is because when HTML is inserted dynamically, all the rules in the scope need to be checked, is my understanding correct?

I am asking since there're two divs A and B, and there's a rule R (:nth-last-child()) matches B, but when there's a javascript insert HTML to A, A will be set flag NODE_HAS_SLOW_SELECTOR by nthChildGenericMatches() even R is not matched, and it makes appendChild() to A afterward triggers a subtree restyle for A.

Hope this time I make the question clearer. Thanks.

	David Baron :dbaron:
	Comment 5 • 10 years ago

(In reply to Ting-Yu Chou [:ting] from comment #4)
> dbaron, sorry for keep bothering, but I need your help to clarify this:
> 
> There must be some reasons that nthChildGenericMatches() set the flag
> NODE_HAS_SLOW_SELECTOR before knowing whether the element matches the rule,
> and I guess it is because when HTML is inserted dynamically, all the rules
> in the scope need to be checked, is my understanding correct?

When we match selectors, we're looking at the parts separated by combinators (space, +, >, etc.) from right to left.  (Though we're only trying to match them if they've passed a bloom filter that's looking, I think, at tags, classes, and IDs on the whole selector.)  If we've gotten to calling nthChildGenericMatches, we know that part of the selector matches, and we've gotten to the point of seeing if it matches :nth-child().  Whether or not it currently matches or does not match, that means that inserting or removing a content node somewhere else in its siblings could change its index and cause it to match.  This means that we have to set NODE_HAS_SLOW_SELECTOR so that inserting and removing elements from that child list causes us to rerun selector matching on its descendants.

> I am asking since there're two divs A and B, and there's a rule R
> (:nth-last-child()) matches B, but when there's a javascript insert HTML to
> A, A will be set flag NODE_HAS_SLOW_SELECTOR by nthChildGenericMatches()
> even R is not matched, and it makes appendChild() to A afterward triggers a
> subtree restyle for A.

When we match selectors, we have already checked whether descendant selectors match, but not ancestors (except for what's done by the bloom filter, which is a probabilistic algorithm).

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 6 • 10 years ago

Comment on attachment 8510136 [details]
PR

Learned a lesson, thanks.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 7 • 10 years ago

I forgot to check the gaia test results, will set later after checking.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 8 • 10 years ago

The tests are green.

	Carsten Book [:Tomcat]
	Comment 9 • 10 years ago

https://github.com/mozilla-b2g/gaia/commit/64f4a6fa02e16140e005b3ec1195c90a6a698aeb

	Julien Wajsberg [:julienw]
	Comment 10 • 10 years ago

(In reply to Carsten Book [:Tomcat] from comment #9)
> https://github.com/mozilla-b2g/gaia/commit/
> 64f4a6fa02e16140e005b3ec1195c90a6a698aeb

Mmm wrong bug number in the commit log :/
I'd advise backing out and relanding with the correct bug number, which will make it easier in the future to know why we scoped this rule.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 11 • 10 years ago

revert in https://github.com/mozilla-b2g/gaia/commit/04a47dbf5251abe58447d8971bd255e991b6e0dc
re-merged in https://github.com/mozilla-b2g/gaia/commit/e91d99e4d96954f06383c00bb9d79598a697e310 with correct commit message.

	Julien Wajsberg [:julienw]
	Comment 12 • 10 years ago

Thanks a lot Tim :)

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 13 • 10 years ago

Julien & Tim, thank you for the correction.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 14 • 10 years ago

Comment on attachment 8510136 [details]
PR

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): the css rule of sound volume
[User impact] if declined: unnecessary restyle (~40ms) during app launch 
[Testing completed]: done, check the PR
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: n/a

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 15 • 10 years ago

Comment on attachment 8510136 [details]
PR

I just realized the request forms are the same.

	bhavana bajaj [:bajaj]
	Comment 16 • 10 years ago

Given we've met the perf goals for 2.0/2.1, I think this can ride the trains.

	Gregor Wagner [:gwagner]
	Comment 17 • 10 years ago

Based on comment 16