380438 - Black lines appearing on the Opera Desktop Team Blog comments

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel.S]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.0.11) Gecko/20070312 Firefox/1.5.0.11
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070511 Minefield/3.0a5pre ID:2007051106 [cairo]

On the left side of the comments on the Opera Desktop Team Blog (see the given URL) there are black lines appearing.

The regression window is:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070206 Minefield/3.0a2pre ID:2007020611 [cairo] (No bug visible).

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070207 Minefield/3.0a3pre ID:2007020702 [cairo] (Bug appears: black lines).

The black lines are always to the left of the comments text. Sometimes black lines also appear at the bottom of the last text line.

In earlier builds (2007-02-28-04, which I passed by looking for the regression window) the black line on the left expanded partially to 2px when the text was selected/unselected.

Reproducible: Always

Comment 1

[Steve England [:stevee]]

Regression Range

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-02-06+11&maxdate=2007-02-07+03&cvsroot=%2Fcvsroot

Comment 2

[Francesco Castellana]

I'm seeing this on Linux, too; maybe OS should be changed to All

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070514 Minefield/3.0a5pre ID:2007051404 [cairo]

Comment 3

[Eli Friedman]

Attachment: Patch v1

I think this is the right way to fix this.

I don't like views.

Comment 4

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

The nsIViewObserver change is presumably not part of this patch...

+  nsRect newBounds(NSAppUnitsToIntPixels(viewBounds.x, p2a),
+                   NSAppUnitsToIntPixels(viewBounds.y, p2a),
+                   NSAppUnitsToIntPixels(viewBounds.x + viewBounds.width, p2a),
+                   NSAppUnitsToIntPixels(viewBounds.y + viewBounds.height, p2a));
+
+  newBounds.width -= newBounds.x;
+  newBounds.height -= newBounds.y;

This is slightly evil. In my compositor patch I added a new method to nsRect, ScaleRoundPreservingCenters. Want to adopt that here?

+  NS_ASSERTION(!viewRect.x && !viewRect.y, "When exactly is this supposed to be non-zero?");

When the view has child views that overflow to the left or above. Maybe you want to roll that into mViewToWidgetOffset?

Comment 5

[Eli Friedman]

(In reply to comment #4)
> The nsIViewObserver change is presumably not part of this patch...

That's part of bug 379015... I can check it in anytime, though.

> +  nsRect newBounds(NSAppUnitsToIntPixels(viewBounds.x, p2a),
> +                   NSAppUnitsToIntPixels(viewBounds.y, p2a),
> +                   NSAppUnitsToIntPixels(viewBounds.x + viewBounds.width,
> p2a),
> +                   NSAppUnitsToIntPixels(viewBounds.y + viewBounds.height,
> p2a));
> +
> +  newBounds.width -= newBounds.x;
> +  newBounds.height -= newBounds.y;
> 
> This is slightly evil. In my compositor patch I added a new method to nsRect,
> ScaleRoundPreservingCenters. Want to adopt that here?

I've kinda been wondering about that... are we sure we're not going to get an accuracy loss multiplying by the inverse?  Or do we not care?

> +  NS_ASSERTION(!viewRect.x && !viewRect.y, "When exactly is this supposed to
> be non-zero?");
> 
> When the view has child views that overflow to the left or above. Maybe you
> want to roll that into mViewToWidgetOffset?

Yeah, that's what I thought... I patched in the assertion a few days ago, though, and I haven't managed to hit it, so I haven't figured out how to test it.  (It's probably possible with an XUL deck, though... I wish I could get rid of the evil deck frame hackery, but I haven't quite figured it out.)

Comment 6

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

> are we sure we're not going to get an accuracy loss multiplying by the inverse?

round(appunits/p2a) should be equal to round(appunits*(1/p2a)), given that appunits is an integer. floor or ceil would be a problem, yes.

> Yeah, that's what I thought... I patched in the assertion a few days ago,
> though, and I haven't managed to hit it, so I haven't figured out how to test
> it.

Now that views are not used for position:fixed, position:absolute and opacity, it's definitely a lot harder. How about overflow:hidden content where there's overflow to the left or above?

Comment 7

[Eli Friedman]

(In reply to comment #6)
> > are we sure we're not going to get an accuracy loss multiplying by the inverse?
> 
> round(appunits/p2a) should be equal to round(appunits*(1/p2a)), given that
> appunits is an integer. floor or ceil would be a problem, yes.

Okay, fine. (I suppose we don't really care about the case where the number of appunits can't be exactly represented as a float... that would be a really long webpage...)

> > Yeah, that's what I thought... I patched in the assertion a few days ago,
> > though, and I haven't managed to hit it, so I haven't figured out how to test
> > it.
> 
> Now that views are not used for position:fixed, position:absolute and opacity,
> it's definitely a lot harder. How about overflow:hidden content where there's
> overflow to the left or above?

Actually, it's more difficult than that: we won't hit the assertion unless the view has a widget, because Refresh only gets called on views with widgets.  The only places that create widgets at the moment are iframes, plugins, scrollable views for overflow: auto divs, tree bodies, popups, and children of decks.  Except for the children of deck frames, all of those clip their overflow.

(I'd really like to get rid of the children of deck frames creating widgets thing, but I've run into issues.)

Comment 8

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

> (I suppose we don't really care about the case where the number of
> appunits can't be exactly represented as a float

I think we *kinda* care, but not very much. A few rounding-error lines drawn would be OK, crashing or hanging not OK...

Good point about the views with widgets.

Comment 9

[Eli Friedman]

Attachment: Patch v2

New version with nsRect updates from Compositor patch.

Comment 10

[Eli Friedman]

Checked in.  (Note that none of our current automated tests can catch this.)

Comment 11

[Eli Friedman]

It looks like this caused a couple regressions; I'll look into it later.