Bug 45164 – REGRESSION: <a><img align=top></a> Clickable area too large

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

Created attachment 66473 [details]
Testcase

When using a image with align="top" as a link the clickable area may expand above the image, overlapping other links.

Only Safari 5 is affected, Safari 4 work as expected. You can acctually see in the Developer Tools that the <a> is highlighted above the <img>.

[Alexey Proskuryakov]

Broken hit testing == not good.

[My Shin]

Created attachment 76315 [details]
proposed patch

I couldn't make test case because it needs hitTest to verify this issue.
And I modified expected files of some test cases on Mac platform to change box position's calculation.
I have no idea about expected files on other platform. I don't have environment about other platform.

please take look.

[Simon Fraser (smfr)]

You can test hit testing with document.elementFromPoint(), or with event sender in DRT.

[My Shin]

Thank you.
I'll try to make test case.

[My Shin]

Created attachment 76489 [details]
proposed patch(2nd)

I added test case.
please review again.

[Alexey Proskuryakov]

<rdar://problem/8895939>

[Dave Hyatt]

Comment on attachment 76489 [details]
proposed patch(2nd)

View in context: https://bugs.webkit.org/attachment.cgi?id=76489&action=review

This isn't right.

> WebCore/rendering/InlineFlowBox.cpp:597
> +            int posAdjust = 0;
>              if ((isInlineFlow && !static_cast<InlineFlowBox*>(curr)->hasTextChildren()) && !curr->boxModelObject()->hasInlineDirectionBordersOrPadding() && !strictMode)
>                  childAffectsTopBottomPos = false;
> -            int posAdjust = maxAscent - curr->baselinePosition(baselineType);
> +            else
> +                posAdjust = maxAscent - curr->baselinePosition(baselineType);

I don't understand this at all.  There's nothing wrong with this code as far as I can tell.  You still have to set the correct logicalTop.  You can't just leave it at 0.

[Dave Hyatt]

The actual bug here is in:

adjustMaxAscentAndDescent

which also needs the notion of setMaxAscent and setMaxDescent (which was added to computeLogicalBoxHeights recently).  If those aren't set, then adjustMaxAscentAndDescent needs to make sure both end up set instead of only adjusting one of the two values.

[Dave Hyatt]

This is actually even more complicated.

If there is room, you probably want all of the inlines to fit within the space provided by the image.  The correct layout would be that adjustMaxAscentAndDescent sets maxAscent to the maxAscent of all the boxes that didn't really affect the line layout instead of just giving all the space to maxDescent.

The same goes for vertical-align:bottom and max descent.

So I guess in addition to tracking whether or not maxAscent and maxDescent have been set, you also have to track the maxAscent and maxDescent including the additional boxes.  Yay for two more member variables.

[Dave Hyatt]

(In reply to comment #9)
>  Yay for two more member variables.

Method parameters. :)

[Dave Hyatt]

By the way another observed bug is that although we properly clip the painting when a box extends above lineTop, we didn't stop hit testing.  We really should fix that as well, so you could punt on the harder line layout issue and just patch hit testing to match painting.

[Dave Hyatt]

Created attachment 85713 [details]
Patch to clamp hit testing

[Dave Hyatt]

Fixed in r81055.

[Dave Hyatt]

Just closing this out, since I don't think the mis-positioning is interesting enough to bother fixing.  No need to make the code more complicated when the positions of those boxes don't really matter.