Closed
Bug 421885
Opened 17 years ago
Closed 17 years ago
Google reader search results have strange red line and broken border around them
Categories
(Core :: Layout, defect, P2)
Core
Layout
Tracking
()
VERIFIED
FIXED
mozilla1.9
People
(Reporter: davidcorley, Assigned: roc)
References
()
Details
(Keywords: regression, testcase)
Attachments
(10 files, 1 obsolete file)
101.20 KB,
image/png
|
Details | |
8.13 KB,
text/html
|
Details | |
1.31 KB,
text/html
|
Details | |
1.34 KB,
text/html
|
Details | |
14.00 KB,
text/html
|
Details | |
130.29 KB,
image/png
|
Details | |
427 bytes,
image/gif
|
Details | |
326 bytes,
text/html
|
Details | |
31.98 KB,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
30.96 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
Comment 2•17 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.9?
Comment 3•17 years ago
|
||
Comment 4•17 years ago
|
||
Dupe of Bug 404502.
Comment 5•17 years ago
|
||
(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
Assignee | ||
Comment 6•17 years ago
|
||
Is this still an issue? Could possibly have been fixed by bug 403181.
Reporter | ||
Comment 7•17 years ago
|
||
yup. still seeing it with 20080312 nightly
Assignee | ||
Comment 8•17 years ago
|
||
We need some testcase reduction magic here
Comment 9•17 years ago
|
||
Comment 10•17 years ago
|
||
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).
Comment 11•17 years ago
|
||
None of the black lines you see, should have a blue-ish line accompanying it.
Reporter | ||
Comment 12•17 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?
Comment 13•17 years ago
|
||
Updated•17 years ago
|
Updated•17 years ago
|
Reporter | ||
Comment 15•17 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•17 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•17 years ago
|
||
Shows the gaps that appear when dragging the "about minefield" window over search a search result in google reader.
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → roc
Priority: -- → P1
Assignee | ||
Updated•17 years ago
|
Whiteboard: [depends on 421069]
Comment 18•17 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...
Updated•17 years ago
|
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•17 years ago
|
||
Yes it is fixed. Thanks :)
Comment 21•17 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.
Comment 22•17 years ago
|
||
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•17 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•17 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...)
Comment 25•17 years ago
|
||
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•17 years ago
|
||
It is that bug 417178 - I had left panel hidden so I lost all topics.
Comment 27•17 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.
Assignee | ||
Comment 28•17 years ago
|
||
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•17 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.
Assignee | ||
Comment 30•17 years ago
|
||
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•17 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•17 years ago
|
||
Oops... redoing roc's changes that I undid by mistake
Priority: P1 → P2
Target Milestone: mozilla1.9beta5 → ---
Comment 33•17 years ago
|
||
Little image from Google Reader with a bunch of lines
Comment 34•17 years ago
|
||
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•17 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•17 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•17 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.
Assignee | ||
Comment 38•17 years ago
|
||
Thanks for the testcase!!!
Assignee | ||
Comment 39•17 years ago
|
||
Yeah, so we're obviously taking the DrawTile path here. Ugh.
Assignee | ||
Comment 40•17 years ago
|
||
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.
Assignee | ||
Comment 41•17 years ago
|
||
I posted to the cairo list about the pixman issue.
Assignee | ||
Comment 42•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
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+
Assignee | ||
Comment 44•17 years ago
|
||
Okay, that makes sense.
Keywords: checkin-needed
Whiteboard: [needs review] → [needs checkin]
Assignee | ||
Comment 45•17 years ago
|
||
Without the comment that Vlad addressed
Attachment #312134 -
Flags: review+
Comment 46•17 years ago
|
||
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: 17 years ago
Flags: in-testsuite+
Keywords: checkin-needed
OS: Windows 2000 → All
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: [needs checkin]
Target Milestone: --- → mozilla1.9
Comment 47•17 years ago
|
||
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
Comment 48•17 years ago
|
||
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 → ---
Assignee | ||
Comment 49•17 years ago
|
||
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.
Assignee | ||
Comment 50•17 years ago
|
||
Relanded
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Reporter | ||
Comment 51•17 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?
Assignee | ||
Comment 52•17 years ago
|
||
Did you build your own build with the patch? My landing can't possibly have reached the nightly builds yet.
Reporter | ||
Comment 53•17 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•17 years ago
|
||
Verified as ok at all + zoom levels with 20080331 nightly.
Comment 55•17 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.
Description
•