Closed
Bug 598080
Opened 14 years ago
Closed 6 years ago
when outline-style is 'none', outline-width must compute to '0'
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: crazy-daniel, Assigned: wisniewskit)
References
(Blocks 1 open bug, )
Details
(Keywords: css2, testcase)
Attachments
(2 files, 2 obsolete files)
|
(deleted),
text/html
|
Details | |
|
(deleted),
patch
|
dbaron
:
review-
|
Details | Diff | Splinter Review |
Like border-width, outline-width should compute to '0' (zero) if the style is set to 'none'.
Currently, the code below draws a red outline around #child.
#parent { outline: 20px none blue }
#child { outline-color: red; outline-style: solid; outline-width: inherit; }
|
||
Updated•14 years ago
|
Component: Layout: Block and Inline → Style System (CSS)
QA Contact: layout.block-and-inline → style-system
|
||
Comment 1•14 years ago
|
||
|
|
||
Updated•14 years ago
|
Blocks: css2.1-tests
|
||
Comment 2•12 years ago
|
||
|
Assignee | |
Comment 3•8 years ago
|
||
Here's a patch to bring Firefox in line with Chrome, WebKit and Edge, all of which pass the test in comment 2. If it seems reasonable then I'll add a reftest based on the test-case.
Try seems alright with it, at least: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d446d8f8c1b8
Attachment #8794561 -
Flags: review?(dbaron)
|
|
||
Comment 4•8 years ago
|
||
Too tired to review right now, but one note: the tricky part of this review is checking that the code does the right thing with aStartStruct in the interesting cases. The interesting cases are where multiple relevant (outline-style, maybe also outline-width) declarations apply to an element, but only some of those declarations apply to another element earlier in the document that matches the exact same sequence of rules other than the last one(s), and which caches the struct in the rule tree.
|
|
Assignee | |
Comment 5•8 years ago
|
||
So basically, whether or not the case I added (when outlineStyleExplicitlySetToNone) should call aConditions.SetUncacheable()? You're right, that could be tricky. I had taken a look at border-width in layout/style/test/property_database.js to see if outline-width seemed to be testing things the same way, but maybe that wasn't enough?
|
|
||
Comment 6•8 years ago
|
||
> If it seems reasonable then I'll add a reftest based on the test-case.
Thomas,
I just did it in CSS2.1 Test suite:
http://hg.csswg.org/test/rev/1b3276301e95
http://hg.csswg.org/test/rev/9ede4a0725d6
http://test.csswg.org/source/css21/ui/?C=M;O=D
|
|
Assignee | |
Comment 7•8 years ago
|
||
Thanks Gérard!
I'm guessing that I should still import a copy of that reftest into the Firefox tree as part of this bug's final patch, right David?
|
|
||
Comment 8•8 years ago
|
||
Comment on attachment 8794561 [details] [diff] [review]
598080-set_outline-width=0_when_outline-style=none.diff
So I think the thing I was worried about is in fact a problem.
In particular, I suspect if you have a test like this:
<!DOCTYPE HTML>
<style>
p { outline-style: none }
p.foo { outline-width: 20px }
</style>
<p>one</p>
<p class="foo">two</p>
Then the outline-width calculation for the second paragraph will happen without outlineStyleExplicitlySetToNone (because you'll have a non-null aStartStruct coming from the style we cached while computing style for the first paragraph) and you'll end up with a 20px computed outline-width, but if you remove the first paragraph that won't happen and you'll end up with 0px because of the code you added.
Also, the initial value of outline-style is 'none', so anything that happens for an explicit 'none' should also happen for one that results from the initial value. So it's not clear to me why you want to depend on the 'none' being explicit.
I think solving this correctly requires having two different outline widths in the style struct, just like nsStyleBorder has mBorder and mComputedBorder (where the latter has the effects of 'border-style: none' included, but the former does not).
Attachment #8794561 -
Flags: review?(dbaron) → review-
|
|
Assignee | |
Comment 9•8 years ago
|
||
Actually, there already is code for an analogous mActualOutlineWidth property, and also a check in nsStyleOutline::RecalcData which checks for the style to be none and sets the width to 0.
The problem is that in this test the element's outline-style isn't none, but rather the parent's. And so the element inherits the width from its parent, and because there is no code to adjust the parent's width to 0, it inherits the wrong value.
Does that alleviate your concerns, or should the check be made somewhere else to cover this case?
>Also, the initial value of outline-style is 'none', so anything that happens for an explicit 'none' should also happen for one that results from the initial value.
Agreed. However, there are a couple of test-failures with initial/unset value expectations when I change the logic to just always check if the outline-style is none, explicitly or not, in layout/style/test/test_initial_computation.html:
>failed | initial should cause initial value for 'outline-width' - got "3px", expected "0px"
>failed | initial should cause initial value for 'outline-width' - got "3px", expected "0px"
>failed | unset should cause initial value for 'outline-width' - got "3px", expected "0px"
>failed | unset should cause initial value for 'outline-width' - got "3px", expected "0px"
I'm not yet sure what the issue is there, however.
|
|
||
Comment 10•8 years ago
|
||
(In reply to Thomas Wisniewski from comment #9)
> Actually, there already is code for an analogous mActualOutlineWidth
> property, and also a check in nsStyleOutline::RecalcData which checks for
> the style to be none and sets the width to 0.
>
> The problem is that in this test the element's outline-style isn't none, but
> rather the parent's. And so the element inherits the width from its parent,
> and because there is no code to adjust the parent's width to 0, it inherits
> the wrong value.
>
> Does that alleviate your concerns, or should the check be made somewhere
> else to cover this case?
So in both the border and outline cases, mComputedBorder/mActualOutlineWidth differs from mBorder/mOutlineWidth in two ways: the former has the effects of border/outline-style included, and also is rounded to device pixels. For inheritance, we're required to include the first of these effects, and required not to include the second. (In the outline case there's also a third difference, which is that the enumerated values aren't resolved. That difference is just a waste of space in the style struct; there's no need to store the enumerated values in the style struct.) So the code in both places (outline and border) is currently wrong.
In the border case, nsRuleNode::ComputeBorderData uses mComputedBorder for inheritance. In the outline case, nsRuleNode::ComputeOutlineData uses mOutlineWidth.
The code to handle inheritance could easily start from mBorder/mOutlineWidth and reapply the fixup for the parent's *-style being 'none' to avoid also picking up the rounding. (We do need to store the value without that fixup to handle the non-null aStartStruct case.)
> >Also, the initial value of outline-style is 'none', so anything that happens for an explicit 'none' should also happen for one that results from the initial value.
>
> Agreed. However, there are a couple of test-failures with initial/unset
> value expectations when I change the logic to just always check if the
> outline-style is none, explicitly or not, in
> layout/style/test/test_initial_computation.html:
>
> >failed | initial should cause initial value for 'outline-width' - got "3px", expected "0px"
> >failed | initial should cause initial value for 'outline-width' - got "3px", expected "0px"
> >failed | unset should cause initial value for 'outline-width' - got "3px", expected "0px"
> >failed | unset should cause initial value for 'outline-width' - got "3px", expected "0px"
>
> I'm not yet sure what the issue is there, however.
I'd try fixing the known bugs here and see if it goes away.
|
|
Assignee | |
Comment 11•8 years ago
|
||
Yes, thanks, that does feel like the right thing to do (at least, this new patch is what I think you were suggesting as the fix). I have a try run ongoing here just in case, but it is indeed passing the tests I mentioned earlier: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d408a4f79b39
I take it that if this patch is fine, I should just add the CSS 2.1 test to it (as a reftest), and make any changes to the border code (if necessary) in a separate patch? (I haven't had time to investigate the border code yet; it seems a fair bit different).
Assignee: nobody → wisniewskit
Attachment #8794561 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8796905 -
Flags: review?(dbaron)
|
|
||
Comment 12•8 years ago
|
||
Comment on attachment 8796905 [details] [diff] [review]
598080-set_mActualOutlineWidth=0_when_inheriting_outline-width_and_outline-style_is_none.diff
Doing this as after-the-fact fixup in RecalcData rather than just doing it right the first time in ComputeOutlineData seems like an odd approach.
This approach also produces incorrect results for inheritance happening twice, e.g.:
<div style="outline-style: none">
<div style="outline-width: inherit; outline-style: solid">
<div style="outline-width: inherit; outline-style: solid">
</div>
</div>
</div>
where it will incorrectly produce a nonzero with on the innermost div. When the parent style is none and we inherit the parent's width, then mOutlineWidth should be 0 (not just mActualOutlineWidth). This is different from mActualOutlineWidth becoming 0 when the style is none, since in that case a more specific rule might override the style and require the outline width to be restored to a nonzero value from mOutlineWidth.
Attachment #8796905 -
Flags: review?(dbaron) → review-
|
|
Assignee | |
Comment 13•8 years ago
|
||
Okay. Unless I'm still missing something, this patch should be what you had in mind. I'll request a review when you're back, dbaron. Try seems fine with it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c38b350e8dd95d7a922614274dcd138da86f3f8d
Note that I had to let the case where unit==CSSUnit_None fall through, as well as to SetUncacheable. Otherwise various test would fail for initial values, which makes sense to me.
I take it that I ought to also add a copy of Gérard's reftest in comment 6 to the tree?
Attachment #8796905 -
Attachment is obsolete: true
|
|
||
Updated•8 years ago
|
Attachment #8804554 -
Flags: review?(dbaron)
|
|
||
Comment 14•8 years ago
|
||
Comment on attachment 8804554 [details] [diff] [review]
598080-override_user-specified_outline-width_to_0_if_outline-style_is_none_or_if_inheriting_from_parent_with_outline-style_of_none.diff
Sorry again for the delay getting to this.
>+ // If the user specifies an outline-width, but the outline-style is none,
"user" isn't appropriate here because that sounds like you're talking
about user style sheets. Please reword to avoid the terms "user" or
"author", since this covers values specified in any level of the
cascade.
But this patch still seems wrong in a number of ways:
* It's treating explicit 'initial' and 'unset' values differently
from all other values, when the same rules apply to them. The same
applies to the initial value (when unspecified).
* It again fails to consider the start struct, and I believe will
produce incorrect results (i.e., a computed value of 20px when it
should produce 0) for the inverse of the test case from comment 8,
i.e.:
<!DOCTYPE HTML>
<style>
p { outline-width: 20px }
p.foo { outline-style: none }
</style>
<p>one</p>
<p class="foo">two</p>
* It's unnecessarily (relative to alternative approaches) using
SetUncacheable.
I think you should still take the approach of storing 2 values in the
style struct (like for border) as I suggested in the last paragraph in
comment 8 and in comment 10.
Attachment #8804554 -
Flags: review?(dbaron) → review-
|
|
||
Comment 15•8 years ago
|
||
needinfo? to make sure you see the review comments
Flags: needinfo?(wisniewskit)
|
||
Updated•8 years ago
|
Priority: -- → P3
|
|
Assignee | |
Comment 16•8 years ago
|
||
Thanks, David. I'll try to get to this again soon. Hopefully I'll get it right next time :)
Flags: needinfo?(wisniewskit)
|
|
||
Comment 18•6 years ago
|
||
Firefox 60.4.0 ESR and Firefox 66.0a1 buildID=20190109214248 pass
http://test.csswg.org/suites/css2.1/latest/html4/outline-width-096.htm
http://test.csswg.org/suites/css21_dev/nightly-unstable/html4/outline-width-096.htm
Resolving as WORKSFORME
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•