Google reader search results have strange red line and broken border around them

VERIFIED FIXED in mozilla1.9

Status

()

P2
trivial
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: davidcorley, Assigned: roc)

Tracking

({regression, testcase})

Trunk
mozilla1.9
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(10 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5pre) Gecko/2008031004 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5pre) Gecko/2008031004 Minefield/3.0b5pre

When carrying out a search in Google Reader using the latest Firefox 3.0 nightly, the border around the news item displays a red line and is not formatted correctly (border is discontinuous)

Reproducible: Always

Steps to Reproduce:
1.Open google reader
2.Search for any news item
3.When the search results appear, click on any of them to see the broken border

Actual Results:  
The new item appears with a corrupt border

Expected Results:  
No corrupted border and no red lines

I've reproduced this on Windows XP and Windows 2000.
(Reporter)

Comment 1

11 years ago
Created attachment 308409 [details]
Screenshot of reader fault
It started with this regression range:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1182366840&maxdate=1182369239
but only if I set a minimum font-size (of 13).
There was also a second regression (more red lines), and I'll try to find the range as soon as I have more time.
Blocks: 367177
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
Version: unspecified → Trunk
Flags: blocking1.9?

Comment 4

11 years ago
Dupe of Bug 404502.  
(In reply to comment #4)
> Dupe of Bug 404502.  
> 
I think so, only in that bug the vertical red lines are not shown on the screen shot. I couldn't reproduce them.
No longer blocks: 367177
Is this still an issue? Could possibly have been fixed by bug 403181.
(Reporter)

Comment 7

11 years ago
yup. still seeing it with 20080312 nightly
We need some testcase reduction magic here
Created attachment 309099 [details]
Unminimized testcase
Created attachment 309111 [details]
testcase

I first thought this was some kind of rounding thing, but the strange thing is, that I see some blue-ish lines (1 horizontal and 1 vertical one), even though I converted the background-images to black and white ones (one consists of horizontal lines, the other one consists of vertical lines).
Created attachment 309115 [details]
testcase2

None of the black lines you see, should have a blue-ish line accompanying it.
(Reporter)

Comment 12

11 years ago
There's some anti-aliasing issues going on here. Some lines look gray when seen at default zoom. If you increase the zoom level, it's clear those lines are black, and I think they should appear black at default zoom also, no?
The seconds testcase shows this, and the first testcase has VERY strange behaviour when zoom is increased. The lines disappear?
Created attachment 309116 [details]
screenshots of testcase2 of trunk and branch
Blocks: 404502
No longer depends on: 404502
Blocks: 376124
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, testcase
Hopefully fixed by patch in bug 421069
Depends on: 421069
(Reporter)

Comment 15

11 years ago
When should I see the 421069 patch? The 20080315 nightly, or does it have to wait until Dbaron signs off on it?
(Reporter)

Comment 16

11 years ago
I'm seeing something else strange. Can't be sure if it started with 20080315 nightly, or before.
In the same search results page where I see the red lines, if I click "Help" on the toolbar, then "About Minefield" and drag the "About Minefield" window over the red lines, gaps start appearing in unwanted red lines and the expected blue border around the result. If I then minimize the minefield window, or cover it with another application window, then make it visible again, the gaps are gone. It's either related to this bug, or there are some repainting issues with the "About Minefield window".
(Reporter)

Comment 17

11 years ago
Created attachment 309656 [details]
Screenshot of repaint issue

Shows the gaps that appear when dragging the "about minefield" window over search a search result in google reader.
Flags: blocking1.9? → blocking1.9+
Assignee: nobody → roc
Priority: -- → P1
Whiteboard: [depends on 421069]

Comment 18

11 years ago
Trouble is with CSS rule background-repeat:repeat-x;

When is it ON then image (line) is aliased (bit blur) AND with red line. When I turn it off then image is small but sharp and without red line...
Target Milestone: --- → mozilla1.9beta5
I'm pretty sure this is fixed by bug 421069, which landed yesterday (and stuck)... can someone check and verify?

Comment 20

11 years ago
Yes it is fixed. Thanks :)

Comment 21

11 years ago
I'm not sure if I'm seeing this bug, bug 403181, or something else similar, but I'm afraid that I still get thin red lines appearing in Google reader when using page zoom, using 2008032106 on Win2k.
Yes, I see that too when zooming in. I'm also seeing that with the unminimized testcase. But it seems to me that is not a bug, but intended.
The Google Reader uses a very weird striped background image, so when zooming in, I can imagine that the background image is placed at a different place, making one of the stripes of the background image visible.

But the main problem is fixed, I think, the blurry stripes you see with the attached screenshots. That blurriness is gone, the lines are now sharp.

Comment 23

11 years ago
I'm sorry, i didn't test zooming (I don't use that). When I zoom in then red lines jumps back. But when I zoom out then whole content disappear... Vista x64

Comment 24

11 years ago
I agree the main issue is fixed.

I also agree that the remaining red lines issue I'm still seeing is not serious. However, it is a change in behaviour since Firefox 2, and I rather doubt that the Google folks intend for random red lines to appear in reader.  So, if all the current zooming/red lines bugs are marked fixed, I think there should be a new bug (maybe it's tech evang if it's decided that Firefox is ok now and Google's site is wrong...)
Firefox 2 doesn't support full Zoom, so it's not possible to make that comparison.
I don't see the issue happening when using text Zoom, btw.
But feel free to file a new bug on this.

The whole content disappearing on zoom out is bug 417178.

Comment 26

11 years ago
It is that bug 417178 - I had left panel hidden so I lost all topics.

Comment 27

11 years ago
(In reply to comment #25)
> Firefox 2 doesn't support full Zoom, so it's not possible to make that
> comparison.

I see your point, but I'd say that adding full zoom was a change ;)  I've made myself a note to file a new bug, but might be a few days before I can make a useful report.
Bug 403181 should have fixed issues with the incorrect parts of background images leaking in, as long as the background is just one tile. Maybe there's still a problem in the tiled case? Do we have a reduced testcase that shows the remaining issue?

Comment 29

11 years ago
The red line issue does seem fixed as of the latest nightly on OS X. Major improvement!

At the risk of going offtopic, bug 423312 is still making Google Reader an unpleasant experience on the Mac.
Moving to P2 for the remaining issue. I think we may need a new testcase and my Windows build is stale, if any of these testcases still show a problem on zooming, or someone can make a new testcase that does, that'd be very helpful...
Priority: P1 → P2
Whiteboard: [depends on 421069]
Target Milestone: mozilla1.9beta5 → ---

Comment 31

11 years ago
(In reply to comment #28)
> Do we have a reduced testcase that shows the remaining issue?

Not as far as I've seen :(  Testcases in bug 403181, bug 421069 and this bug are all ok for me now (including zoom). It's why I hesitated to file a new bug - I can have a go at making a new reduced testcase, but that probably won't be until after the weekend, and reducing a Google Reader page doesn't look easy...
Priority: P2 → P1
Target Milestone: --- → mozilla1.9beta5

Comment 32

11 years ago
Oops... redoing roc's changes that I undid by mistake
Priority: P1 → P2
Target Milestone: mozilla1.9beta5 → ---

Comment 33

11 years ago
Created attachment 311142 [details]
image for testcase 3

Little image from Google Reader with a bunch of lines

Comment 34

11 years ago
Created attachment 311143 [details]
Testcase 3

This shows the remaining problem for me.   The table cell width is limited to 15px, and the background gif position is set at -32px.  Google's image has 4 sections for different types of line, separated by 1px red lines.  With no zoom, the intended section with blue line is shown and nothing else.  Zooming in (or out) at some zoom levels makes a red line (from one side or the other of the intended image section) show up as well.

Comment 35

11 years ago
Given that I got my platform wrong in an earlier comment, I should mention that I'm using 2008032106 on WinXP.  Hope that testcase is useful.

Comment 36

11 years ago
By the way, I also see the issue in comment 16. Loading my testcase, zooming to make a red line appear, and then dragging the about box (or another app e.g. windows calculator) over and away from the red line, the repainting of the red line is inconsistent - sometimes bits of the line disappear, sometimes they reappear.
(Reporter)

Comment 37

11 years ago
FWIW, in Minefield 20080322 I don't see the red lines at zoom levels 0,2,6,8 any more. I do see the lines re-appear at +1 (red at top and bottom) and +3(red at left and top) zoom levels.
News item disappears completely at zoom levels +4, +5 and +7 and also at every zoom level below 0.

Thanks for the testcase!!!
Yeah, so we're obviously taking the DrawTile path here. Ugh.
Created attachment 311332 [details] [diff] [review]
fix

This is the fix for bug 403181 but for the tiled path this time. Hopefully the comments explain what's going on.

The one hitch here is that the reftest doesn't currently pass. I think another pixman bug where it's sampling the wrong pixels :-(. We shouldn't really need to disable the OVER->SOURCE optimization for the tmpSurface path, but we do.
I posted to the cairo list about the pixman issue.
Created attachment 311936 [details] [diff] [review]
fix v2

My understanding of pixman was faulty and we need to take a slightly different approach to get padding right. This patch seems to work well and it's not too complicated. We build an entirely new tile image covering the image data that needs to be tiled, plus any necessary padding. Hopefully the comments explain adequately.
Attachment #311332 - Attachment is obsolete: true
Attachment #311936 - Flags: review?(vladimir)
Whiteboard: [needs review]
Comment on attachment 311936 [details] [diff] [review]
fix v2


Looks fine, but:

>diff -NrpU12 mozilla-trunk.f13044f71bfb/gfx/thebes/src/gfxASurface.cpp mozilla-trunk/gfx/thebes/src/gfxASurface.cpp
>--- mozilla-trunk.f13044f71bfb/gfx/thebes/src/gfxASurface.cpp	2008-03-27 12:42:00.000000000 +1100
>+++ mozilla-trunk/gfx/thebes/src/gfxASurface.cpp	2008-03-27 12:42:00.000000000 +1100
>@@ -58,24 +58,25 @@
> #include <stdio.h>
> #include <limits.h>
> 
> static cairo_user_data_key_t gfxasurface_pointer_key;
> 
> // Surfaces use refcounting that's tied to the cairo surface refcnt, to avoid
> // refcount mismatch issues.
> nsrefcnt
> gfxASurface::AddRef(void)
> {
>     if (mSurfaceValid) {
>         if (mFloatingRefs) {
>+            // XXXroc huh? how can this be right?
>             // eat a floating ref
>             mFloatingRefs--;
>         } else {
>             cairo_surface_reference(mSurface);
>         }

gfx*Surface's refcounts are tied directly to cairo surface refcounts.  Cairo surfaces (all cairo objects) come into life with a refcount of 1, unlike xpcom objects which start at 0 until they're explicitly AddRef'd.  We record that 1 as a floating ref (meaning that the object has been ref'd, but it's not yet known who owns that ref), and whoever calls AddRef first gets "assigned" that ref.  After that, mFloatingRefs goes to 0, and the refcounts match up correctly from then on.
Attachment #311936 - Flags: review?(vladimir) → review+
Okay, that makes sense.
Keywords: checkin-needed
Whiteboard: [needs review] → [needs checkin]
Created attachment 312134 [details] [diff] [review]
fix for checkin

Without the comment that Vlad addressed
Attachment #312134 - Flags: review+
Checking in gfx/public/nsIRenderingContext.h;
/cvsroot/mozilla/gfx/public/nsIRenderingContext.h,v  <--  nsIRenderingContext.h
new revision: 1.83; previous revision: 1.82
done
Checking in gfx/public/nsRect.h;
/cvsroot/mozilla/gfx/public/nsRect.h,v  <--  nsRect.h
new revision: 1.24; previous revision: 1.23
done
Checking in gfx/src/nsRect.cpp;
/cvsroot/mozilla/gfx/src/nsRect.cpp,v  <--  nsRect.cpp
new revision: 3.25; previous revision: 3.24
done
Checking in gfx/src/thebes/nsThebesImage.cpp;
/cvsroot/mozilla/gfx/src/thebes/nsThebesImage.cpp,v  <--  nsThebesImage.cpp
new revision: 1.81; previous revision: 1.80
done
Checking in gfx/src/thebes/nsThebesImage.h;
/cvsroot/mozilla/gfx/src/thebes/nsThebesImage.h,v  <--  nsThebesImage.h
new revision: 1.31; previous revision: 1.30
done
Checking in gfx/src/thebes/nsThebesRenderingContext.cpp;
/cvsroot/mozilla/gfx/src/thebes/nsThebesRenderingContext.cpp,v  <--  nsThebesRenderingContext.cpp
new revision: 1.51; previous revision: 1.50
done
Checking in gfx/src/thebes/nsThebesRenderingContext.h;
/cvsroot/mozilla/gfx/src/thebes/nsThebesRenderingContext.h,v  <--  nsThebesRenderingContext.h
new revision: 1.15; previous revision: 1.14
done
Checking in gfx/thebes/public/gfxPoint.h;
/cvsroot/mozilla/gfx/thebes/public/gfxPoint.h,v  <--  gfxPoint.h
new revision: 1.15; previous revision: 1.14
done
Checking in layout/base/nsCSSRendering.cpp;
/cvsroot/mozilla/layout/base/nsCSSRendering.cpp,v  <--  nsCSSRendering.cpp
new revision: 3.360; previous revision: 3.359
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/421885-1-ref.xml,v
done
Checking in layout/reftests/bugs/421885-1-ref.xml;
/cvsroot/mozilla/layout/reftests/bugs/421885-1-ref.xml,v  <--  421885-1-ref.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/421885-1.xml,v
done
Checking in layout/reftests/bugs/421885-1.xml;
/cvsroot/mozilla/layout/reftests/bugs/421885-1.xml,v  <--  421885-1.xml
initial revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.421; previous revision: 1.420
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
OS: Windows 2000 → All
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: [needs checkin]
Target Milestone: --- → mozilla1.9
Helps if I catch all the files when landing... :(

Checking in layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,v  <--  nsTreeBodyFrame.cpp
new revision: 1.351; previous revision: 1.350
done
This failed the added reftest on Mac, so I backed it out.

REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_osx/mozilla/layout/reftests/bugs/421885-1.xml
Status: RESOLVED → REOPENED
Flags: in-testsuite+ → in-testsuite?
Resolution: FIXED → ---
Sigh. The problem here is simply that I forgot to attach the new test images square-top-bottom-32x32.png and square-left-right-32x32.png. In fact I'm not sure how to do that easily, so I'll just re-land this myself.
Relanded
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
(Reporter)

Comment 51

11 years ago
I'm still seeing the red line issue at +1,3,4 zoom levels. Do I need to write a separate bug report for this?
Also, the search result disappears completely when I use any of the - zoom levels, though I suspect that is a separate bug?
Did you build your own build with the patch? My landing can't possibly have reached the nightly builds yet.
(Reporter)

Comment 53

11 years ago
Sorry Robert,
My reading of your last post was that the test itself hadn't been updated (i.e., the source was fine, but the mac reftest wasn't passing because the test was outdated). So I thought the only changes were to the test.
Will check again once todays (or tomorrows, if the landing didn't make it) nightly builds are out.
(Reporter)

Comment 54

11 years ago
Verified as ok at all + zoom levels with 20080331 nightly.

Comment 55

11 years ago
Also fixed for me: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008033105 Minefield/3.0pre

Thanks roc :)

(In reply to comment #53)
> My reading of your last post was that the test itself hadn't been updated
> (i.e., the source was fine, but the mac reftest wasn't passing because the test
> was outdated). So I thought the only changes were to the test.

That was the reason for the "sigh" in that comment - your understanding of the problem was correct, but the whole thing was backed out because of it (not knowing that that was the issue).

(comment 51)
> Also, the search result disappears completely when I use any of the - zoom
> levels, though I suspect that is a separate bug?

Yes, that's bug 417178
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.