Closed Bug 1349651 Opened 8 years ago Closed 8 years ago

stylo: Need to implement HasAuthorSpecifiedRules, so -moz-appearance will actually work with values other than "none"

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: mbrubeck)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

Bug 1349651 - Add Servo_ResolveRuleNode and Servo_HasAuthorSpecifiedRules glue.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1349651 - Generate bindings for NS_AUTHOR_SPECIFIED_*.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1349651 - Implement has_author_rules for Servo RuleNode.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1349651 - Implement author_colors_allowed in has_author_specified_rules.
59 bytes, text/x-review-board-request
bholley
: review+
Details
HasAuthorSpecifiedRulesFromnContentRemoved
6.20 KB, text/plain
Details
gecko patch for checkin
132.22 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
Right now nsPresContext::HasAuthorSpecifiedRules calls nsRuleNode::HasAuthorSpecifiedRules which does: #ifdef MOZ_STYLO if (aStyleContext->StyleSource().IsServoComputedValues()) { NS_WARNING("stylo: nsRuleNode::HasAuthorSpecifiedRules not implemented"); return true; } #endif which means -moz-appearance is all broken and all form control rendering is busted.
Assignee: nobody → mbrubeck
Priority: -- → P1
(bz says this is pretty high priority, and may lead to fixing several hundred failing tests).
Blocks: 1351971
See Also: → 1354990
Summary: stylo: Need to implement HasAuthorSpecifiedStyle, so -moz-appearance will actually work with values other than "none" → stylo: Need to implement HasAuthorSpecifiedRules, so -moz-appearance will actually work with values other than "none"
Blocks: 1321769
Manish did a try push to force gecko and stylo to have the same HASR behavior: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25833dac876c3881af7d1f5a48674b88ea161c47 Looks like a lot of passing tests!
Blocks: 1361632
No longer blocks: 1361632
Blocks: 1341815
https://treeherder.mozilla.org/#/jobs?repo=try&revision=95f46972a98430cda5b099bcedf1b4c86c155751 I still need to fix a crash in Servo_HasAuthorSpecifiedRules in a few tests, but the majority of tests are passing as expected, and I think these patches are ready for an initial review. The crashing stack looks like this: 0 libxul.so!core::sync::atomic::atomic_load<usize> [atomic.rs : 1365 + 0x18] 1 libxul.so!style::rule_tree::StrongRuleNode::get [mod.rs:95f46972a984 : 659 + 0xb] 2 libxul.so!style::rule_tree::StrongRuleNode::parent [mod.rs:95f46972a984 : 586 + 0x5] 3 libxul.so!style::rule_tree::{{impl}}::next::{{closure}} [mod.rs:95f46972a984 : 980 + 0x8] 4 libxul.so!core::option::Option<&style::rule_tree::StrongRuleNode>::map<&style::rule_tree::StrongRuleNode,&style::rule_tree::StrongRuleNode,closure> [option.rs : 383 + 0xb] 5 libxul.so!style::rule_tree::{{impl}}::next [mod.rs:95f46972a984 : 979 + 0xc] 6 libxul.so!style::rule_tree::StrongRuleNode::has_author_specified_rules<style::gecko::wrapper::GeckoElement> [mod.rs:95f46972a984 : 890 + 0x8]
Hundreds tests passed! I did dig into a crash test (dom/base/crashtests/612018-1.html). In the test case, all style data for the proplematic element (input) has been cleared before has_author_specified_rules() is called. The style data is cleared in FragmentOrElement::SetXBLInsertionParent [1]. Both of SetXBLInsertionParent() and nsPresContext::HasAuthorSpecifiedRules() are called from nsNodeUtils::ContentRemoved [2], but unfortunately SetXBLInsertionParent() is called prior to HasAuthorSpecifiedRules(). [1] https://hg.mozilla.org/mozilla-central/file/37a5b7f6f101/dom/base/FragmentOrElement.cpp#l1190 [2] https://hg.mozilla.org/mozilla-central/file/37a5b7f6f101/dom/base/nsNodeUtils.cpp#l226
Added a null check that fixes the crash in that test (and hopefully the other ones too). https://treeherder.mozilla.org/#/jobs?repo=try&revision=b93027aecc0867f2f64dab9faf35f3df7f3f8bc8
Attachment #8865270 - Flags: review?(bobbyholley)
Oops, added a missing patch to support the `allow_author_rules` parameter. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdbc4637d08ba2f0495eceb9dec6ce8e12726ff5
Comment on attachment 8865178 [details] Bug 1349651 - Generate bindings for NS_AUTHOR_SPECIFIED_*. https://reviewboard.mozilla.org/r/136844/#review140116
Attachment #8865178 - Flags: review?(bobbyholley) → review+
Comment on attachment 8865177 [details] Bug 1349651 - Add Servo_ResolveRuleNode and Servo_HasAuthorSpecifiedRules glue. https://reviewboard.mozilla.org/r/136842/#review140118 r=me with those things addressed. ::: layout/base/nsPresContext.cpp:2223 (Diff revision 2) > - nsRuleNode::HasAuthorSpecifiedRules(aFrame->StyleContext(), > + nsRuleNode::HasAuthorSpecifiedRules(aFrame->StyleContext(), > - ruleTypeMask, > + ruleTypeMask, > - UseDocumentColors()); > + UseDocumentColors()); > + } else { > + Element *elem = aFrame->GetContent()->AsElement(); > + if (aFrame->GetContent()->IsNativeAnonymous()) { Nit: This can just be |elem|. ::: layout/base/nsPresContext.cpp:2226 (Diff revision 2) > + RefPtr<RawServoRuleNode> ruleNode = Servo_ResolveRuleNode(elem, > + aFrame->StyleContext()->GetPseudo(), > + &mShell->StyleSet()->AsServo()->RawSet()).Consume(); Nit: I think it would be a bit more consistent with other stuff to make this a method on ServoStyleSet. So here we'd do mShell()->StyleSet()->AsServo()->ResolveRuleNode(elem, aFrame->StyleContext()->GetPseudo()); And then we'd have: already_AddRefed<ServoRuleNode> ServoStyleSet::ResolveRuleNode(...) { RefPtr<RawServoRueNode> ruleNode = Servo_ResolveRuleNode(...); return ruleNode.forget(); } ::: servo/components/style/gecko/arc_types.rs:82 (Diff revision 2) > [Servo_PageRule_AddRef, Servo_PageRule_Release]); > > impl_arc_ffi!(Locked<SupportsRule> => RawServoSupportsRule > [Servo_SupportsRule_AddRef, Servo_SupportsRule_Release]); > + > +// RuleNode is a Arc-like type but it does not use std::sync::Arc. Nit: The other Arcs here are actually style::stylearc::Arc at this point, so just make this comment say "does not use Arc". ::: servo/components/style/rule_tree/mod.rs:541 (Diff revision 2) > struct WeakRuleNode { > ptr: *mut RuleNode, > } > > /// A strong reference to a rule node. > +#[repr(C)] Hm, does this really need to be repr(C)? It's not a problem per se, but it doens't feel to me like it should be necessary, and the rule node stuff is weird enough that I'd like to avoid adding constraints if we can avoid it. Is it just to support the transmute in into_strong? If so, can we just convert the member instead? ::: servo/components/style/stylist.rs:529 (Diff revision 2) > + if self.pseudos_map.get(pseudo).is_none() { > + return self.rule_tree.root(); > + } I think this part originally came from my patch, but thinking about it more I think we should represent the value as an Option<StrongRuleNode> rather than using the root as a sentinel, because it avoids the atomic refcount bump and makes it easier for callers to short-circuit unnecessary work. ::: servo/components/style/stylist.rs:560 (Diff revision 2) > self.push_applicable_declarations(element, > None, > None, > None, > AnimationRules(None, None), > Some(pseudo), > guards, > &mut declarations, > &mut set_selector_flags); Please also check for an empty vec after this and return None in that case rather than calling insert_ordered_rules (which will trigger refcounting traffic and return the rule tree root). ::: servo/ports/geckolib/glue.rs:951 (Diff revision 2) > + -> RawServoRuleNodeStrong > +{ > + let element = GeckoElement(element); > + let doc_data = PerDocumentStyleData::from_ffi(raw_data); > + let guard = (*GLOBAL_STYLE_DATA).shared_lock.read(); > + let maybe_data = element.mutate_data(); Hm, can we just unwrap() this? I know there's that FIXME in Servo_ResolvePseudoStyle, but that might be able to be removed now that bug 1345695 is fixed, and we certainly shouldn't add more handing for buggy cases unless we know they exist. So please replace this with an unwrap(), and then also remove the warning about unstyled elements below, since the None case will just indicate no matching rules, rather than that _or_ an error condition (like it does with this path). If that causes failures, we followup bug with the specific failures listed so that people can investigate them. In that case, please early-return if mutate_data() returns None, so that we can separate the unexpected Nones from the expected ones. ::: servo/ports/geckolib/glue.rs:1021 (Diff revision 2) > + PseudoElementCascadeType::Eager => styles.pseudos.get(&pseudo).map(|s| s.rules.clone()), > + PseudoElementCascadeType::Precomputed => unreachable!("No anonymous boxes"), > + PseudoElementCascadeType::Lazy => { > + let d = doc_data.borrow_mut(); > + let guards = StylesheetGuards::same(guard); > + Some(d.stylist.lazy_pseudo_rules(&guards, &element, &pseudo)) When lazy_pseudo_rules returns an Option, the Some(..) here can go away.
Attachment #8865177 - Flags: review?(bobbyholley) → review+
> nsPresContext::HasAuthorSpecifiedRules() are called from nsNodeUtils::ContentRemoved What's the callstack for that? Does it work correctly if the value returned is wrong?
Flags: needinfo?(hikezoe)
Comment on attachment 8865179 [details] Bug 1349651 - Implement has_author_rules for Servo RuleNode. https://reviewboard.mozilla.org/r/136846/#review140172 ::: servo/components/style/rule_tree/mod.rs:880 (Diff revision 2) > + // We need to be careful not to count styles covered up by user-important or > + // UA-important declarations. But we do want to catch explicit inherit styling in > + // those and check our parent style context to see whether we have user styling for > + // those properties. Note that we don't care here about inheritance due to lack of > + // a specified value, since all the properties we care about are reset properties. "style context" is a gecko concept - please make this accurate to servo. ::: servo/components/style/rule_tree/mod.rs:885 (Diff revision 2) > + // > + // FIXME (mbrubeck): The above comment is copied from Gecko, but the last sentence > + // is no longer correct since 'text-shadow' support was added. This is a bug in > + // Gecko, replicated in Stylo for now. Can you file a bug about this and reference it here? ::: servo/components/style/rule_tree/mod.rs:917 (Diff revision 2) > + // This property was set by a non-author rule. Stop looking for it in > + // this element's rule tree. nit: "this element's rule tree path" or "this element's rule nodes". ::: servo/components/style/rule_tree/mod.rs:934 (Diff revision 2) > + } > + } > + } > + } > + // Author rules: > + _ => { Can you explicitly enumerate these so that we'll notice new additions? ::: servo/components/style/rule_tree/mod.rs:947 (Diff revision 2) > + } > + > + if !have_explicit_ua_inherit { break } > + > + // Continue to the parent element and search for the inherited properties. > + element = match element.as_node().parent_element() { nit: TElement has a parent_element(), it's just inherited from selectors::Element: http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/servo/components/selectors/tree.rs#123 ::: servo/components/style/rule_tree/mod.rs:951 (Diff revision 2) > + // Continue to the parent element and search for the inherited properties. > + element = match element.as_node().parent_element() { > + Some(parent) => parent, > + None => break > + }; > + let parent_data = element.mutate_data(); You can just unwrap here. If we have a non-null data then the parent does as well. ::: servo/components/style/rule_tree/mod.rs:953 (Diff revision 2) > + Some(parent) => parent, > + None => break > + }; > + let parent_data = element.mutate_data(); > + let parent_rule_node = parent_data.and_then(|data| > + data.get_styles().map(|styles| styles.primary.rules.clone()) You can just do styles() instead of get_styles and then drop the expect().
Attachment #8865179 - Flags: review?(bobbyholley) → review+
Comment on attachment 8865270 [details] Bug 1349651 - Implement author_colors_allowed in has_author_specified_rules. https://reviewboard.mozilla.org/r/136902/#review140176
Attachment #8865270 - Flags: review?(bobbyholley) → review+
Here is the call stack that HasAuthorSpecifiedRules called from ContentRemoved. (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #16) > > nsPresContext::HasAuthorSpecifiedRules() are called from nsNodeUtils::ContentRemoved > > What's the callstack for that? Does it work correctly if the value returned > is wrong? I have the same question. If we don't return the correct value, we fail to add nsChangeHint_RepaintFrame [1]. But it seems to me that it's probably not a problem since the primary frame is repainted when the element is removed. [1] https://hg.mozilla.org/mozilla-central/file/e0955584782e/layout/base/RestyleManager.cpp#l362
Flags: needinfo?(hikezoe)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #15) > So please replace this with an unwrap(), and then also remove the warning > about unstyled elements below, since the None case will just indicate no > matching rules, rather than that _or_ an error condition (like it does with > this path). > > If that causes failures, we followup bug with the specific failures listed > so that people can investigate them. In that case, please early-return if > mutate_data() returns None, so that we can separate the unexpected Nones > from the expected ones. I made this change and did a another try push; looks like it panics in the same cases that crashed earlier, where HasAuthorSpecifiedRules is called after style data is cleared: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4d2eaf4df72070ba80e6cc575dd4e61d1bf62b2&selectedJob=97404898
How about checking HasServoData before invoking HasAuthorSpecifiedRules?
Oh, one other thing I forgot (the optimization we talked about a few weeks ago) - In the case that IsNativeAnonymous() returns true, we should see if element->GetPseudoElementType() matches the pseudo on the style context. If so, we should just ask servo for the primary (not pseudo) rule node for that element. Not worth blocking the landing, but please file a followup with a patch to do that. r=me with that and a !HasServoData() false early-return In HasAuthorSpecifiedRules.
Blocks: 1363244
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #28) > Not worth blocking the landing, but please file a followup with a patch to > do that. r=me with that and a !HasServoData() false early-return In > HasAuthorSpecifiedRules. Done, and filed follow-up bug 1363244. Updated patches to address all review feedback, rebased onto mozilla-central tip, and pushed to try again to get a final list of test expectations: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4dab7a476c8286bd3b3a46e6e656295b13c56e1
This is a squashed patch without servo changes, rebased to autoland tip, for landing when the Servo change syncs to autoland. (The Servo PR merged while the autoland repo was closed, so I couldn't land this right away. Posting it here in case I'm not around when it syncs.)
Attachment #8866045 - Flags: review+
Pushed by mbrubeck@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/35ec5509fce4 stylo: Implement HasAuthorSpecifiedRules. r=bholley
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/58beae2c80d7 Further test expectation adjustments. r=me
Through no fault of yours, servo vcs sync screwed up the conversion of the Git commit corresponding to PR 16784. We had to rewind history of integration/autoland. This meant deleting changesets referenced in comments 39, 40, and 41. Those changesets will need to be relanded after servo vcs sync is restored (tracked in bug 1363635). If you want me to do it, just needinfo me. It looks like some of the changesets can be combined. So if you want me to do that, give me specific instructions, like who to list as the author for attribution purposes.
Flags: needinfo?(mbrubeck)
Flags: needinfo?(cam)
I can't tell what the contents of the two expectation adjustments are now that the changesets have disappeared. So I think we just have to re-land the main patch (attachment 8866045 [details] [diff] [review], with mbrubeck's attribution) and we'll deal with updating the test adjustments again once it's landed. It's probably simplest if we get you to do that landing after you press the button and the Servo changes have come through, thanks.
Flags: needinfo?(mbrubeck)
Flags: needinfo?(gps)
Flags: needinfo?(cam)
> Here is the call stack that HasAuthorSpecifiedRules called from ContentRemoved. Thanks. I filed bug 1363846 on this; we shouldn't even get that far, ideally.
Pushed by mbrubeck@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d5b141b9583e stylo: Implement HasAuthorSpecifiedRules. r=bholley
mbrubeck and I just raced to reland the main patch. mbrubeck won. Expectations follow-ups need to be relanded still. Per comment #43, that's not my responsibility. So I think my work is done here.
Flags: needinfo?(gps)
https://hg.mozilla.org/mozilla-central/rev/d5b141b9583e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
FYI mbrubeck - the patch that touched all reftest.list files had at least one rebase/merge error, at https://hg.mozilla.org/mozilla-central/rev/d5b141b9583e#l3.126 - it removed the skip-if(webrender) annotation on 672709.html. As it turns out we wanted to remove that anyway (bug 1362424 comment 17) so no harm done, but you may want to check the patch again and make sure there's no other accidental problems with it.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: