Closed
Bug 1087905
Opened 10 years ago
Closed 10 years ago
Insert or append to #windows div causes a subtree restyle and hurts app launch time
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.2 fixed)
RESOLVED
FIXED
2.1 S7 (24Oct)
People
(Reporter: ting, Assigned: ting)
References
Details
Attachments
(1 file)
46 bytes,
text/x-github-pull-request
|
alive
:
review+
timdream
:
feedback+
bajaj
:
approval-gaia-v2.0-
bajaj
:
approval-gaia-v2.1-
|
Details | Review |
+++ 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.
Assignee | ||
Updated•10 years ago
|
status-b2g-v2.0:
--- → affected
status-b2g-v2.0M:
--- → affected
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → affected
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8510136 -
Flags: review?(timdream)
Comment 2•10 years ago
|
||
Comment on attachment 8510136 [details] [review]
PR
Need a second opinion to say if this is a good fix, or we should simply rewrite the CSS.
Attachment #8510136 -
Flags: review?(timdream)
Attachment #8510136 -
Flags: review?(alive)
Attachment #8510136 -
Flags: feedback+
Comment 3•10 years ago
|
||
Comment on attachment 8510136 [details] [review]
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?
Flags: needinfo?(tchou)
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.
Flags: needinfo?(tchou) → needinfo?(dbaron)
(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).
Flags: needinfo?(dbaron)
Comment 6•10 years ago
|
||
Comment on attachment 8510136 [details] [review]
PR
Learned a lesson, thanks.
Attachment #8510136 -
Flags: review?(alive) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 7•10 years ago
|
||
I forgot to check the gaia test results, will set later after checking.
Keywords: checkin-needed
Comment 9•10 years ago
|
||
Assignee: nobody → tchou
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (24Oct)
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.
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.
Updated•10 years ago
|
Comment 12•10 years ago
|
||
Thanks a lot Tim :)
Assignee | ||
Comment 13•10 years ago
|
||
Julien & Tim, thank you for the correction.
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8510136 [details] [review]
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
Attachment #8510136 -
Flags: approval-gaia-v2.0?
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8510136 [details] [review]
PR
I just realized the request forms are the same.
Attachment #8510136 -
Flags: approval-gaia-v2.1?
Comment 16•10 years ago
|
||
Given we've met the perf goals for 2.0/2.1, I think this can ride the trains.
Updated•10 years ago
|
Attachment #8510136 -
Flags: approval-gaia-v2.1?
Attachment #8510136 -
Flags: approval-gaia-v2.1-
Attachment #8510136 -
Flags: approval-gaia-v2.0?
Attachment #8510136 -
Flags: approval-gaia-v2.0-
Assignee | ||
Updated•10 years ago
|
Blocks: AppStartup
Comment 17•10 years ago
|
||
Based on comment 16
You need to log in
before you can comment on or make changes to this bug.
Description
•