9809 - [outline]Dynamic changes of outline properties aren't repainted

Status: RESOLVED FIXED

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

Description

[alla]

When the outline property of some content changes (eg. it tracks focus with
outline) the difference in the styles are not noticed byt the style system,
leading to no repaint.

The cause of this is that StyleSpacingImpl::CalcDifference doesn't compare
outline data.

Comment 1

[alla]

Attachment: Fixes the problem

Comment 2

[Pierre Saslawsky]

Reassigning peterl's bugs to myself.

Comment 3

[Pierre Saslawsky]

Accepting peterl's bugs that have a Target Milestone

Comment 4

[Pierre Saslawsky]

Pushing my M15 bugs to M16

Comment 5

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

Is this still a problem?  I know there are still problems with outline
repainting, but are they with the *region* repainted?

Comment 6

[Pierre Saslawsky]

The style code was fixed a while ago to generate a repaint when the outline 
changes (StyleSpacingImpl::CalcDifference() was returning NS_STYLE_HINT_VISUAL). 
Problem: a repaint wasn't sufficient to dynamically update the display. See the 
the end of the "[e-mail]  1999-07-15 13:26" comment in bug 9816 for more 
info. In order to work around some aspects of the problem, I changed the code to 
return NS_STYLE_HINT_REFLOW.

We are now left with 2 problems which are both related to the outline area that 
Kipp and Troy were talking about in bug 9816.
- We shouldn't need to return NS_STYLE_HINT_REFLOW when the outline changes.
- The outline doesn't paint over surrounding frames. See the testcase that I'm 
going to attach.

Reassigned to Troy to add the outline area in nsIFrame.

Comment 7

[Pierre Saslawsky]

Attachment: testcase

Comment 8

[troy]

Rod, I think you're the one who's doing outlines so re-assigning to you

Comment 9

[rods (gone)]

changed summary

Comment 10

[rods (gone)]

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

Comment 11

[rods (gone)]

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

Comment 12

[rods (gone)]

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

Comment 13

[Sean Richardson]

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

Comment 14

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

I've been thinking about a workaround to fix this (unless someone wants to write
an invalidation API like kipp mentioned in bug 9816), and, well I want to write
down my thoughts here to see what others think about it and so I don't forget
them (and so I can close all these LXR windows I have open).

Anyway, it might work to:

* add an additional style change hint called NS_STYLE_HINT_DOUBLE_VISUAL between
NS_STYLE_HINT_VISUAL and NS_STYLE_HINT_REFLOW

* change nsStyleSpacing::CalcDifference at
http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsStyleContext.cpp#790
to return the new NS_STYLE_HINT_DOUBLE_VISUAL (meaning the frame would have to
be invalidated with both old and new data)

* Change
http://lxr.mozilla.org/seamonkey/source/layout/base/public/nsStyleChangeList.h &
http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsStyleChangeList.cpp so
that nsStyleChangeData also has an nsCOMPtr<nsIStyleContext> previousContext (or
maybe make it a raw pointer and/or use it only for outline hints???)

* change CaptureChange at
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsFrameManager.cpp#1136
to store the old style context using the above change

* Change ProcessRestyledFrames at
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp#9099
to call something that does the proper invalidation (this is the part I haven't
thought through completely yet).

That still leaves issues with views and the problems that Pierre's attachment
demonstrates... but does this seem like a sensible approach for propogating the
style information?  (The problem I'm talking about fixing still exists,
right...?)

Comment 15

[rods (gone)]

moving to M17

Comment 16

[rods (gone)]

Outlines paint on the outside of a frame and curently there is no mechanism 
built into frame to make the view large enough to contain the frame plus outline

Comment 17

[karnaze (gone)]

Marking remind. This will not make release 1.

Comment 18

[R.K.Aa.]

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

Comment 19

[Christopher Blizzard (:blizzard)]

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

Comment 20

[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 21

[Pierre Saslawsky]

See my notes in bug 24676: it looks like outlines have been disabled already.

Comment 22

[karnaze (gone)]

Kevin, can you or Rod do the workaround that was discussed. There is not enough 
time for the more elaborate solution. 

Comment 23

[Kevin McCluskey (gone)]

Rod, what's the status on supporting outline?

Comment 24

[rods (gone)]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another 
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration. 

Comment 25

[R.K.Aa.]

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

Comment 26

[R.K.Aa.]

bug 18579 alone had 7 dups before marked dup of this
Adding mostfreq keyword.

Comment 27

[Hixie (not reading bugmail)]

As per meeting with ChrisD yesterday, taking QA.

Comment 28

[Jason Eager]

blah. Anything I can do to help?

Comment 29

[R.K.Aa.]

on linux the outline of dropdown boxes don't vanish before you click in page.
Many bugs about that phenomena have been marked dup of this bug.
Many of them - identical - have also instead been marked dup of bug 39992
39992 has 11 dups already. Is it a dup of this one or not?

Comment 30

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

Bug 39992 has nothing to do with the 'outline' property.

Comment 31

[rods (gone)]

jce2, we would love to help on this issue except it is quite involved which is 
why we are postponing it until 6.x.

The issue is that outlines need to painted on the outside of a frame. Frames are 
contained inside Views. In some cases the view may have to be enlarged to hold 
the frame plus the outline (like absolutely positioned elements). Then there are 
all the issues with invalidating/clipping/painting just the outline which may 
actually (or most likely) be part of the frame's parent frame. Frames know about 
views, but views don't know about frames. So this all seemed like a bit too much 
to bite off for 6.0.

But we are very open to any research or investigation in this area. And if you 
were to find that the impact turned out to be small and of lower risk we could 
always consider it for 6.0, or at the very least it would give us a great jump 
start on the issue for 6.x  Thank you for offeeing your help, let us know if you 
are still interested in looking into this issue.

Comment 32

[R.K.Aa.]

David Baron: if 39992 has nothing to do with this bug, then many of the dups
here aren't dups at all.

Comment 33

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

You're right.  All the dups of this bug since May (I think -- haven't checked
quite all yet) are really dups of bug 39992.  This is a totally different bug. 
Those bugs should not have been verified duplicates of this bug.  People
verifying dups should be more careful.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

When bug 13213 is fixed, we should be able to take a big bite out of this bug by 
ensuring that HTMLReflowMetrics::mOverflowArea contains the outline area (if 
any). Then the view will automatically be sized and positioned to include the 
outline area.

Comment 35

[rods (gone)]

marking invalid, because we currently aren't implmenting this per the spec.

Comment 36

[Hixie (not reading bugmail)]

Reopening. Once we switch '-moz-outline' to 'outline' then we'll have to fix 
this bug.

Comment 37

[Hixie (not reading bugmail)]

Reassigning to Robert to get this off Rod's open bug list. Robert: I hope that's
ok. If it's not then feel free to reassign it to me and I'll keep it; however
I'm likely to forget about it if it is assigned to me as I don't look at my
assigned bugs list very regularly! :-)

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The view manager work that landed last milestone has made this bug solvable, but
it's still not 1.0 material.

Comment 39

[Oddsson]

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

Comment 40

[Bill Mason]

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

Comment 41

[Stefan Borggraefe]

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

Comment 42

[R.K.Aa.]

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

Comment 43

[Aaron Leventhal]

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

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We're getting close to being able to fix this. Soon I should land code that
computes a correct overflowArea for outlined elements, so views for outlined
elements will be sized correctly.

There's still the problem of what to repaint when outline changes. dbaron
described a way that involves looking at the old style data. I think an easier
way would be to add a style hint that forces invalidation of a frame's entire
view, nsChangeHint_RepaintView.

There are other issues in the duped bugs, like outline painting not respecting
clipping. We'll have to look at those duped bugs before we can decide we have
working outline.

Comment 45

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

There's also the issue of outlines on elements that don't have views.

See also the discussion in bug 151375.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

RepaintView would repaint the nearest containing view containing the frame, so
taking care of frames without views as well.

There certainly are various other things that would need to be fixed. For
example during reflow we'd have to invalidate the outline area whenever we
invalidate the border area. And other things I haven't thought of...

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We'll have to modify event handling so that mouse events in the outline area go
to the outlined element. I think.

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

A better approach would be to simply ensure that a frame with 'outline' always
has a view.

Comment 49

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

You want to create and destroy views every time the focus changes?

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We only need to create a view for a frame when it first becomes focused. There's
no harm in leaving a view attached to a frame that doesn't really need it
anymore, so there's no need to destroy views when the outline is removed.

If even that is considered too expensive, we can improve the way we create
views. Right now we do a FRAMECHANGE if a view might be needed after a style
change. As you know plans are well advanced to add a FrameNeedsView hint which
will do a FRAMECHANGE only if there isn't already a view. We can even improve
that by instead of upgrading FrameNeedsView to a FRAMECHANGE, just creating a
new view for the existing frame, inserting it into the view hierarchy, and
reparenting the views for the descendant frames of the restyled frame to point
to the restyled frame's new view. That would be followed by a reflow of the
restyled frame to give the new view the correct geometry.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That would be a lot faster than a FRAMECHANGE and also bypass the brokenness of
the event state manager w.r.t. FRAMECHANGEs.

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

One other thing: the style hint for 'outline-width' will end up looking like
nsChangeHint_FrameNeedsView | nsChangeHint_RepaintView | nsChangeHint_ReflowFrame

We need to reflow when the width changes so that the overflow area(s) get
updated correctly.

Comment 53

[Daniel Wang]

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

Comment 54

[Daniel Wang]

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

Comment 55

[Aaron Leventhal]

The testcase no longer works. It doesn't seem like we support css outline at all.

Comment 56

[Aaron Leventhal]

Attachment: Test case using -moz-outline instead of outline (it's currently called that because outline has too many bugs)

Comment 57

[Aaron Leventhal]

Using -moz-outline the test case works for me.

Comment 58

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

The point is that outline (outside the border) is harder than -moz-outline
(inside the border) and this bug needs to be fixed before we really support outline.

Comment 59

[Aaron Leventhal]

Are we still going to support -moz-outline, or are we just going to make it work
outside the frame and rename it outline then.

I thought it was just temporarily renamed to -moz-outline into it worked correctly.

Comment 60

[Aaron Leventhal]

Fixed by checkin to bug 151375.

Comment 61

[Mick Langford]

Hey Guys! Good Job!