417178 - Google reader does not show subscribed topics when zooming in

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Brandon Ehle]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3

Google Reader does not display the topic pane in firefox 3 beta 3.  Double checked and it was working in firefox 3 beta 2 and below.

It seems like a layout related bug because if you examine the document with DOM inspector you can find the missing text in the DOM tree, it just never renders to the screen.

If you click around in the missing pane in the DOM


Reproducible: Always

Steps to Reproduce:
1. Load google reader
2. Subscribe to some RSS feeds with a previous version of firefox, note the topics in the lower right hand pane
3. Load the same page with firefox 3 beta 3
Actual Results:  
All of the RSS feed topics are missing from the window.

Expected Results:  
Render the RSS feed topics that are visible in firefox 3 beta 2 and below.

Comment 1

[Brandon Ehle]

Neant to add that if you if you click around in the DOM inspector on the text that does not appear, it will eventually show up in the main screen instead of everything else that was rendering properly initially.

Comment 2

[Brandon Ehle]

Looking at it some more, when you resize the screen it reveals the missing text, but it appears to be off the bottom of the screen.

Comment 3

[Brandon Ehle]

Attachment: Screenshot showing the incorrect layout.

Comment 4

[Alessio Bolognino]

I can confirm this bug on MacOSX (Leopard) and Firefox 3 beta3 (works fine with Firefox beta2)

Comment 5

[Brian Polidoro]

It seems to be an issue with zooming.  This only happens to me at some zoom settings.  

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008021304 Minefield/3.0b4pre ID:2008021304

Comment 6

[Brian Polidoro]

Ok    2008-01-28
broke 2008-01-29

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=2008-01-28+04%3A00%3A00&maxdate=2008-01-29+04%3A00%3A00&cvsroot=%2Fcvsroot

maybe bug 403660 or bug 134706

Comment 8

[Brandon Ehle]

I can confirm that this is zoom related.

If I use View/Zoom/Reset in the menu to manually force the zoom back to default it makes the page render correctly.

Using a variety of zoom settings in firefox 3 beta 2, I haven't been able to find a zoom setting (both zooming in and out) that results in the page rendering incorrectly.  Doing the same test in firefox 3 beta 3 results in a number of different incorrect layouts.

Comment 10

[Brian Ewins]

The nav-toggler (the arrow between the navbar and the content, that toggles the navbar on and off) seems to be partly to blame; the zoom problem goes away if that is removed, eg with this Stylish userscript:

@namespace url(http://www.w3.org/1999/xhtml);
@-moz-document url-prefix("http://www.google.com/reader") {
#nav-toggler { display:none }
}

And with the next script you can see that in zoomed view #viewer-box-table (the story cards) is displaying below the bottom of the nav-toggler

@namespace url(http://www.w3.org/1999/xhtml);
@-moz-document url-prefix("http://www.google.com/reader") {
#nav-toggler { height:100px !important }
}

And this one shows the bug fixed again by changing the width of the story cards:

@namespace url(http://www.w3.org/1999/xhtml);
@-moz-document url-prefix("http://www.google.com/reader") {
#nav-toggler { height:100px !important }
#viewer-box-table {width:99% !important} /* was 100% */
}

So it appears as if the nav-toggler is managing to affect the layout of some content inside the '#chrome' div but not others. Weird.

Comment 11

[Brian Ewins]

Attachment: reduced testcase

Reduced from google reader. Nav area is coloured green, nav-toggler is red, title of stories section says 'title here', the misbehaving content says 'Stories here'. When you load this and zoom in 1 step in FF3b3, the 'Stories here' jumps to below the red nav-toggler.

Comment 12

[Brian Ewins]

(In reply to comment #11)
> 'Stories here'. When you load this and zoom in 1 step in FF3b3, ...

Obviously I meant to say 'zoom /out/ 1 step'

Comment 13

[KENZ (Kenji Irie)]

This bug remains...
per reduced testcase, NG and OK zoom levels are:

NG: 50%, 60%, 70%, 80%, 90%, 150%, 160%, 170%, 180%, 190%, 240%, 250%, 260%, 270%, 280%
OK: 100%, 110%, 120%, 130%, 140%, 200%, 210%, 220%, 230%, 290%, 300%

It looks like a rounding problem. (maybe NS_floor(x + 0.5) or like?)

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

+ regression keyword, adding some people to cc list from comment 6.

Comment 15

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

Could somebody test an hourly builds archive and narrow the regression range?

Comment 16

[Brian Polidoro]

(In reply to comment #15)
> Could somebody test an hourly builds archive and narrow the regression range?
> 
What hourly archive?  http://hourly-archive.localgho.st/ only has builds for the last 15 days.  

Comment 17

[Martijn Wargers (dead)]

Maybe someone else has still those hourly builds in that time period.

Comment 18

[Steve England [:stevee]]

Works: 20080128_2340_firefox-3.0b3pre.en-US.win32
Broken: 20080129_0018_firefox-3.0b3pre.en-US.win32

Checkins to module PhoenixTinderbox between 2008-01-28 23:40 and 2008-01-29 00:18 :
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=2008-01-28+23%3A40&maxdate=2008-01-29+00%3A18&cvsroot=%2Fcvsroot

Comment 19

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

So probably bug 134706

Comment 20

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

Attachment: further reduced testcase

Reduced some more. Definitely looks like a rounding issue, probably exposed by bug 134706 rather than introduced by it.

Comment 21

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

So in the case where I'm failing, I'm seeing mAvailSpaceRect with a width of 18680 appunits but aCBWidth which was passed to OneWidthToClearFloats is 18700.

Comment 22

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

Okay, so what happens here is that that 1px border gets rounded to device pixels, so it's actually 80 appunits wide, 20 appunits wider than when it wasn't zoomed, and so of course it sticks out a tiny bit past the left margin of the second DIV and the 100%-width overflow:auto DIV fails to fit.

Off the top of my head I'm not sure what we should do about this. We really do need to round border widths at layout time and that's going to lead to issues like this where widths don't add up as expected. David, any ideas?

Comment 23

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

Unless someone comes up with a compelling argument that there's a better way to do this, this can't block.

Comment 24

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

And this is probably WONTFIX, although I won't mark it so until there's a consensus for that larger than just me.

Comment 25

[Michaël Arnauts]

Isn't it possible to round downwards? 
I don't know exactly how the zoomcode works, but i know google reader isn't the only site that has this behaviour...

WONTFIX looks like a bad idea for me...

Comment 26

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

Rounding downwards is an interesting idea. It won't help when the user zooms out, since we won't want borders to disappear entirely, but it should prevent problems like this when zooming in and it would be pretty safe.

Comment 27

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

Attachment: fix

This does it. Running reftests now.

Comment 28

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

Actually reftests are kind of a waste of time since they don't use zoom, but whatever.

Comment 29

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

Did you check that this matches the way at least some other browsers round border widths?

Comment 30

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

Yeah, it matches Safari 3 and IE7.

Comment 31

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

Attachment: reftest

Note that Safari and IE actually round borders <1px down to zero, which we don't want to do.

Comment 32

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

Comment on attachment 311296 [details] [diff] [review]
fix

r+sr=dbaron

Comment 33

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

The reftest should be checked in too.

Comment 34

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

I'm restoring the blocking status I removed in comment #23.

Comment 35

[Reed Loden [:reed]]

Checking in layout/style/nsStyleStruct.h;
/cvsroot/mozilla/layout/style/nsStyleStruct.h,v  <--  nsStyleStruct.h
new revision: 3.132; previous revision: 3.131
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/417178-1-ref.html,v
done
Checking in layout/reftests/bugs/417178-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/417178-1-ref.html,v  <--  417178-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/417178-1.html,v
done
Checking in layout/reftests/bugs/417178-1.html;
/cvsroot/mozilla/layout/reftests/bugs/417178-1.html,v  <--  417178-1.html
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.407; previous revision: 1.406
done

Comment 36

[Reed Loden [:reed]]

I had to back this out due to one mochitest failing. I left the reftests checked-in, but I removed them from reftest.list.

*** 21976 ERROR FAIL | undefined | got "5px", expected "6px" | /tests/layout/style/test/test_bug412901.html

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

roc: what's the blocking status now that this was backed out due to test failure?

Comment 38

[Martijn Wargers (dead)]

It seems to me that test needs to be changed, because it should now round downwards to "5px" instead of "6px".

Comment 39

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I agree with Martijn -- that test is checking that a border-bottom-width: 5.5px gets rounded to 6px.  With this patch it will (intentionally) get rounded to 5px, right?

Comment 40

[Mike Schroepfer]

(In reply to comment #39)
> I agree with Martijn -- that test is checking that a border-bottom-width: 5.5px
> gets rounded to 6px.  With this patch it will (intentionally) get rounded to
> 5px, right?
> 

If so can we just mod the test?

Comment 41

[Reed Loden [:reed]]

(In reply to comment #40)
> If so can we just mod the test?

I'll modify the test and reland this shortly.

Comment 42

[Reed Loden [:reed]]

Checking in layout/style/nsStyleStruct.h;
/cvsroot/mozilla/layout/style/nsStyleStruct.h,v  <--  nsStyleStruct.h
new revision: 3.134; previous revision: 3.133
done
Checking in layout/style/test/test_bug412901.html;
/cvsroot/mozilla/layout/style/test/test_bug412901.html,v  <--  test_bug412901.html
new revision: 1.2; previous revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.411; previous revision: 1.410
done

Comment 43

[Mats Palmgren (inactive)]

Did this bug cause the current reftest failures on qm-centos5-01
and qm-win2k3-01?

Comment 44

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #43)
> Did this bug cause the current reftest failures on qm-centos5-01
> and qm-win2k3-01?

Yes. It looks like the reftests need to be updated, I'm doing that now.

Comment 45

[:Gavin Sharp [email: [e-mail]]]

Attachment: reftest changes

Since we're now rounding down rather than up, I added 1 to all the tests that expected x.5 to round to x+1 (so they're now testing that x+1.5 rounds to x+1). Some of the tests in these files did not require any changes, and I'm not sure why.

This makes the failing tests pass locally, so I'm going to land this to get the tree green. If there's a problem with these changes we can address them separately, or just back out the original patch.

Comment 46

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

Comment on attachment 311600 [details] [diff] [review]
reftest changes

I think a much simpler and probably more correct way to fix this would be to just make border-*5 and border-*6 have the same reference as border-*4 rather than using border-*10 as a reference.

If that works, could you do that instead and revert these changes?

Comment 47

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

(In other words, you should be changing only the reftest.list -- to change which reference is used for these tests -- and reverting the changes to the tests.)

Comment 48

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

Comment on attachment 311600 [details] [diff] [review]
reftest changes

Gavin pointed out that three tests weren't changed.  Maybe I put those tests in the wrong file, but I guess this is ok, at least for now.

Comment 50

[Michael Lefevre]

Think that dupe is filed with a build containing the fix, and I also still see half of the originally reported issue in Google reader and in "further reduced testcase" (using 2008033105 on WinXP) - it now only happens with negative zoom levels (i.e. 90%, 80%, etc). Reopen this? New bug?

Comment 51

[David Corley]

I didn't see that this had been closed (doh! Though it was still open). I'll re-open 426193.

Comment 53

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

Changed summary to indicate what was fixed here. I reopened bug 426193 to  

Comment 54

[Frank Harper]

Confirmed fixed with 

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5) Gecko/2008040514 Firefox/3.0b5