9816 - Outlines should be disabled completely for FCS

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[alla]

David baron has written some code that does this.
http://www.fas.harvard.edu/~dbaron/tmp/mydiffs5

It's bundled up with a fix for bug 2010 too.

peterl@netscape has this to say about it:
> All the Calc[Margin|Border|Padding]For methods in nsStyleContext are
> deprecated. We've been trying to move all that logic into frame land, so
> I'd rather not see CalcOutlineFor added (which is why I didn't add it).
>
> I haven't had the time to look through the rendering code diffs yet...

I can understand if this is not the best way to do it. But i needed it right
now, and it works. If i got time i can rewrite it if someone tells me how it
should be done.

Comment 1

[kipp]

To quote from peterl:

No. Don't put it in. The paradigm is that the style context will pre-compute and
cache the widths for all of those spacing values if it can
be determined by the context (ie: no percents). Otherwise it's the caller's
responsibility.

Since outline can't have percent values (today), the context should always have
a pre-cached value that you can get from
GetOutlineWidth. If the outline someday accepts percents, then GetOutlineWidth
will simply return PR_FALSE.

There is a bug in the style context currently (sigh) so that GetOutlineWidth
never returns PR_TRUE (probably what prompted the
creation of CalcOutlineWidth). This can easily be fixed by this diff (which I'm
sitting on until my next checkin, feel free to put it in now).
diff -r3.90 nsStyleContext.cpp
640a641
>     mHasCachedOutline = PR_TRUE;

If GetOutlineWidth should ever really return PR_FALSE, computing the actual
width at the call site is trivial:
nscoord outWidth;
if (! spacing->GetOutlineWidth(outWidth)) {
  if (eStyleUnit_Percent == spacing->mOutlineWidth.GetUnit()) {
    outWidth = NSToCoordRound(float(frameWidth) *
spacing->mOutlineWidth.GetPercentValue());
  }
  else {
    outWidth = 0;
  }
}

There's also another bug in there regarding computing the change of the outline
style (David's patch is incomplete). Here's the real diff
for that.
661a663,667
>     }
>     if ((mOutlineWidth != aOther.mOutlineWidth) ||
>         (mOutlineStyle != aOther.mOutlineStyle) ||
>         (mOutlineColor != aOther.mOutlineColor)) {
>       return NS_STYLE_HINT_VISUAL;

There's also the issue that current invalidation code isn't taking the outline
area into account. Since the style system can't know where
the frame is going to draw the outline, I think the frame should take it into
account.
Troy and I have been discussing an API on nsIFrame to compute the invalid area
for a frame (to include children that stick outside), that
API should add in outline area as needed.

Comment 2

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

Yes... I did try GetOutlineWidth first, but since it was always returning
false... I imitated the code that was already there (for borders).  The patch
also needs more work on the rendering end if you actually want to use that part
(rounded borders, invert (if possible...), and table collapsing border model,
and ???).  And it doesn't support outlines on inline elements correctly.  That's
hard, and the spec doesn't define it well.  See my latest rants on www-style.
It would probably be better to make a separate PaintOutline to do this, and find
all the calls to PaintBorder, and...

Outlines would probably be useful for GFX form widgets, too.

I guess I should stick to testing...

Comment 3

[Hixie (not reading bugmail)]

The patch has now been applied, but as mentioned above, 'outline' is currently
not fully implemented. There is a comprehensive test page for this at:
  http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/outline.html

Note that links are currently getting focus outlines -- I am not sure how, since
the outline property is currently not supported on inline elements. Whoever
implements outline on inlines may wish to leverage the focus outline code.
See bug 1859.

Comment 4

[Hixie (not reading bugmail)]

*** Bug 6647 has been marked as a duplicate of this bug. ***

Comment 5

[Peter Linss]

Moving non-beta 1 items to M15

Comment 6

[Pierre Saslawsky]

Reassigning peterl's bugs to myself.

Comment 7

[Pierre Saslawsky]

Accepting peterl's bugs that have a Target Milestone

Comment 8

[Pierre Saslawsky]

Pushing my M15 bugs to M16

Comment 9

[Hixie (not reading bugmail)]

Migrating from {css2} to css2 keyword. The {css1}, {css2}, {css3} and {css-moz}
radars should now be considered deprecated in favour of keywords.
I am *really* sorry about the spam...

Comment 10

[hangas]

This is a big issue for our widgets.  Most of our widgets require an outline and 
it is not working for html:input and xul:titledbutton.  I am trying to land the 
styles for widgets now and things are not working.  These widgets will not look 
right until we can use the outline style.

Comment 11

[Pierre Saslawsky]

This is a layout & clipping issue. Reassigned to Rod Spears who was taking care 
of all the outline bugs at some point. I noticed a few days ago that Karnaze had 
marked Remind/M20 a bunch of outline bugs saying that they won't make it for this 
release. 

Comment 12

[rods (gone)]

reassigning

Comment 13

[karnaze (gone)]

hangas, I have been marking outline bugs as "remind". If this is a high priority 
then we need to rethink our priorities for the first release. To do outlines 
correctly is more than a few days worth of work and layout and rendering folks 
need to sit down and discuss it. CCing rickg, ekrock, buster, kmcclusk. 

Comment 14

[Hixie (not reading bugmail)]

We cannot ship FCS with this still buggy -- this makes the 'outline' property
unusable in many cases. We really should either remove support for 'outline'
altogether or fix this.

Reopening.

Comment 15

[Hixie (not reading bugmail)]

Reassigning to me so that I can work out exactly what the current problem is
and make a minimal test case.

Comment 16

[ekrock's old account (dead)]

My current take: I think it would be an overreaction to desupport outlines in 
FCS. However, I agree with Ian that there seem to be a bunch of bugs right now. 
Ian, we need the following help to figure this out:

1) abstracting from the numerous tests on your test page, could you please list 
for us what the key problems seem to be?
2) would you please note whether each problem is a CSS1 or CSS2 issue?
3) if you could list the related bug #s that would really help!

I think the best course would be to define a subset of all outline functionality 
(preferably including all of the CSS1 features) that we think we can get to work 
reliably, and support that.

Paul: Could you please specify in detail exactly which features of outlining 
*must* work for our widgets to work--we want to minimize dependencies as much as 
possible, so if you can drop the need for various things at the cost of a 
slightly plainer UI, let's please do that. 

Comment 17

[Hixie (not reading bugmail)]

Ok. Outline is a purely CSS2 feature, so there are no CSS1 issues here.

Listing the bugs is going to take somewhat longer:

1. It appears that we don't support 'dotted' and 'dashed' outline-styles at all.
   This is wrong, IIRC we should treat them as 'solid' if we are not able to
   actually treat them as dotted or dashed styles (BUT I MAY BE WRONG THIS NEEDS
   TO BE CHECKED!!!).

2. We do not appear to support the 'invert' outline-color.

3. We have major drawing errors with inset and outset outline-styles.

4. We have some odd, annoying repaint problems (as I understand it, this is
   quite a major problem code-wise). This is bug 9809, marked Future, mostfreq.

5. We interpret 'groove' and 'ridge' the wrong way round.

6. We do not support outline on inline elements (the main use of outline!!!)

7. We draw the outline in the wrong place (as far as I can tell?).

8. According to bug 22892, we don't do 'outline' on XUL -- which I would 
   guess is one of the major uses of outline as far as we are concerned.

Are we currently using it anywhere? A search of the entire Mozilla nightly build
indicates that the only places where we refer to 'outline' are in the DLLs where
we support it, the Test* utilities, and 3 stylesheets... but closer examination
shows that of the 3 stylesheets, 2 of them only refer to outlines to turn them
off (!) and the third is html.css, where it is used, once, to draw a dotted 
outline around focussed links -- and that has no effect anyway.

Given the above bugs, several of which are rather major, the fact that we don't 
(AFAICT) currently provide 'outline' for the two most important uses of it 
(inline elements and XUL), and the fact that we don't use it anywhere, I would 
suggest we *remove* support for 'outline' altogether, and Future support for it.

Comment 18

[Hixie (not reading bugmail)]

As per meeting with ChrisD yesterday, taking QA.

Reassigning to Pierre. As noted above, there are some remaining cases where
'outline' has *some* effect, albeit a rather buggy one. We should disable
outlines altogether for RTM.

Nominating for nsbeta3. This is a relatively low-risk change, since as far as
we can tell, nothing uses 'outline' anymore, and in any case 'outline' is
currently waaay too buggy to be shipped as is.

Comment 19

[Pierre Saslawsky]

That's an easy one: fix in hand / nsbeta3.

Comment 20

[Marc Attinasi]

Approving for nsbeta3

Comment 21

[Pierre Saslawsky]

Fix checked in nsCSSParser.cpp

Comment 22

[Hixie (not reading bugmail)]

VERIFIED.
No sign of outlines when using 'outline'. 
(They are now in fact called '-moz-outline').