252920 - The text shifts a bit downwards when focusing the link

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Martijn Wargers (dead)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a3) Gecko/20040722 Firefox/0.9.1+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a3) Gecko/20040722 Firefox/0.9.1+

It happens with a lot of the links to the subfora at that url, but I'll attach a
testcase here.
The problem is that the text and the link shifts 1px downwards when focussing
the link. 
It happens with a specific combination of tags and style. See testcase.

It doesn't happen in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a3) Gecko/20040716
But it happens in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a3) Gecko/20040717

Maybe this is a regression of the fixing of bug 151375?



Reproducible: Always
Steps to Reproduce:
1. See testcase and focus link
2.
3.

Actual Results:  
No shift of text and link

Expected Results:  
Little shift of text and link

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

Comment 2

[Martijn Wargers (dead)]

Here, at this url focusing the link 'David Baron explains in a post to
netscape.mozilla.public.seamonkey' causes the table to grow a bit to the right.
http://www.mozillazine.org/talkback.html?article=3405

Comment 3

[Aaron Leventhal]

Oh joy! Another regression from our outline work.

Comment 4

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

Most likely a table incremental reflow bug.

Comment 5

[Martijn Wargers (dead)]

On this site appears a horizontal scrollbar when you focus the 'help', 'bookmark
this page' or 'demo' links, at the most right of the page:
http://www.ukpersonal.hsbc.co.uk/public/ukpersonal/internet_banking/en/logon.jhtml
It disappears again when it is unfocused.

Comment 6

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

I plan to work on this in a week or two. If I forget, someone please ping me.
And someone please make sure that we don't release 1.8a3 or 1.8beta with this!

Comment 7

[Boris Zbarsky [:bzbarsky]]

Setting blocking request flag per comment 6

Comment 8

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

Attachment: testcase2

OK, I didn't get to it for 1.8a3 but here's a testcase that shows this is
definitely a generic table incremental reflow bug. The table, the
vertical-align:middle, the space after the <br>, and the empty span at the end
of the line are all necessary to see this bug!

Comment 9

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

Actually I wonder if even the stable rendering of that testcase is at all correct.

Comment 10

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

I think the text should be at the top of the cell, with a blank line under it,
not the middle. Anyone else have an opinion?

Comment 11

[Bernd]

Attachment: reflow log

I doubt that it is table reflow bug but rather a block reflow issue, especially
I don't understand why the inner block produces all over sudden an overflow
area.
Don't we still have that br-1twips hack in block code ?

Comment 12

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

> Don't we still have that br-1twips hack in block code ?

er, what hack is that?

Comment 13

[Bernd]

btw: the default for the table cell is valign:middle and this is also the
computed style shown in inspector

Comment 14

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

Ah yes, so the stable rendering is correct since the following line is
suppressed and the lone line is centered in the cell.

Comment 15

[Bernd]

1- twips hack  is that code still used ?
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsBRFrame.cpp#189

Comment 16

[Bernd]

Attachment: frames before hover

Comment 17

[Bernd]

Attachment: after hover

Comment 18

[Martijn Wargers (dead)]

Forgive me if this is a stupid question, but I thought that focusing itself
causing a reflow is a bug. But that's not a bug?

I've seen this shifting-when-focusing stuff happening at a few other places.
Should I report them all?
One of such a case I still can remember is the xul error page. The 'Try again'
link shifts down when focused (probably indeed into the right position). I can
attach a testcase, if you like.

Comment 19

[Bernd]

the testcase has a  td:hover { letter-spacing:0.001px; } and it should cause a
style change reflow on the table cell on hover, which it does.
see 
 cell 0234FE38 r=1 incr. (Style) a=2808,UC c=2760,600 cnt=898 

what fascinates me more is already the initial reflow:
block 0234FE98 r=0 a=UC,UC c=UC,UC cnt=870 
...
block 0234FE98 d=2760,240 me=636 o=(0,0) 2760 x 282
how a block during an unconstrained reflow can generate an overflow area is strange

Comment 20

[Mats Palmgren (inactive)]

Minimal table-free testcases can be found at bug 255584

Comment 21

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

Bug 255584 turned out not to be related to this.

Comment 22

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

Attachment: fix

[Sorry for the small context in this diff, I had to separate it from my other
nsBlockFrame patches...]

There are two bugs here. The issue that Bernd noticed with the overflow area
was a bug in ComputeCombinedArea, which unions rects by hand and doesn't
properly ignore lines whose combined areas are empty. So I changed it to use
nsRects and that problem is fixed.

The other bug, which is the real bug here, is that when ReflowDirtyLines skips
a not-dirty line, it updates aState.mY (the current vertical position) to the
line's mBounds.YMost() even if the line is empty. In this case, had the line
been dirty, PlaceLine() would *not* have advanced the mY. So we need to change
the optimized path to match the normal path and only advance mY for non-empty
lines. Note that normally setting mY in the ReflowDirtyLines loop doesn't have
much effect because deltaY is used to control the placement of lines and mY is
ignored or reset. But mY is eventually used to compute the block height (and
according to the comments, also in float damage computation) so we'd better set
it right.

Comment 23

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

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

These seem simple, but my understanding of margins is limited and hey, block
reflow, you never know what could happen :-)

Comment 24

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

Isn't what PlaceLine does more complicated than this?

Comment 25

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

This is what PlaceLine does:

  else { // aLine->IsEmpty()
    // Don't let the previous-bottom-margin value affect the newY
    // coordinate (it was applied in ReflowInlineFrames speculatively)
    // since the line is empty.
    // We already called |ShouldApplyTopMargin|, and if we applied it
    // then BRS_APPLYTOPMARGIN is set.
    nscoord dy = aState.GetFlag(BRS_APPLYTOPMARGIN)
                   ? -aState.mPrevBottomMargin.get() : 0;
    newY = aState.mY + dy;
    aLine->SlideBy(dy); // XXXldb Do we really want to do this?
    // keep our ascent in sync
    // XXXldb If it's empty, shouldn't the next line control the ascent?
    if (mLines.front() == aLine) {
      mAscent += dy;
    }
  }

My understanding of this code is weak, I admit, but I think non-zero dy is just
undoing something that was done speculatively in ReflowInlineFrames, and
therefore there is no need to do it again when the line has not been reflowed.

PlaceLine does do an extra thing before setting mY = newY; it checks if the line
fits in the available height. But ReflowDirtyLines just doesn't try to handle
that right now when it slides lines, and this is not the right occasion to
correct that. (I actually have a fix for this as part of my columns work.)

Comment 26

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

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

OK, I reread nsBlockFrame::ReflowDirtyLines and I agree that the last change
does make sense.

Comment 27

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

Checked in.

Comment 28

[Jesse Ruderman]

I added a reftest:
http://hg.mozilla.org/mozilla-central/rev/dc4e22e87f6c

Firefox-2004-07-22-09-trunk fails if ".outline" is changed to the older ".MozOutline".