[PATCH] various nodeAtPoint() fixes

[Germain Garand]

Hello there, hello Dirk !

I've dived since a week into nodeAtPoint's internals to solve a number of problems
 with mouse hovering and link detection.

 I finally found 3 errors and made a patch.

As the whole method is very tense and touchy, I think it's better if I details exactly 
what my changes do :

-------------------------------

- 1°: relative coordinates change don't propagate recursively to childrens.
(I posted this last week on kfm-devel but noone replied so it's still pending)

 if (!child->isPositioned() && child->nodeAtPoint(info, _x, _y, _tx+xPos(), _ty+yPos()))

Here, it should pass tx and ty, which are exactly the same as _tx+xPos()/_ty+yPos() excepted if a relative translation occured.

 Testcase: http://www.phoenix-library.org/bug-konq-css.html
 Bug: #42681 (+ #43282)

------------------------------------------------------

- 2°: on the same line, the test for !child->isPositioned() need to be !child->isSpecial()
(I do know it was changed in r1.171, but please read on.. there is more on this later)

rationale :
----------
All specialObjects (i.e : isFloating() || isPositioned() ) are parsed only in RenderFlow::nodeAtPoint().
It first iterates over specialObjects, calling ->nodeAtPoint() on each.
When there are no more, it then inspects NORMAL children by calling the base class method on
"this".
Thus, if you just filter child->isPositioned(), all floating objects will end up beiing parsed twice.

That's no good, because when innerNode is found, nodeAtPoint() proceeds unsetting mouseInside()
for EVERY remaining node, without checking if it already parsed them.
In other words, it will cancel a previous hover on floating objects instantly.

There are quite a lot of subtle problems with hovering which are caused by this double parsing.

Now, the original !child->isSpecial() was replaced by !child->isPositioned() in revision 1.171
to force the parsing of non-RenderFlow floating objects.

So a simple solution to correct everything is :

 if (!(child->isSpecial() && isFlow()) && child->nodeAtPoint(info, _x, _y, tx, ty))

That way,  specialObjects are filtered out only if the parent inherits RenderFlow.

Here is one of my testcases for the duplicate parsing error: 
http://www.phoenix-library.org/germain/testcases/bug_khtml_hover.html
Bug: #44602             
(the bug points to a real-world page, but my testcase is a reduction of it)

And for filtering out every specialObjects vs. filtering only when this->isFlow():
http://www.phoenix-library.org/germain/testcases/khtml-absolute-rendercontainer.html
Bug: #40519
(same remark as above)
------------------------------------------

3°-innerNode() and URLElement() are sometimes differents, the former being selected before
 the later, thus avoiding mouse hover (or mouse active) on some links.

I've made a drawing to make it simpler to explain the kind of problems it causes :
http://www.phoenix-library.org/germain/testcases/intersection.jpg  (kprez rulez! :)

Div A and Div B are transparent containers for Link A and Link B (they don't appear on screen).
But Div A covers a large part of Link B.
So if you click on the (blue) covered part of Link B, nodeAtPoint() will:
- track the mouse cursor to Div A
- check that mouse is outside of Link A
- proceed to assign Div A to innerNode
- continue (unsetting mouseInside() on the way) until it reaches Link B
- assign URLElement to Link B

So Link B will have the hand cursor, but hover will only be triggered if you hover the
white, non-covered part, of Link B

Example: 
----------
http://www.w3.org/Style/CSS

Here, you can't hover "CSS Test Suites" at all, because it's totally covered by transparent containers.
"What's new" only light up when you hover on the middle region, that is : between the invisible 
containers of "Learning CSS" and "Specs".

Now the rationale behind my patch :
--------------------------------------------
- All the point of having an innerNode is "there can be only one hovered branch at a time,
 and one active container if it's a click event".
- Thus there's no point in selecting an innerNode that has no "hasHover" or "hasActive", better wait until :
  A) we encounter an URLElement (and we give it the innerNode)
or
 B) we encounter a better innerNode, with hover or active.
or
 C) we default to the root node.

This doesn't change the logic : it's just about being smarter about the choice of innerNode.

+        if (!info.innerNode() && (isRoot() || (style() && style()->hasHover()) || (info.active() && style()->hasActive())))
             info.setInnerNode(element());
[...]
                     info.setURLElement(p->element());
+                    if (!info.innerNode())
+                        info.setInnerNode(element());

I'm waiting for your comments - it would be great if at least point 1) made it's way to KDE 3.1
because it's the most obvious and the most ennoying (links inaccessibles in a lot of DHTML pages).

Cheers,

Germain







-------------- next part --------------
A non-text attachment was scrubbed...
Name: render_object.cpp.3.patch
Type: text/x-diff
Size: 1552 bytes
Desc: not available
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20020713/4e0a6f88/attachment.patch>