Closed Bug 421885 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Layout, defect, P2, trivial)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: davidcorley, Assigned: roc)

References

()

Details

(Keywords: regression, testcase)

Attachments

(10 files, 1 obsolete file)

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.
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
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.
yup. still seeing it with 20080312 nightly
We need some testcase reduction magic here
Attached file 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).
Attached file testcase2
None of the black lines you see, should have a blue-ish line accompanying it.
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?
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
When should I see the 421069 patch? The 20080315 nightly, or does it have to wait until Dbaron signs off on it?
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".
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]
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?
Yes it is fixed. Thanks :)
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.
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
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.
It is that bug 417178 - I had left panel hidden so I lost all topics.
(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?
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 → ---
(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
Oops... redoing roc's changes that I undid by mistake
Priority: P1 → P2
Target Milestone: mozilla1.9beta5 → ---
Attached image image for testcase 3
Little image from Google Reader with a bunch of lines
Attached file 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.
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.
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.
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.
Attached patch fix (obsolete) — Splinter Review
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.
Attached patch fix v2Splinter Review
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]
Attached patch fix for checkinSplinter Review
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
Closed: 12 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
Closed: 12 years ago12 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
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.
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.
Verified as ok at all + zoom levels with 20080331 nightly.
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.