437183 - [RTL] [QUIRKS] Firefox3 breaks rendering of image-in-a-table toolbars

Status: NEW

(Core :: Layout, defect)

Description

[Yehuda]

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

When using quirks mode and dir=rtl, Firefox 3 treats white spaces between A and IMG tags differently then Firefox 2. While Firefox 2 ignored these spaces in rtl, Firefox 3 RC1 adds a small margin above the image. This situation is common when a toolbar is made of images, and the page author uses some images as spacers, and some images as links.

Reproducible: Always

Steps to Reproduce:
1. Using Firefox2, navigate to http://www.mapa.co.il/general/searchresult_locked.asp
2. Observe the toolbar on the top left hand of the map
3. Using Firefox3, navigate to the same address
Actual Results:  
Toolbar looks broken in Firefox 3

Expected Results:  
Toolbar should look the same as Firefox 2

See also attached simple case for reproducing this bug.

Also note the following:
All other major browsers (Safari 2 and 3, IE6 and IE7) render the example correctly in quirks mode and stand mode. This is also true for Firefox 2 and Firefox 3 in standard compliant mode (by adding DOCTYPE as the first line of mapa.html)

Comment 1

[Yehuda]

Attachment: small testcase for showing the error

Comment 2

[Yehuda]

Attachment: Example in standard mode as displayed in Fx2

Comment 3

[Yehuda]

Attachment: Example in quirks mode as displayed in Fx2

Comment 4

[Yehuda]

Attachment: Example in standard mode as displayed in Fx3

Comment 5

[Yehuda]

Attachment: Example in quirks mode as displayed in Fx3

Comment 6

[Yehuda]

Attachment: Example in quirks mode as displayed in IE6

Comment 7

[Yehuda]

Attachment: Example in quirks mode as displayed in IE7

Comment 8

[Yehuda]

Attachment: Image of original web site with Fx3

Comment 9

[Yehuda]

Attachment: Image of original web site with Fx2

Comment 10

[Yehuda]

Chat transcript that may provide some more clues:
<yehuda> The difference is the following:
<yehuda> First td: only image - spaces are ignored
<yehuda> Second td: A and IMG tags without spaces - works again
<Uri> Is this really rtl-related?
<yehuda> Third TD - Spaces between A tag and IMG tag - extra margin is added in FF3
<yehuda> Uri: yes. If "dir=rtl" is removed then FF2 and FF3 looks the same
<Uri> that is, the images don't line up in Fx 2 either?
<Uri> Because they don't seem to line up in FX3
<yehuda> without dir=rtl - yes
<yehuda> it looks like a bug in FF2, but a bug that at least some Israeli sites depend on
<Uri> So Fx 3 makes the rtl and ltr behavior identical, whgereas in Fx 2they differed?
<yehuda> right
<Uri> Yeah, that's what I was about to say
<Uri> Is this a standards vs. quirks mode thing?
<smontagu> do quirks mode/standard mode make a diffence?
<yehuda> once again - yes
<yehuda> with modern DOCTYPE, FF2 and FF3 are the same again
<yehuda> but then again, Israeli sites are almost always in quirks mode
<Uri> So Fx 2 behaved, for RTL, as if it were in standards mode, even when it wasn't?
<smontagu> or to put it another way, the quirk only exlsts in LTR
<yehuda> yes - at least with regard to whitespace
<Uri> So, how come this works in IE? (I assume it does)
<yehuda> because IE's quirks mode is different then Firefox
<Uri> (just wondering, erally - you don't have to answer :-)
<Uri> it's *more* standard?
<Uri> I wouldn't expect that
<Uri> (even in this tiny case)
<yehuda> we have to test the example in IE. - I didn't do that
* Uri is too lazy to pull out his Windows laptop right now
<yehuda> I only know that the orignal site looks good in IE - but it could be because of some other markup
<Uri> right
<Uri> I'll try to find out the "regression" range and see what "broke" it
<Uri> but in the end, I'm pretty convinced that this is really a correct bug fix
<Uri> and that the sites will have to adjust
<Uri> BTW, are you aware of other sites this is breaking?
<Uri> (besides Mapa)?
<yehuda> no
<Uri> So it doesn't seem like a clear case of "breaking the web"
<yehuda> I'm testing on Safari 2.0 on Mac, and it also behaves like Fx2
<yehuda> will try safari 3...
<Uri> hmm
<yehuda> bad news: safari 3 also displays all images aligned
<yehuda> (that is, like safari2 and Fx2)
<Uri> but also in LTR, right?
<yehuda> just a sec...
<yehuda> yes
<yehuda> so we have Safari3 and Fx3 disagree on quirks mode, RTL or LTR
<Uri> So either this should be "fixed" for both directions, or left as-is
<Uri> probably because Safari has different quirks
<yehuda> let me check standard mode for a sec.
<Uri> (I'm searching for a regression range)
<yehuda> for the record: Standard mode: Fx3 and Safari3 RTL and LTR all images aligned
<Uri> sometime between 2007-10-13 and 2007-10-24
<Uri> So Safari (and IE??) probably doesn't have a quirk that Firefox has
<Uri> which is kind of strange...
<yehuda> yes, but only in RTL (I think - too many test cases...)
<Uri> no, the issue was that the quirk was *missing* from RTL in Fx2
<Uri> (according to the data points you gave me)
<Uri> ]ok, I guess that's what you were saying
<Uri> so forget the "no" :-)
<yehuda> OK
<yehuda> I'll run one more test to make sure
<Uri> OK. It might be useful to track down the list of quirks and find out which quirk exactly this is
<Uri> and the verify that IE/Safari really don't have it
<Uri> and if that's the case, it should die.
<Uri> I'm downloading builds from 10/2007
<Uri> yehuda: I reduced the regression range to 2007-10-19 - 2007-10-20, but nothing there seems RTL-related
<smontagu> nothing much there seems table-related either
<smontagu> except bug 244135, which doesn't look like this
<Uri> Bug 244135 ??
<Uri> (it's somewhat table-related, at least)
<firebot> Uri: Bug https://bugzilla.mozilla.org/show_bug.cgi?id=244135 nor, --, ---, [e-mail], RESO FIXED, table borders disappear occasionally with border-collapse and rowspan
<Uri> hmm... there isn;t a date/time/place yet, is there?
<Uri> smontagu: Bug 393096 is whitespace-related... :-/
<firebot> Uri: Bug https://bugzilla.mozilla.org/show_bug.cgi?id=393096 nor, --, mozilla1.9beta1, [e-mail], RESO FIXED, page renders too wide since reflow branch
<smontagu> Uri: I don't see that within the regression range
<Uri> smontagu: you don't see what in the regression range? bug 393096?
<Uri> http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=%2Fmozilla%2Flayout&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-19+03%3A00&maxdate=2007-10-20+05%3A00&cvsroot=%2Fcvsroot
<smontagu> ah, I didn't put in hours
<Uri> (that's only /layout)
<Uri> the builds are from 04 O'clock

Comment 11

[Tomer Cohen :tomer]

I can confirm that behavior (Firefox3rc1).

Comment 12

[Bernd]

I would bet from the regression range that this is block reflow issue.

Comment 13

[Uri Bernstein]

After some more investigation, I tend to think that this is a bug with applying a quirk, that from whatever reason didn't affect RTL up to the regression date given above.

The quirk is the the one used to deal with the issue described in http://developer.mozilla.org/en/docs/Images%2C_Tables%2C_and_Mysterious_Gaps.

I couldn't find a description of the quirk anywhere but the way it could be roughly described, I think, is:
"In a table cell (or any block?) that does not contain any non-whitespace text, do not leave vertical space below the baseline".

The problem is that if some of the whitespace in the cell is contained within an inline element (such as <a>), the quirk isn't activated.

I'll attach a testcase immediately.

Comment 14

[Uri Bernstein]

Attachment: testcase

The two middle cells should have the quirk applied to them, and therefore should look just like the first (leftmost) cell. Instead, the images in them are vertically aligned like in the fourth cell, where the quirk is not applied.

Comment 15

[Boris Zbarsky [:bzbarsky]]

That last testcase is all LTR...  And behaves the same way in Gecko 1.8 and Gecko 1.9.  So I can't imagine that it accurately represents this bug (though it may show some related issue, of course).

Comment 16

[Uri Bernstein]

Attachment: patch??

At the point where nsLineLayout::VerticalAlignFrames() is reached, the whitespace text element is apparently not yet collapsed (pfd->mBounds.width is non-zero). The flags, however, seem to be set correctly. So this fix just ignores the bounds and looks only at the flags.

However, there must have been a good reason for testing the bounds, so it's very possible that this is not the correct fix.

Comment 17

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

Duplicate of bug 434174?

Comment 18

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

I think the patch is wrong because it would cause us to incorrectly ignore the metrics for a space between two images.  Things like:

<td><img> <img></td>

are laid out with the font metrics for the space reliably in browsers going way back.

Comment 19

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

(And see bug 434174 for more discussion; it took me at least 5 minutes to find that bug... so my comment 17 actually had a midair collision notification with both comment 15 and comment 16.)

Comment 20

[Uri Bernstein]

(In reply to comment #15)
> That last testcase is all LTR...  And behaves the same way in Gecko 1.8 and
> Gecko 1.9.  So I can't imagine that it accurately represents this bug (though
> it may show some related issue, of course).
> 

In comment #13, I said that I don't believe this is really an RTL bug, but rather a general bug that RTL just happened to be spared from until recently.

I'm reading through the other comments (and the bug David points to) now.

Comment 21

[Uri Bernstein]

Comment on attachment 323768 [details] [diff] [review]
patch??

You're right, this patch activates the quirk in the "<img> <img>" case, which is probably incorrect.

Comment 22

[Uri Bernstein]

This of course has the same root cause as bug 434174 (which could probably use a better title and perhaps a better testcase).

The root bug was apparently obscured in certain cases (such as RTL and whatever the case is in that bug), and was uncovered (for both cases) by the fix for bug 393096.

My testcase here shows a case of the root bug that was never obscured.