746015 - Column rules are not properly drawn on overflow columns

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Scott Johnson (:jwir3)]

Attachment: testcase, scroll to the right to see overflow columns without column rules

While debugging bug 733416, I came across this issue. Specifically, when overflow columns are created within an nsColumnSetFrame, columns that are out of the viewport do not have their column rules drawn properly. (See attached screenshots and testcase).

As per Example 27 of http://dev.w3.org/csswg/css3-multicol/ it appears that overflow columns should have column-rules drawn like normal column boxes. Regardless of whether or not we draw column rules for overflow columns, we should be maintaining consistent behavior between overflow columns regardless of whether they are in the view or not.

If you zoom out on the testcase so that all columns are visible, the column rules for the overflow columns are then drawn.

This could be a regression, but I'm not sure yet.

Comment 1

[Scott Johnson (:jwir3)]

Attachment: screenshot of the test case without scrolling

This is a screenshot of the testcase as it's loaded initially.

Comment 2

[Scott Johnson (:jwir3)]

Attachment: screenshot after scrolling to the right

This is a screenshot of the same testcase, same rendering, but scrolled to the right to view columns that were initially out of view.

Comment 3

[Scott Johnson (:jwir3)]

Attachment: screenshot of OPERA's behavior with the same test case, scrolled

This is a screenshot from Opera 11.62 with the same test case, scrolled to the right to see the overflow columns.

Comment 4

[Scott Johnson (:jwir3)]

Attachment: screenshot of current nightly after zooming out so all columns are visible

This is a screenshot of the initial view of the testcase, but after zooming out so all columns are visible.

Comment 5

[Scott Johnson (:jwir3)]

This doesn't appear to be a regression - I went back as far as I could on hg (around 2007) and it seems to be exhibiting the same behavior.

Comment 7

[Daniel Holbert [:dholbert]]

bug 870162 might be related to this.

Comment 8

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

It seems like this is likely to be because nsColumnSetFrame uses nsDisplayGeneric for the thing that paints column rules, and nsDisplayGeneric uses as its bounds the frame bounds.  (I'm assuming that these rules are outside the frame's bounds but inside its overflow area, though that's worth checking.)  Seems like we'd perhaps want to add either a boolean flag or a subclass of nsDisplayGeneric that uses the frame's overflow area (rather than its rect) as the display item's bounds.  Alternatively, we could just subclass nsDisplayItem, though it seems like a pattern that might come up elsewhere.

Comment 9

[Scott Johnson (:jwir3)]

Attachment: b746015

This is an implementation of the second approach proposed in comment 8 - I added a subclass of nsDisplayGeneric, called nsDisplayGenericOverflow, that performs the same functionality as nsDisplayGeneric, but with the default bounds set to the visual overflow rectangle instead of the frame's bounds.

Comment 10

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

Comment on attachment 792830 [details] [diff] [review]
b746015

>+/**
>+ * Generic display item that can contain overflow. Use this in lieu of
>+ * nsDisplayGeneric if you have a frame that should use the visual overflow
>+ * rect of its parent frame when drawing items, instead of the frame's bounds.

"parent frame" -> "frame"

>+class nsDisplayGenericOverflow : public nsDisplayGeneric {
>+  public:
>+    nsDisplayGenericOverflow(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
>+                     PaintCallback aPaint, const char* aName, Type aType)

fix indent

>+    /**
>+     * Similar to default nsDisplayGeneric::GetBounds(), but uses the frame's
>+     * visual overflow rect instead of the frame's bounds.
>+     *
>+     * This does not take the item's clipping into account.
>+     *
>+     * @param aSnap A pointer to a boolean which is set to true if the returned
>+     *        rect will be snapped to nearest device pixel edges during actual
>+     *        drawing. It might be set to false and snap anyway, so code
>+     *        computing the set of pixels affected by this display item needs to
>+     *        round outwards to pixel boundaries when *aSnap is set to false.
>+     *
>+     * @returns A rectangle relative to aBuilder->ReferenceFrame() that
>+     * contains the area drawn by this display item
>+     */

This doesn't need such a big comment; most of it is documented in what it's overriding.  Just say that it's returning the visual overflow (i.e., basically the second half of the first sentence of the above).

>+    virtual nsRect GetBounds(nsDisplayListBuilder* aBuilder, bool* aSnap)

Please use MOZ_OVERRIDE.

>+    {
>+      *aSnap = false;
>+      nsRect rect(Frame()->GetVisualOverflowRect());
>+      return rect + ToReferenceFrame();

You should be able to replace the last 2 lines with just:
  return Frame()->GetVisualOverflowRect() + ToReferenceFrame();

>diff --git a/layout/reftests/columns/columnrule-overflow-ref.html b/layout/reftests/columns/columnrule-overflow-ref.html

Using text in the test to fill a certain size makes it unnecessarily brittle.  Seems like it would be better to use a series of empty (or colored) inline-blocks or something like that.

Also, no need to use calc(-570px); could just use -570px.

Does the test reliably fail without the patch?  (It should if the rules visible in the test are all outside the frame.)

r=dbaron with those things fixed

Comment 11

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #10)
> "parent frame" -> "frame"

Fixed.

> fix indent

Fixed.

> This doesn't need such a big comment; most of it is documented in what it's
> overriding.  Just say that it's returning the visual overflow (i.e.,
> basically the second half of the first sentence of the above).

Fixed.

> Please use MOZ_OVERRIDE.

Ok, I'll add this.

> You should be able to replace the last 2 lines with just:
>   return Frame()->GetVisualOverflowRect() + ToReferenceFrame();

Fixed.

> Using text in the test to fill a certain size makes it unnecessarily
> brittle.  Seems like it would be better to use a series of empty (or
> colored) inline-blocks or something like that.

Yes, I realized this after I posted the patch for review. I used a blue-colored div with width= the width of the column box, and height = 2 columns in the reference, and 2+ columns + the width of the column set frame in the test.
 
> Also, no need to use calc(-570px); could just use -570px.

Fixed.

> Does the test reliably fail without the patch?  (It should if the rules
> visible in the test are all outside the frame.)

Yes, it does reliably fail without the patch. I verified this prior to implementing the solution.

Comment 12

[Scott Johnson (:jwir3)]

Inbound, with fixes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8cbc29a72e

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5b8cbc29a72e