Closed
Bug 1387906
Opened 7 years ago
Closed 7 years ago
stylo: style rules of pseudo-elements may be applied multiple times
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: xidorn, Unassigned)
References
Details
In devtools/server/tests/mochitest/test_styles-applied.html, the subtest "inheritedSystemStyles" fails because it finds 3 more rules than expected (16 vs. 13) when querying applied rules.
After some investigation, the outstanding three rules are:
* https://searchfox.org/mozilla-central/rev/b52285fffc13f36eca6b47de735d4e4403b3859e/layout/style/res/ua.css#317-323
* https://searchfox.org/mozilla-central/rev/b52285fffc13f36eca6b47de735d4e4403b3859e/layout/style/res/forms.css#914-928
* https://searchfox.org/mozilla-central/rev/b52285fffc13f36eca6b47de735d4e4403b3859e/layout/style/res/forms.css#938-948
When using Gecko's style system, these three rules show up in the list, but with Stylo, each of them shows up twice, which causes this failure.
This may potentially have some performance impact, though probably not a big deal on that aspect.
Comment 1•7 years ago
|
||
Are we getting different return values from DOMUtils.getCSSStyleRules in _getElementRules?
Updated•7 years ago
|
Flags: needinfo?(xidorn+moz)
Reporter | ||
Comment 2•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #1)
> Are we getting different return values from DOMUtils.getCSSStyleRules in
> _getElementRules?
_getElementRules you mean [1]? That seems to be just applying a filter to the result from DOMUtils.getCSSStyleRules. The result of DOMUtils.getCSSStyleRules is from rule nodes [2].
[1] https://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/devtools/server/actors/styles.js#563
[2] https://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/servo/ports/geckolib/glue.rs#1824
Flags: needinfo?(xidorn+moz)
Comment 3•7 years ago
|
||
So all those have important and non-important rules, so they generate two rule nodes, one for the normal level, one for the important level. I suspect the API expects them to get filtered out or something?
Reporter | ||
Comment 4•7 years ago
|
||
Oh, that's a good point. We probably should filter them out somehow.
Reporter | ||
Updated•7 years ago
|
Priority: -- → P3
Comment 5•7 years ago
|
||
Ah, good catch. On the Gecko side of DOMUtils.getCSSStyleRules, we have:
RefPtr<Declaration> decl = do_QueryObject(ruleNode->GetRule());
if (decl) {
The !important rulenodes will return an ImportantStyleData* from GetRule(), which will not QI to Declaration, so nothing will get appended for those rulenodes.
Reporter | ||
Comment 6•7 years ago
|
||
Then what would happen if a rule node only contains important rules?
Comment 7•7 years ago
|
||
Gecko always has the non-!important rulenode in the ruletree. The !important bits are added by walking the non-!important branch and asking everything in it whether there are !important rules.
Comment 8•7 years ago
|
||
We can just read the declaration block if the level is important, and skip it if it returns true for any_normal. That should also match Gecko's order IIUC. Will write a patch as soon as I get to the office.
Comment 9•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> We can just read the declaration block if the level is important, and skip
> it if it returns true for any_normal. That should also match Gecko's order
> IIUC. Will write a patch as soon as I get to the office.
Like so: https://github.com/servo/servo/pull/18008
I just noticed that it won't return the rules in the same order as Gecko for rules only containing important properties, but that doesn't sound like a huge deal I think, and it's pre-existing.
Reporter | ||
Comment 10•7 years ago
|
||
It seems the fix has been merged. I've got unexpected pass for this test with m-c today.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 11•7 years ago
|
||
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•