266122 - Outline applied on a multi-line element should encompass all the line boxes and stay connected (join outlines together)

Status: NEW

(Core :: Layout, defect)

Description

[Aaron Leventhal]

One an element is broken up into several layout frames, the CSS outline should
be drawn around the entire collection of frames. It should not be drawn as
several overlapping outlines.

This needs to take into account -moz-outline-radius and -moz-outline-offset as well.

Comment 1

[Aaron Leventhal]

Attachment: Testcase

Comment 2

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

There are some spec issues here, I think. How should the outline be drawn in a
situation like this?

<style>.vot::first-line { opacity:0.5; }</style>
<div class="vot">
  <span style="outline:2px solid red;">Hello<br>Kitty</span>
</div>

So we still need to break up the unified outline into pieces that belong to
individual frames and paint each piece when its frame paints. But we need to
make sure that no part of the unified outline is painted twice or we'll get
incorrect results for rgba() colors. We may also need to think about how dotted
or dashed borders are going to work.

I thought this interacted with z-ordering too, but as far as I can tell it
really doesn't.

So in PaintOutline, we look at the other frames for the content and figure out
what the union outline is and which part this frame should paint. Here's how
this could work. There are two regions associated with each outline: the
interior (the area enclosed by the outline's inner edge) and the exterior (the
area enclosed by the outline's outer edge). Say that a frame paints its regular
outline clipped to exclude all the exteriors of its prev-in-flow frames'
outlines and to exclude all the interiors of its next-in-flow frames' outlines.
This will guarantee that no point in any interior is painted, and every point in
the union of the outlines is painted exactly once.

I think we could actually implement it this way by building clip regions, even
for outline-radius by gluing together 1-pixel-high rectangles. It might be a bit
slow for frames with a large outline-radius but we could optimize by only doing
the clipping thing when we know the frame's overflow area intersects with other
frames in its in-flow list.

Comment 3

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

That doesn't address getting dots and dashes right. That seems really tricky. To
fix that, we'd have to do the clipping thing but instead of painting the frame's
outline normally, compute the geometric path for the union outline and paint
that. That would require taking the post-segmentation polygon for each frame
outline, figuring out intersections and which segments are the boundary
(difficult since it has width), and then painting that.

Comment 4

[Hixie (not reading bugmail)]

The outline for an element should be rendered all at once, not piecemeal. So 
there isn't a problem here. You'd render the outline for that inline way after 
you've rendered the rest of the block (in fact you should do it at the end of 
the stacking context).

Comment 5

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

But in that example, the first line of the <span> element is in a different
stacking context from the second line.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[John Mellor (Jomel)]

Attachment: changes from 1.0.4 to 1.0+

On the 19/6/05 nightly, rendering has improved for the testcase in this bug,
but still fails for the testcase in bug 280087 (attachment 172646 [details]), as shown in
this collection of screenshots.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050619
Firefox/1.0+

Comment 8

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

Now that bug 317375 has landed, outlines for all frames for a single element are painted consecutively in z-order whenever possible. We could copy the merging logic from nsDisplayOpacity to nsDisplayOutline, or use some other approach, to ensure that the outlines for a single element are painted in a single operation. Cairo drawing APIs are available through Thebes in cairo builds, so we should be able to go ahead and fix this now in cairo builds.

The only remaining problem is to figure out exactly how this should be implemented in its full generality. The idea in comment #2 would be fairly easy to do with cairo but it doesn't handle dotted and dashed outlines well.

Comment 9

[Wayne Woods]

Out of interest, is this likely to fix bug 328193?

Comment 10

[Uri Bernstein]

I don't know about bug 328193, but speaking of dependencies, I'm pretty sure it should fix bug 337274 :-)

Comment 11

[F.Rajkumar]

Guys is there any patch available for this.

Comment 12

[Martijn Wargers (dead)]

Attachment: testcase2

Comment 14

[j.j.]

Attachment: testcase3

IE8, Safari and Opera do this.

Comment 15

[j.j.]

(The second example in testcase3 is probably another bug)

Comment 18

[Gérard Talbot]

Testcase from CSS2.1 test suite
-------------------------------

http://test.csswg.org/suites/css2.1/latest/html4/outline-layout-004.htm

http://test.csswg.org/suites/css2.1/nightly-unstable/html4/outline-layout-004.htm


Relevant chunk of spec
----------------------
"
Outlines may be non-rectangular. For example, if the element is broken across several lines, the outline is the minimum outline that encloses all the element's boxes. In contrast to borders, the outline is not open at the line box's end or start, but is always fully connected if possible.
"
18.4 Dynamic outlines: the 'outline' property
http://www.w3.org/TR/CSS21/ui.html#dynamic-outlines

Comment 19

[Gérard Talbot]

The inaccuracy of the bug summary makes it difficult to find this bug report; "glob" is not a real english verb, otherwise, it is a word difficult to understand or to translate.

Bug summary changed to:
Outline applied on a multi-line element should encompass all the line boxes and stay connected.

I considered "wrap around", "enclose" and "surround".

-------------

(In reply to j.j. (inactive in 2012) from comment #15)
> (The second example in testcase3 is probably another bug)

Such exaple is interesting because there is no test for such kind of line box scenario at CSS 2.1 test suite: a line box with a few inline boxes of various line-heights.
I'm working on a test to be submitted based on your 2nd example in testcase3. 

When the spec says "Outlines may be non-rectangular. For example, if the element is broken across several lines, the outline is the minimum outline that encloses all the element's boxes.", then I think it suggests that a line box with several inline boxes of various line-heights should be enclosed using a non-rectangular outline.

Gérard

Comment 20

[Gérard Talbot]

> (...) there is no test for such kind of line
> box scenario at CSS 2.1 test suite: a line box with a few inline boxes of
> various line-heights.
> I'm working on a test to be submitted based on your 2nd example in
> testcase3.

Recently submitted test at CSS 2.1 test suite:
http://test.csswg.org/suites/css2.1/nightly-unstable/html4/outline-layout-006.htm

IE9, IE10, Opera 12.12 and Opera 12.50 pass this test.

I'll create another bug report for this one.

Gérard

Comment 21

[Gérard Talbot]

> I'll create another bug report for this one.

Bug 825058: Outline applied on element with a few inline boxes of various line-heights should be minimal and should encompass all the inline boxes and stay connected

Comment 23

[Gérard Talbot]

With 'writing-mode' set to 'vertical-rl'
http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/outline-inline-vrl-012.xht

With 'writing-mode' set to 'vertical-lr'
http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/outline-inline-vlr-013.xht

Chrome 50.0.2661.94, IE11 and Edge pass these 2 tests.

Comment 24

[Michal Čaplygin [:myf]]

Attachment: testcase3 rendered in IE11 and Google Chrome 52 and Firefox 48

For reference: attaching image capture displaying testcase3 rendered in current browsers.

Comment 25

[Gérard Talbot]

Testcase from CSS2.1 test suite
-------------------------------

http://test.csswg.org/suites/css2.1/latest/html4/outline-layout-004.htm

http://test.csswg.org/suites/css2.1/nightly-unstable/html4/outline-layout-004.htm

That test is now in the NeedsWork category because the shape of the outline border (non-rectangular) is not specified by the CSS2.x spec (and not even in CSS2 UI). Only the "stay connected" and "enclose all content" items are required by the CSS2.x spec.


Therefore, I have removed the

http://test.csswg.org/suites/css2.1/nightly-unstable/html4/outline-layout-006.htm

test.

The tests

With 'writing-mode' set to 'vertical-rl'
http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/outline-inline-vrl-012.xht
 
With 'writing-mode' set to 'vertical-lr'
http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/outline-inline-vlr-013.xht

have been updated accordingly and will be submitted soon.

Comment 26

[Gérard Talbot]

> the shape of the outline
> border (non-rectangular) is not specified by the CSS2.x spec (and not even
> in CSS2 UI). 

I meant to say
(...) is not specified by the CSS2.x spec (and not even in CSS3 UI).

Comment 27

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 30

[Matt Woodrow (:mattwoodrow)]

Carrying on conversation from bug 1624061.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

I was thinking of making an nsDisplayOutline just on the first continuation and make it responsible for drawing all the other outlines, but that seems that it may not play well with invalidation...

Matt, do we have a precedent for something like this?

Indeed invalidation can be quite a pain for cases like this, and we really prefer to have each display item be based on the layout/style data from a single frame for that reason.

There are however indeed a few cases where we deal with this sort of thing.

One is nsDisplayItemBase::GetDependentFrame, which lets you register a second frame from which data is read. The DL invalidation code will handle invalidating the item if either of the frames change. It's possible that we could extend this to support a list of frames, but DL code can be very perf sensitive (due to giant lists), so adding size or indirections can be painful.

Another example is invalidating items for border-collapse tables. For this, the various table component frames override nsIFrame::InvalidateFrame, and propagate the invalidation up to their parent so that it eventually invalidates the root of the table. We could do something similar by adding a check in nsIFrame::MarkNeedsDisplayItemRebuild which checks if we had outlines as last paint (state bit?), and are a continuation, and if so also invalidate the primary frame. The same performance concerns apply.

(In reply to Miko Mynttinen [:miko] from comment #5)

We do display item merging by putting them together in one temporary item[1]. Based on the comment, it seems this was also meant to be used by nsDisplayOutline. Merging happens at the display list iteration level, in FlattenedDisplayListIterator[2].

This code is very messy because it previously required mutating the display items, which we cannot do with retained display lists. There is also a second problem with retained display lists: the display item ordering can change during display list merging, which means that we cannot rely on items staying consecutive (it should still be the common case). As it is, the code only works for container based display items.

If it's not too hard to extend the merging code to support leaf items, then I think it might be worth trying to support merging for outlines, given that we're already paying the complexity cost of supporting merging for other items (and have no real path forward to removing that).

Maybe this "grouping of display items" is better done during display list building or display list merging rather than painting?

As above, created merged items in the retained list means that we need to handle invalidating them correctly when any of the input frames changes, which isn't super fun.

I think it'd also be interesting to look at whether it's possible to merge during the Gecko DL -> WR DL conversion, or on the WR deserialization/scene building side.

Comment 32

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 8 duplicates.
:dholbert, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 33

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Comment 34

[Thomas Wisniewski [:twisniewski]]

Unsetting the webcompat priority as no sites seem negatively affected by this.