434174 - trimmed trailing whitespace inside inline disables vertical alignment quirk, causing misalignment

Status: NEW

(Core :: Layout: Block and Inline, defect, P3)

Description

[Florin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9) Gecko/2008051206 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9) Gecko/2008051206 Firefox/3.0

After the webside loads the banner, the banner content goes 4 pixels up and the part that exits the screen apears under the banner as it would be sort of repeating itself. Tested www.e-safety.ro on other web browsers too and it looks ok, including mozilla firefox 2.0.And i've noticed the problem repeats on other sites that have flash banners too, i dont know yet if the version of flash matter.
If www.e-safety isn't loading, try another site that has flash banner, like http://www.electronica.pub.ro/Home.php.

Reproducible: Always

Steps to Reproduce:
1.open the website, wait for it to load
2.that's it, the flash banner will apear as i explained

Actual Results:  
-

Expected Results:  
-

-

Comment 1

[Ria Klaassen (not reading all bugmail)]

Confirmed on Windows XP. Regression window:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1192864680&maxdate=1192865699

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

It seems to be related to white-space in the <object> element.

Comment 3

[Jason Bassford]

No explanation for the resolution.  Either the specific bug / patch that fixed the browser has not been referenced, or this is really a Tech Evangelism bug.

Comment 4

[Martijn Wargers (dead)]

Floring, the site may have fixed the problem by adjusting their code, but this bug needs to remain open for now.
A decision needs to be made if the current behavior on trunk is considered what is wanted or not.

Comment 5

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

The <embed> and <object> are inline elements, so you'd expect the <embed> to be baseline-aligned with space below it for the descent of the current font. If you put Martijn's testcase in standards mode that's exactly what you get. I know that in quirks mode we have a specific quirk regarding replaced elements in table cells that eliminates the descent. I'm not sure why that quirk isn't kicking in for the testcase with whitespace, it certainly did in FF2.

Comment 6

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

The problem is this code:

    for (PerFrameData* pfd = psd->mFirstFrame; pfd; pfd = pfd->mNext) {
      if (pfd->GetFlag(PFD_ISTEXTFRAME) &&
          (pfd->GetFlag(PFD_ISNONWHITESPACETEXTFRAME) || preMode ||
           pfd->mBounds.width != 0)) {
        zeroEffectiveSpanBox = PR_FALSE;
        break;

In the first table of Martijn's testcase, the space after the <embed> is given nonzero width during its reflow. It is not and will never be collapsed with prior whitespace, since the <embed> is immediately before it. It will be *trimmed* because it ends up at the end of the line, but we have no way of knowing that here because we're calling VerticalAlignFrames after we've finished reflowing the span, before we've finished reflowing the line.

Therefore I can't see an easy way to fix this bug. We probably shouldn't vertically align spans as we go, but instead vertically align the entire line's frame subtree after reflowing the line.

Since this actually gives us standards mode behaviour in more cases, if this isn't breaking much real content then I think we could live with the change. But I'm not sure.

Comment 7

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

Is the regression because we changed when we do whitespace trimming?

Doing all of the vertical alignment at the end makes sense; just don't trip over my patch in bug 118501 (writing some good tests for that is on my list of things to do soon...).

Comment 8

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

I don't know off the top of my head why this would have worked before. I'd have to debug it in an old build.

It might be possible to restructures inline layout to put all vertical alignment at the end without much regression risk ... but it might not. It's not something I want to do right away.

Comment 9

[Uri Bernstein]

All/All, more details to the summary.

Bug 437183 has another (real-world) case of this that appeared following the fix for bug 393096, as well a a testcase (attachment 323749 [details]) demonstrating that the problem existed (in some situations) even before bug 393096 was fixed.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 4 duplicates.
:jfkthame, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 15

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.