Closed Bug 430813 Opened 16 years ago Closed 16 years ago

Wikipedia pages that are a part of a portal aren't rendered with the portal links directly to the right of the content, causing a large white space to appear before the content of the page

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9

People

(Reporter: mmortal03, Assigned: dbaron)

References

(
URL
)

Details

(Keywords: regression, testcase, top100, Whiteboard: [has patch][has review][has approval])

Attachments

(7 files)

IE & FF screen shot
209.04 KB, image/png
Details
Firefox with large white space, clean profile
115.42 KB, image/png
Details
mostly simplified testcase
660 bytes, text/html; charset=UTF-8
Details
slightly simpler testcase
842 bytes, text/html; charset=UTF-8
Details
patch
5.20 KB, patch
roc
: review+
roc
: superreview+
damons
: approval1.9+
Details | Diff | Splinter Review
simplified testcase #2
398 bytes, text/html; charset=UTF-8
Details
patch #2
12.03 KB, patch
roc
: review+
roc
: superreview+
mtschrep
: approval1.9+
Details | Diff | Splinter Review 
Wikipedia pages that are a part of a portal aren't rendered with the portal links directly to the right of the content, when placed at the top of the Wikipedia page content source, causing a large white space to appear.  

Example case: http://en.wikipedia.org/w/index.php?title=Cognitive_dissonance&oldid=207223476

Internet Explorer and Safari render it in manners that don't show the large white space (placing the portal links directly to the right of the content).
Attached image IE & FF screen shot — 
please attach screen shot of where they look different

WFM. They look exactly the same to me - IE 7 and Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre)
(In reply to comment #1)
> Created an attachment (id=318078) [details]
> IE & FF screen shot
> 
> please attach screen shot of where they look different
> 
> WFM. They look exactly the same to me - IE 7 and Mozilla/5.0 (Windows; U;
> Windows NT 6.0; en-US; rv:1.9pre) 
> 

Hmm, this could be a common add-on causing this, because I had others over at the Mozillazine forums state that they saw the bug, as well.  I will check this in my clean testing profile and get back to you in a minute.
Clean profile, no extensions or plugins.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042705 Minefield/3.0pre ID:2008042705
Wayne, I just went back and tested it with your 2008040606 build, and indeed got the results you did, so this is, in fact, a regression that has occurred since then.
Regression occurred after the 2008041406 trunk. In the builds following that, starting with 2008041506, I see the bug.
Keywords: regression
I don't see anything like the screenshot in today's nightlies, are you still seeing it?
(In reply to comment #7)
> I don't see anything like the screenshot in today's nightlies, are you still
> seeing it?
> 

In the latest Windows hourly, I still see it.  Which build are you using?
ah, sorry. I was looking at the current Cognitive Dissonance page rather than the actual link in comment 0. I didn't notice the oldid= query argument, and when I add that I can reproduce the problem. (ftr: 2008042906 and 2008043007)
URL: http://en.wikipedia.org/w/index.php?t...
Nominating for 1.9 because I don't know how serious this might be in general use, but FF3 seems to be the only browser that doesn't display the test URL as intended.
Flags: blocking1.9?
Confirmed using 
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008043004 Minefield/3.0pre

OS / Hardware -> All/All
OS: Windows XP → All
Hardware: PC → All
It does need to be a table rather than an overflow:auto div; the % margin is not required, and the problematic margin is the margin-right.
I have a feeling it is Bug 427782, but I could be wrong.  Your test case seems very similar to that bug.  Could someone back that one out of the 20080415 trunk build and see what it does?
OS: All → Windows XP
Hardware: All → PC
OS: Windows XP → All
Hardware: PC → All
Assignee: nobody → dbaron
Attached patch patch — — Splinter Review 
This fixes the bug, and full reftests pass (including the two I added).  (Except zwnj-02, which always fails for me.)

ComputeReplacedBlockOffsetsForFloats has already done the subtraction that the addition I'm removing in this patch was doing.  (It subtracted the value from the number we're subtracting here, and two -s are a +.)

Note that in the case where mAvailSpaceRect.width == mContentArea.width, it doesn't do that subtraction, but it doesn't need to since (assuming positive margins) doing it would be undone by the leftOffset = PR_MAX(leftOffset, 0).  And in the simple case for negative margins we're probably best off with zero offsets anyway, so I think that's fine.

I would like to get this in for 1.9.
Attachment #318723 - Flags: superreview?(roc)
Attachment #318723 - Flags: review?(roc)
Attachment #318723 - Flags: superreview?(roc)
Attachment #318723 - Flags: superreview+
Attachment #318723 - Flags: review?(roc)
Attachment #318723 - Flags: review+
Comment on attachment 318723 [details] [diff] [review]
patch

I think we should fix this for 1.9.  I think this is the correct fix, and it's also conservative in that it's going to cause the clearing behavior that's new in 1.9 to happen less often (well, except for the case of negative margins, but that's very rare).

Reftests included in patch, and passes existing reftests.
Attachment #318723 - Flags: approval1.9?
Comment on attachment 318723 [details] [diff] [review]
patch

a1.9+=damons
Attachment #318723 - Flags: approval1.9? → approval1.9+
Fix checked in to trunk.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
This isn't fixed at all from what I can see in today's Trunk.  No change can be seen in the rendering behavior using the link from my initial description.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050206 Minefield/3.0pre ID:2008050206
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The even simpler testcase (attachment 318703 [details]) seems to work for me with a latest-nightly, but the other testcase and URL still fail.

dbaron: do you still think this blocks? it is pretty noticable on wikipedia ...
Ah, true.  I probably forgot to check the original site amid all the other testing I was doing...

Though now the problem is only present at some screen widths (wide enough, and it goes away).


I want to try to figure out what it simplifies to to figure out whether it should block; I'll try to get back to you shortly.
And this testcase did regress in the same window, although it's not clear to me why since it really shouldn't have been affected by that patch...
(In reply to comment #20)
> The even simpler testcase (attachment 318703 [details]) seems to work for me with a
> latest-nightly, but the other testcase and URL still fail.
> 
> dbaron: do you still think this blocks? it is pretty noticable on wikipedia ...
> 

Just my two cents about blocking, since I didn't make this explicit in the original description, but this happens on quite a few pages where a template is inserted on the very first line of the article, and Wikipedia is a obviously a very high traffic site. The "frame" of links on these selected pages that is causing the problem are implemented by using the template feature (two curly braces encasing the name of the template).

I can't verify whether it will happen on EVERY page with a template included at the very first line, as there may be different classes of templates that one can create, some that may span the entire page. (I am not an expert on Wikipedia's code).  But this does happen on a good number of pages.  I couldn't find a way to query the exact number of them, though.
I can verify now that not all template pages do it, but only ones like this one where it has these kind of divs.  Here is the template page in question if you didn't already take a look at it, and you can click "edit this page" to see the div html code in the first few lines, if you are curious.

http://en.wikipedia.org/wiki/Template:Psychology
Well, I figured out why this case regressed.

I'd realized before what was happening -- we're computing the width to clear past the floats when the current position is above the current collapsed margin rather than with it applied, which means that we're using a different available width than the one we should be using.

The key point that I didn't realize until after stepping through the old code is that this actually isn't a regression.  It's just that before the patch in bug 427129, we weren't using the available width, so it didn't matter.  (We were using the intrinsic minimum width instead.)
Attached patch patch #2 — — Splinter Review 
This patch does in fact fix Wikipedia.

It's not as complicated as it looks -- all I'm really doing here is delaying the call to the width computation (and doing it a little more often, although in the normal case it's just twice instead of once, and shouldn't be a problem) until we're at the correct vertical position.  So I'm basically pushing the width computation inside functions that it was previously outside, which changes function parameter types in a bunch of places.

The only slightly confusing piece (beyond understanding why the bug happened in the first place) is in ComputeBlockAvailSpace and its call to ComputeReplacedBlockOffsetsForFloats.  The other callers of ComputeReplacedBlockOffsetsForFLoats are inside WidthToClearPastFloats, so it clearly can't call the function that calls it.  And it only needs the result of WidthToClearPastFloats in the outer table frame case, so we just call it in that one case, at the one caller that doesn't already (thanks to these changes) have a ReplacedElementWidthToClear.

I'm still waiting for a full reftest run to finish; I'll request reviews if that turns out clean.
This seems to have broken the reftests for bug 379349.
Except those reftests seem to fail for me in the exact same way in today's nightly build.
Comment on attachment 319098 [details] [diff] [review]
patch #2

I think this patch is good; those 5 reftests started failing on my machine (being displayed at significantly different font size than before) between the 2008-04-28-04-trunk and 2008-04-29-04-trunk nightlies.  Still need to figure out what's going on there...
Attachment #319098 - Flags: superreview?(roc)
Attachment #319098 - Flags: review?(roc)
(In reply to comment #30)
> those 5 reftests started failing on my machine
> (being displayed at significantly different font size than before) between the
> 2008-04-28-04-trunk and 2008-04-29-04-trunk nightlies.  Still need to figure
> out what's going on there...

bonsai link for that range:  http://tinyurl.com/4ejl4z

Maybe bug 384090's checkin?  That one's checkin comment indicates it's related to font-size calculations on GTK. ("[GTK+] incorrect logical resolution for converting font sizes in pt, etc. Get the actual resolution used by GTK+/X instead of a best-guess based on screen dimensions")

I filed the reftest failures as bug 431956, which isn't new; the reason I started seeing them is the landing of bug 384090.
Attachment #319098 - Flags: superreview?(roc)
Attachment #319098 - Flags: superreview+
Attachment #319098 - Flags: review?(roc)
Attachment #319098 - Flags: review+
Comment on attachment 319098 [details] [diff] [review]
patch #2

So this one is slightly more involved than the previous patch in this bug, but not as complicated as it looks.  It's also a regression from a recent change, and I think we should take it.
Attachment #319098 - Flags: approval1.9?
Attachment #319098 - Flags: approval1.9? → approval1.9+
(In reply to comment #33)
> (From update of attachment 319098 [details] [diff] [review])
> So this one is slightly more involved than the previous patch in this bug, but
> not as complicated as it looks.  It's also a regression from a recent change,
> and I think we should take it.
> 

Are we reopening the other bug once this lands, or is that now Bug 431956?
Blocking as per the magic word "regression" in comment 33
Flags: blocking1.9? → blocking1.9+
Keywords: testcase, top100
Whiteboard: [has patch][has review][has approval]
Checked in to trunk.

(In reply to comment #34)
> Are we reopening the other bug once this lands, or is that now Bug 431956?

Nothing needs to be reopened; I'm not sure why you think anything does.  (Just because this bug was caused by the patch to bug 427129 doesn't mean that fixing this bug means that bug is no longer fixed.  This bug was caused by a mistake in that patch, but that bug is still fixed.)
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
I think there had just been some ambiguity in your statement from Comment #33 that said "It's also a regression from a recent change"

I thought you meant that it was a regression caused by the entirety of that recent change, and that you were reversing the fix, not that you were just fixing a part of it that caused this.  But no problems now.  Thanks for you work on this!
Both of dbaron's checkins included reftests, so marking in-testsuite+.
Flags: in-testsuite+
verified fixed using  Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050506 Minefield/3.0pre as well as Mac Leopard nightly. Verified using the URL and the test cases in the bug.
Status: RESOLVED → VERIFIED
I am seeing something related to this on the following page if you go down to the See Also section: http://en.wikipedia.org/wiki/Neuropsychology

If you want to report a separate bug, please file a separate bug.  This bug is fixed.
(In reply to comment #41)
> If you want to report a separate bug, please file a separate bug.  This bug is
> fixed.
> 

Thanks for your bluntness.  I went back and checked it in other browsers, and it displayed similarly, so it looks like I did jump the gun.  My fault.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: