253572 - [EVENTTARG]Block links not block when next to float in a float layout

Status: RESOLVED FIXED

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

Description

[Alastair Campbell]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040707 Firefox/0.9.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040707 Firefox/0.9.2

The URL shows a cut-down version of a layout template in use at
http://nomensa.com, and uses links in a list that have been given display: block.

When next to the content section, these links are not block, i.e. they have a
small hit-area of just the text.

Reproducible: Always
Steps to Reproduce:
1. Go to the page and view.

Actual Results:  
The links next to the content are not block, the links below the content
(usually none except in this text case) display the expected behavior.

Expected Results:  
Displayed all the links in the navigation as block-elements.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Interesting.  Somehow the "wrap2" div (the nested float) is ending up _above_
the floated menu (at least for GetFrameForPoint() purposes).   That's wrong....

The problem seems to be in nsBlockFrame::GetFrameForPoint in the
NS_FRAME_PAINT_LAYER_FLOATS case.  We're clicking the background of a
block-level child of a floated child.  So the FOREGROUND search in the floatList
comes up empty.  Then we do a FLOATS search in the floatList, which recurses
into checking floats, including their backgrounds.  So the FLOATS search in the
floatList finds the _background_ of the float-within-a-float.  Then we never do
a BACKGROUND search in the floatList, since we found something....

Isn't the right thing to do here to do the FOREGROUND/FLOATS/BACKGROUND checks
on each item in the floatList before moving on to the next one?

Comment 2

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

Yes.  The easy way to describe it is that GetFrameForPoint should do exactly the
reverse of what Paint does.

Comment 3

[Alastair Campbell]

You make it sound like a fairly easy to understand thing. (As someone who knows
vitually nothing about coding a browser, or 'painting' ;) 

Does that mean it is easy to fix, or are there a whole truck-load of other
things to consider?

(NB: I do have a vested interest in seeing it work, although I think others will
as well, due to an Alistapart article on this type of layout.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 155389 [details] [diff] [review]
Patch

This creates a new paint layer (ALL) and makes GetFrameForPoint know about it
and deal.

The changes to the nsFramePaintLayer enum are not strictly needed, but it made
sense to me to make the whole thing a bitfield given the introduction of
NS_FRAME_PAINT_LAYER_ALL.

Comment 6

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

In the nsBlockFrame.cpp changes, you could have used the third of the three
statements you removed as a basis (the one beginning with |return| rather than
|rv =| and removed the additional

if (NS_OK == rv) {
  return NS_OK;
}

Comment 7

[Boris Zbarsky [:bzbarsky]]

Checked in (and the change David mentions in comment 6 also checked in) for 1.8a3.

Comment 8

[jag (Peter Annema)]

Wow, that's some code... break after return, else after return... Maybe next
time someone touches that :-)

Comment 9

[Alastair Campbell]

Attachment: The original test case for the non-block behaviour.

The test case will disappear from the server, so attaching this original page
for future checks.

Comment 10

[Alastair Campbell]

This bug seems to be back in the preview release, is that because it is using an
older build or because the fix has been undone?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Firefox PR1 is using the 1.7 branch, which branched off the trunk in April;
about 4 months before this bug was fixed.