Bug 14094 - REGRESSION: Issues with scrollbars in phpbb2 based forums
Summary: REGRESSION: Issues with scrollbars in phpbb2 based forums
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL: http://forum.ytunnelpro.com/viewtopic...
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-06-12 09:36 PDT by Adam Williams
Modified: 2011-11-26 11:50 PST (History)
5 users (show)

See Also:


Attachments
Reduction (415 bytes, text/html)
2007-09-03 00:50 PDT, mitz
no flags Details
Screen Captures (317.01 KB, application/zip)
2007-11-23 05:02 PST, Adam Williams
no flags Details
Layout test for this problem (1.62 KB, text/html)
2010-10-16 04:00 PDT, Benjamin Poulain
no flags Details
Other interesting use case of the same problem (1.89 KB, text/html)
2010-10-18 10:26 PDT, Benjamin Poulain
no flags Details
First attempt to solve the issue (1.89 KB, patch)
2010-10-18 11:19 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
It's better with the patch :) (5.76 KB, patch)
2010-10-18 11:22 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch: define the lowest position as the position of the block (5.25 KB, patch)
2010-10-20 09:00 PDT, Benjamin Poulain
hyatt: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Williams 2007-06-12 09:36:41 PDT
in "Safari 3.0":
phpbb2 forums have scroll bars in about 50% of posts for no real reason, there
is no content to scroll but for some reason scroll bars are appearing in these
areas.
example:
http://forum.ytunnelpro.com/viewtopic.php?t=745

Unsure if the phpbb2 problem was new safari or webkit so it was also filed with
bugreports.apple.com under:
"5263028         Scroll bars appearing where they are not needed."
Comment 1 David Kilzer (:ddkilzer) 2007-06-12 10:02:47 PDT
Confirmed with a local debug build of WebKit r22089 with shipping Safari 2.0.4 (419.3) on Mac OS X 10.4.9 (8P135).

This is also a regression from shipping Safari.

Comment 2 David Kilzer (:ddkilzer) 2007-06-12 10:08:26 PDT
If I would only read closer:

<rdar://problem/5263028>

Comment 3 Adam Williams 2007-06-24 20:04:55 PDT
Just thought I'd add note the problem still persists with webkit 522.12 and Safari 3.0.2 update. Although I figured it would but just covering all bases :)
Comment 4 David Kilzer (:ddkilzer) 2007-09-02 21:37:16 PDT
Created test account (waiting to be approved):
E: webkit-test@hotmail.com
P: w3bk1t

Comment 5 David Kilzer (:ddkilzer) 2007-09-02 21:51:39 PDT
(In reply to comment #4)
> Created test account (waiting to be approved):
> E: webkit-test@hotmail.com
> P: w3bk1t

U: webkit
Comment 6 mitz 2007-09-03 00:50:43 PDT
Created attachment 16184 [details]
Reduction

This reduction shows that the scrollbar appears because of small line-height. Firefox renders a scrollbar, too, in this case, but Opera does not.

The reason this is a regression for the live site, I suspect, is that the overflow rule is inside a media query, which Safari 2 doesn't support. The media query used, "@media all and (min-width: 0px)", is a catch-all, so I think it is just a lame Opera detector (see bug 13980).
Comment 7 David Kilzer (:ddkilzer) 2007-09-03 11:03:02 PDT
Verified that removing this CSS from http://forum.ytunnelpro.com/templates/ca_aphrodite/style.css removes the unwanted scrollbars on the site:

@media all and (min-width: 0px) {
        html>body .post-text {
                overflow: auto;
        }
}       
Comment 8 David Kilzer (:ddkilzer) 2007-09-03 11:07:18 PDT
Also confirmed that Safari 2.0.4 (419.3) with original WebKit on Mac OS X 10.4.10 (8R218) behaves the same way if the CSS is present without being enclosed by a media query, so the behavior in the reduction isn't really a regression.

The only regression is the way the web site behaves because WebKit now understands @media queries.

Comment 9 Adam Williams 2007-11-23 05:02:36 PST
Created attachment 17457 [details]
Screen Captures

Screen captures of site behavior in Safari 3.0.4 with correct site behavior and same identical site with same user/password and url on webkit nightly r27953 showing site non functional
Comment 10 Adam Williams 2007-11-23 05:04:05 PST
Comment on attachment 17457 [details]
Screen Captures

oops my bad. I was trying to upload this to an entirely different bug report. Completely Ignore this file.
Comment 11 Benjamin Poulain 2010-10-16 04:00:27 PDT
Created attachment 70949 [details]
Layout test for this problem
Comment 12 Benjamin Poulain 2010-10-18 10:26:27 PDT
Created attachment 71054 [details]
Other interesting use case of the same problem

This could be added as a second layout test. It shows the layout of inline flow + floating boxes.
Comment 13 Benjamin Poulain 2010-10-18 11:19:18 PDT
Created attachment 71060 [details]
First attempt to solve the issue

I am not very familiar with the layout of inline flow so input on the patch would be appreciated :)

One of my concern was the layout of floating elements inside the block after text with line-height < font-size. Our current layout is consistent with Internet Explorer so I did not pushed my change further than at the end of the layout of the inline flow.

This will break quite a few layout test since the size of block is bigger than before when line-height < font-size.
Comment 14 Benjamin Poulain 2010-10-18 11:22:17 PDT
Created attachment 71061 [details]
It's better with the patch :)
Comment 15 Benjamin Poulain 2010-10-18 11:41:03 PDT
Comment on attachment 71061 [details]
It's better with the patch :)

Removing from the review queue. This patch breaks the test css2.1/t0804-c5510-ipadn-00-b-ag.html and should not.
Comment 16 Benjamin Poulain 2010-10-20 09:00:45 PDT
Created attachment 71297 [details]
Patch: define the lowest position as the position of the block

I was trying to avoid this solution because I did not want the text to be clipped. However, not clipping the text in this case would not follow the CSS specification regarding the size of the block.

Internet explorer is also clipping the text at the bottom in this case.
Comment 17 Benjamin Poulain 2010-10-26 08:19:35 PDT
Anything I can do to help getting a review? You need more explanation about the patch?

With the patch, the following tests will need to be rebased:
mathml/presentation/fenced.xhtml
mathml/presentation/mo.xhtml
mathml/presentation/over.xhtml
mathml/presentation/row-alignment.xhtml
mathml/presentation/row.xhtml

I have checked the new results and they still look fine. The differences comes from layers being smaller with this change.
Comment 18 Kenneth Rohde Christiansen 2010-12-08 05:15:12 PST
Hyatt, could you have a look at this one?
Comment 19 Dave Hyatt 2010-12-08 09:12:15 PST
Yeah this issue occurred to me when I was working on the rewrite recently.  We need to be using the line-height box rather than the content box for lines when considering overflow.  Now that layout overflow and visual overflow are truly separated this should be pretty easy to fix.

All you really have to do here is change the default rectangle for layout overflow from the line box's x/y/width/height to a box that includes the line height.  (Remember to account for vertical text and expand/shrink the appropriate axis).

The lineTop/lineBottom clamping would also have to change to factor in line-height.
Comment 20 Dave Hyatt 2010-12-08 09:13:22 PST
I'm not completely convinced we should fix this though.  I like that the text is reachable.  I'd only consider fixing this if it's a major compatibility issue.
Comment 21 Dave Hyatt 2010-12-08 09:14:01 PST
Comment on attachment 71297 [details]
Patch: define the lowest position as the position of the block

Minusing since this code all got changed.
Comment 22 Benjamin Poulain 2010-12-08 09:31:56 PST
(In reply to comment #20)
> I'm not completely convinced we should fix this though.  I like that the text is reachable.  I'd only consider fixing this if it's a major compatibility issue.

I noticed this problem is quite common on code examples. For some reason web developers tend to set line-height < font-size there (e.g. http://developer.qt.nokia.com/forums/viewthread/1184/ ).

I remember seeing this on http://developer.apple.com/ some time ago. And that worked well with IE9. I cannot find it anymore so I guess the css have changed.

Sure you don't want a fix? We close the bug as wontfix?
Comment 23 Dave Hyatt 2010-12-08 10:28:47 PST
What bugs me about it is that when content spills out of a regular block, we make it reachable via overflow mechanisms.  I don't see why content that spills out of a line box should be any different.  There is actual content that the user might want to see.  Clipping the text certainly doesn't seem desirable.

Basically I'm inclined to say WONTFIX unless the compatibility problem is severe enough that it looks like we need to do it.  I'd be curious to know if we're the only browser engine doing this and if all the others clip the text.
Comment 24 Benjamin Poulain 2010-12-08 10:40:03 PST
(In reply to comment #23)
> What bugs me about it is that when content spills out of a regular block, we make it reachable via overflow mechanisms.  I don't see why content that spills out of a line box should be any different.  There is actual content that the user might want to see.  Clipping the text certainly doesn't seem desirable.

What is counterintuitive to me is that "overflow: visible" produce scrollbar in that case.

> Basically I'm inclined to say WONTFIX unless the compatibility problem is severe enough that it looks like we need to do it.  

Fine with me, I close the task. I guess I still have learned something on the way :)

> I'd be curious to know if we're the only browser engine doing this and if all the others clip the text.

Firefox and Opera behave as WebKit in this case. Internet Explorer clip the block.