921713 - nsRuleNode::HasAuthorSpecifiedRules should handle inherited properties differently?

Status: RESOLVED INCOMPLETE

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

In nsRuleNode::HasAuthorSpecifiedRules there is this comment:

  // 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.

text-shadow is one of the properties we look at now, though, after bug 721750, and that's an inherited property.  Is there something in the loop below that that we need to change to handle the inherited property?  Otherwise, might it be possible that HasAuthorSpecifiedRules(NS_AUTHOR_SPECIFIED_TEXT_SHADOW) returns false when it should return true, if it finds a eCSSUnit_Null value for text-shadow?

This might only ever come up if we had say:

  div { text-shadow: 2px 2px 0 red; }
  span { letter-spacing: inherit; }

in an author or UA style sheet, and then with this content:

  <div><span>select me</span></div>

the selected text should paint with a text shadow but wouldn't.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: untested patch

Something like this?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The original use of HasAuthorSpecifiedRules was to decide whether to disable native theme.  What does the current set of uses look like, especially for the callers that care about text-shadow?

Comment 3

[Cameron McCormack (:heycam)]

So the only use of HasAuthorSpecifiedRules that cares about text-shadow is here:

  https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#3590

That's not to disable any native theming, but to allow a ::-moz-selection rule that doesn't have a text-shadow declaration to not clobber any text-shadow specified on the element without that pseudo.

Comment 4

[Boris Chiou [:boris]]

Just notice this bug on my triage monitor. Do we still need this because we don't have nsRuleNode now?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

That code lives here now: https://searchfox.org/mozilla-central/rev/1133b6716d9a8131c09754f3f29288484896b8b6/servo/components/style/rule_tree/mod.rs#1331

Comment 6

[Cameron McCormack (:heycam)]

Emilio, I assume there is nothing to be fixed in the new code and this bug can be closed?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Yeah, I think so.