Closed
Bug 469227
Opened 16 years ago
Closed 16 years ago
redesign mechanism for restricting properties from ::first-line and ::first-letter pseudo elements
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b3
People
(Reporter: dbaron, Assigned: dbaron)
Details
(Keywords: fixed1.9.1)
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
The current mechanism for restricting properties from applying to ::first-line and ::first-letter pseudo elements is somewhat broken; see the comments in nsHTMLCSSStyleSheet that are being removed in this patch.
We could do it much more simply with the flags field in nsCSSProps. Patches coming in a bit.
Note that there are a bunch of recently-added properties that we should have restricted but that we did not.
The only interesting new properties are:
-moz-border-image - just like border, :first-letter but not :first-line
-moz-box-shadow - just like border (ditto)
-moz-text-shadow - both :first-letter and :first-line, I think
word-wrap - just like white-space should have been (although wasn't when we made it work on inlines) -- applies to neither
The current code implicitly blocks some properties (e.g., border-color) by blocking others (e.g., border-style), and also implicitly blocks some by the fact that they will never have any effect. So I'll attach three patches:
* the first patch should give us something equivalent to the current code, but without the deficiencies described in nsHTMLCSSStyleSheet
* the second patch should apply the things that were implicit
* the third patch should fix actual mistakes, so that our list ends up being the list in CSS 2.1 as modified by my comments about new properties above
Flags: blocking1.9.1?
Assignee | ||
Comment 1•16 years ago
|
||
This contains a reftest for a correctness bug fixed by this patch.
Attachment #352647 -
Flags: superreview?(bzbarsky)
Attachment #352647 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•16 years ago
|
||
Attachment #352648 -
Flags: superreview?(bzbarsky)
Attachment #352648 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 3•16 years ago
|
||
Attachment #352649 -
Flags: superreview?(bzbarsky)
Attachment #352649 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 4•16 years ago
|
||
Note that the comment added to nsCSSPropList.h in patch 1 isn't really completely true until all three patches are applied.
Full reftests pass.
Assignee | ||
Comment 5•16 years ago
|
||
... and layout/style mochitests pass.
Updated•16 years ago
|
Attachment #352647 -
Flags: superreview?(bzbarsky)
Attachment #352647 -
Flags: superreview+
Attachment #352647 -
Flags: review?(bzbarsky)
Attachment #352647 -
Flags: review+
Comment 6•16 years ago
|
||
Comment on attachment 352647 [details] [diff] [review]
patch 1: redesign mechanism, don't change which properties are blocked
>@@ -1619,16 +1690,26 @@ nsRuleNode::WalkRuleTree(const nsStyleSt
>+ // A bit in nsCSSProps's flags that has to be present for this
>+ // property to apply.
Maybe:
// If needed, reset the properties that don't have a flag that allows them to be set for this style context.
>+ // Recompute |detail| based on the restrictions we just applied.
>+ detail = CheckSpecifiedProperties(aSID, *aSpecificData);
Hmm. So this is basically relying on the fact that all rules down to our most-specific rule node are rules that apply to the same pseudo-element and don't apply to anything else, right? Otherwise using this detail to propagate dependent and none bits would be wrong.
I wonder whether we can somehow assert this, but at the very least we should document it.
With that looks good, I think. I have to admit that my eyes started glazing over part way through that big header. :(
Updated•16 years ago
|
Attachment #352648 -
Flags: superreview?(bzbarsky)
Attachment #352648 -
Flags: superreview+
Attachment #352648 -
Flags: review?(bzbarsky)
Attachment #352648 -
Flags: review+
Updated•16 years ago
|
Attachment #352649 -
Flags: superreview?(bzbarsky)
Attachment #352649 -
Flags: superreview+
Attachment #352649 -
Flags: review?(bzbarsky)
Attachment #352649 -
Flags: review+
Comment 7•16 years ago
|
||
And the point is, I didn't compare the final header, with all three patches applied, to the spec list. I'm sort of assuming that's been done. ;) I can double-check that if you really want me to, I guess.
Assignee | ||
Comment 8•16 years ago
|
||
(In reply to comment #6)
> >+ // Recompute |detail| based on the restrictions we just applied.
> >+ detail = CheckSpecifiedProperties(aSID, *aSpecificData);
>
> Hmm. So this is basically relying on the fact that all rules down to our
> most-specific rule node are rules that apply to the same pseudo-element and
> don't apply to anything else, right? Otherwise using this detail to propagate
> dependent and none bits would be wrong.
>
> I wonder whether we can somehow assert this, but at the very least we should
> document it.
Er, now that you point this out, I think it's wrong, and I should change back to the code I originally had, which was:
if (detail == eRuleFullMixed)
detail = eRulePartialMixed;
else if (detail == eRuleFullInherited)
detail = eRulePartialInherited;
else if (detail == eRuleFullReset)
detail = eRulePartialReset;
which always produces a more conservative result.
I'll see if I can write a testcase for the bug...
Comment 9•16 years ago
|
||
Oh, right. Rulenodes are per-rule, not per-selector. So a rule which has a selector like ":first-line, .foo" would in fact have a rulenode that would appear on both paths. If that rule specifies a border and the first-line is resolved first, we should get a bug, I'd think.
Assignee | ||
Comment 10•16 years ago
|
||
With the change in comment 8 (although not quite as conservative, since I can compare detail to the result of CheckSpecifiedProperties to see if anything changed), and a test for that (which I wrote before seeing comment 9).
Attachment #352647 -
Attachment is obsolete: true
Attachment #352656 -
Flags: superreview?(bzbarsky)
Attachment #352656 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 11•16 years ago
|
||
Comment on attachment 352656 [details] [diff] [review]
patch 1: redesign mechanism, don't change which properties are blocked
Er, never mind, the problem you're pointing out in comment 9 is yet another issue. Maybe we still need a dummy rule node for properties like this.
Attachment #352656 -
Flags: superreview?(bzbarsky)
Attachment #352656 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 12•16 years ago
|
||
Er, probably I'll just make a pair of dummy rules just like we used to have, except ones that don't do anything.
Comment 13•16 years ago
|
||
The rules will have to have lower specificity than any other rules, right (to separate out the relevant ruletree branches)?
Assignee | ||
Comment 14•16 years ago
|
||
Oh, I had actually been thinking higher, but if I go with lower then I could keep the original adjustment to |detail|. The trick with lower is doing it so that ProbePseudoStyleContext still works.
Comment 15•16 years ago
|
||
I guess we can do higher if we disallow caching above the dummy node in the ruletree. That would certainly make it easier to do ProbePseudoStyleContext...
Assignee | ||
Comment 16•16 years ago
|
||
Putting the extra nodes at the lowest level was actually pretty easy.
Full reftests and layout/style mochitests pass. I added a reftest for the additional case this fixes as well.
Attachment #352656 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Attachment #352672 -
Flags: superreview?(bzbarsky)
Attachment #352672 -
Flags: review?(bzbarsky)
Comment 17•16 years ago
|
||
David, does 469227-2 fail with the first iteration of this patch? I'd expect it to work ok, but for the border to not show up if the <p> and <span> are switched... Maybe we should test both orders?
Updated•16 years ago
|
Attachment #352672 -
Flags: superreview?(bzbarsky)
Attachment #352672 -
Flags: superreview+
Attachment #352672 -
Flags: review?(bzbarsky)
Attachment #352672 -
Flags: review+
Comment 18•16 years ago
|
||
Comment on attachment 352672 [details] [diff] [review]
patch 1: redesign mechanism, don't change which properties are blocked
r+sr=bzbarsky with that nit.
Assignee | ||
Comment 19•16 years ago
|
||
The bug with 469227-2 was that the border did show up on ::first-line even though it shouldn't have.
I'll test the other order as well, though.
Assignee | ||
Comment 20•16 years ago
|
||
Fixed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/3d372e6ff724
http://hg.mozilla.org/mozilla-central/rev/41fd48e76fb0
http://hg.mozilla.org/mozilla-central/rev/ca48a3b92dee
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Updated•16 years ago
|
Attachment #352648 -
Flags: approval1.9.1?
Assignee | ||
Updated•16 years ago
|
Attachment #352649 -
Flags: approval1.9.1?
Assignee | ||
Updated•16 years ago
|
Attachment #352672 -
Flags: approval1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Assignee | ||
Updated•16 years ago
|
Attachment #352648 -
Flags: approval1.9.1?
Assignee | ||
Updated•16 years ago
|
Attachment #352649 -
Flags: approval1.9.1?
Assignee | ||
Updated•16 years ago
|
Attachment #352672 -
Flags: approval1.9.1?
Assignee | ||
Comment 21•16 years ago
|
||
Fixed on mozilla-1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2a49e0977be2
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c187dab6cb9a
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/80136fcf1d21
Keywords: fixed1.9.1
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
You need to log in
before you can comment on or make changes to this bug.
Description
•