Closed Bug 598080 Opened 14 years ago Closed 5 years ago

when outline-style is 'none', outline-width must compute to '0'

Categories

(Core :: CSS Parsing and Computation, defect, P3)

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

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: crazy-daniel, Assigned: wisniewskit)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: css2, testcase)

Attachments

(2 files, 2 obsolete files)

small testcase
391 bytes, text/html
Details
598080-override_user-specified_outline-width_to_0_if_outline-style_is_none_or_if_inheriting_from_parent_with_outline-style_of_none.diff
4.27 KB, patch
dbaron
: review-
Details | Diff | Splinter Review
Attached file small testcase — 
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; }
Component: Layout: Block and Inline → Style System (CSS)
QA Contact: layout.block-and-inline → style-system
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)
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.
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?
> 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
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 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-
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.
(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.
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 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-
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
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-
needinfo? to make sure you see the review comments
Flags: needinfo?(wisniewskit)
Priority: -- → P3
Thanks, David. I'll try to get to this again soon. Hopefully I'll get it right next time :)
Flags: needinfo?(wisniewskit)

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: 5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: