Bug 20443 – Certain parts of link are not clickable when there is frameset

• Component: Frames
• Status: RESOLVED FIXED

[Eric Roman]

There is glitch with links not being clickable when there is a nearby frame divider.

The links are not clickable precisely at the y coordinate where the adjacent frame divider is.

please see the forthcoming repro case for better explanation.

[Eric Roman]

Created attachment 22873 [details]
Load index.html, then follow the instructions in bottom-right pane.

[Brad Garcia]

I've found the problem & I have a fix.  I need to come up with a test to include with the patch.  Can somebody recommend a way to create a test for this?

If I included something like the first attachment in this bug report, would that actually generate expected output to show that it's now clickable?  If not, is there some other way to write a test for this?

[Eric Roman]

How about a text dump layout test, which prints SUCCESS when the link is clicked.

Something along these lines:



<style>
  a {
    /* position it such that it has dead spot at (X,Y) */
  }
</style>

<a href="#" onclick="return onLinkClick()">LINK TO CLICK</a>

<script>
    if (window.layoutTestController) {
        window.dumpAsText();

        // Click in a dead position
        window.eventSender.mouseMoveTo(X, Y);
        window.eventSender.mouseDown();
        window.eventSender.mouseUp();
    }

    function onLinkClicked() {
      window.console.logln("SUCCESS -- the link was clicked");
      return false; // Prevent default.
    }
</script>

[Brad Garcia]

Created attachment 23030 [details]
Fix for bug.  Not yet ready for submitting.

Here is the fix.

The problem was that the interior frameset was saying that the mouse was over one of the borders, and therefore resizable (because the Y-position matched, and we didn't check the X-position because of the use of OR instead of AND).  Then we wouldn't bother checking the left frame, which is where the mouse was actually located.

I could still use some help developing a test for this, if someone doesn't mind holding my hand.  The window.eventSender stuff isn't working.

[Eric Roman]

Created attachment 23039 [details]
Layout test

Thanks Brad, the patch looks good!

I can help you with the layout test . Attached is a modified version which works for me (tested on mac os x, not windows yet)

When you are ready for review, upload your patch and set the review:? flag so that a webkit committer can take a look.

[Brad Garcia]

Created attachment 23053 [details]
Final fix for bug.

Thanks for the help, Eric!

[Eric Seidel (no email)]

Comment on attachment 23053 [details]
Final fix for bug.

Won't this break normal resizing? With this patch you can only resize frames when you click on the intersection of their split regions no?

I would be more confident of this change if you had a test which tested expected frame set adjustment behavior.  Where you clicked on the various parts of the adjuster and tried to adjust the frames and made sure they changed size.

Leaving r=? since maybe this does actually work, but it looks to me like this breaks normal resizing when you don't click on the intersection of the resize bars.

[Eric Roman]

(I thought the same thing initially, but after patching into my build did not observe this limitation)

[Eric Roman]

> +    function onLinkClick() {

WebKit style says curly braces should be on following line for functions. As in:

function onLinkClick()
{

[Dave Hyatt]

Comment on attachment 23053 [details]
Final fix for bug.

This patch isn't right.  canResize is fine.  The issue is higher up (in nodeAtPoint).

[Dave Hyatt]

I think this check is the troublesome one:

bool inside = RenderContainer::nodeAtPoint(request, result, x, y, tx, ty, action)
        || m_isResizing || canResize(IntPoint(x, y));

My memory of this code is hazy (I'm the one who rewrote it most recently I guess), but I think RenderContainer::nodeAtPoint needs to be true in order for you to check canResize.

[Dave Hyatt]

Make a 2-dimensional frameset (with both rows and columns) and you'll see why your patch is  wrong.  You will only return true from canResize if you're over joins of vertical and horizontal splitters.

[Dave Hyatt]

I actually don't even remember why this canResize check is present in nodeAtPoint.

[Dave Hyatt]

I'd suggest just yanking the canResize check completely from nodeAtPoint and see if anything breaks. :)

[Brad Garcia]

(In reply to comment #12)
> Make a 2-dimensional frameset (with both rows and columns) and you'll see why
> your patch is  wrong.  You will only return true from canResize if you're over
> joins of vertical and horizontal splitters.

I've done just this, and I'm still able to resize at any point along the splitters.  Strange.  Have you tried it with this patch?

But I agree that this fix is incorrect.  Your explanation that canResize should only be called when nodeAtPoint is true made the code clearer to me.  I'll test simply removing the call to canResize.

[Brad Garcia]

Created attachment 23069 [details]
Fix for bug

Removing the call to canResize appears to solve the problem with no other anomalies noticed.

[Eric Seidel (no email)]

Comment on attachment 23069 [details]
Fix for bug

It would still be great to have additional tests which test for clicking on the resize area and dragging to resize the frameset.  Although perhaps that's outside the scope of this bug.

[Darin Adler]

Comment on attachment 23069 [details]
Fix for bug

If the correct thing to do here is to remove the canResize function call, then the implementation of canResize also needs to be removed altogether. We don't want to leave dead code around.

The ChangeLog doesn't match the patch. The log says the function being changed is RenderFrameSet::canResize, but the patch is a change to RenderFrameSet::nodeAtPoint.

[Brad Garcia]

Created attachment 23855 [details]
Updated patch incorporating feedback

[Eric Seidel (no email)]

Comment on attachment 23855 [details]
Updated patch incorporating feedback

I think the test could have been better.  With both positive and negative tests made.  (sythesized clicks which should both fail and succeed, on each side of the resize bar).  But the change itself looks fine, at least it matches what Darin and Hyatt asked of you.  Sorry this took over a month to get through the review queue :(

[Brent Fulgham]

Committed in revision 40605.