559829 - Click on an entry in search suggestions drop-down activates a link underneath (the list is click-transparent)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[[:Aleksej]]

Fennec, branch 1.9.2, i686, on Debian GNU/Linux.
Version=1.1a2pre
BuildID=20100416015257

Steps to reproduce:
1. Go to http://www.google.com/search?newwindow=1&q=sleep&lr=&aq=f&aqi=g9g-s1&aql=&oq=&gs_rfai=
2. Click the search field and type a space (to get "sleep " and a search suggestions drop-down).
3. Click a suggestion displayed over a link, so that if there were no suggestions you would hit the link instead.

Actual results:
The link on the page is clicked.

Expected results:
The suggestion entry is used. If you miss the link area underneath the drop-down, this is what happens.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I have a patch for that which need just a few polish.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

This patch move the check to know if the element is click-able or not under the call to nodesFromRect.

This way we're able to look if the container of the element is click-able through the nsIEventListenerService.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

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

This looks like it could be a time consuming block of code.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #3)
> (From update of attachment 439587 [details] [diff] [review])
> This looks like it could be a time consuming block of code.

+
+    // If possible looks in the parents node to find a target
+    if (aElementsInRect) {
+      let count = aElementsInRect.length;
+      for (let i = 0; i < count; i++) {
+        if (aElement.parentNode == aElementsInRect[i]) {
+          let isParentClickable = this._isElementClickable(aElement.parentNode, aElementsInRect);
+          if (isParentClickable)
+            return true;
+        }
+      }
+    }


Right. I can probably refactor the code to be less time consuming.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Same things but without recursion and with a shorter path.  Because the result of nodesFromRect is ordered we can also optimize to update the parentNode once we have a hit between odesFromRect[i] == parentNode

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Initial tests on my N900 show this patch working sometimes, but failing other
times. Fail == clicks a button or link under the suggestion list.

I'll make sure my mozilla-192 tree is up to date

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #6)
> Initial tests on my N900 show this patch working sometimes, but failing other
> times. Fail == clicks a button or link under the suggestion list.
> 
> I'll make sure my mozilla-192 tree is up to date

I updated and rebuilt my mozilla-192 tree. There was a problem with my previous builds. My current build is working much better. I'll keep testing for now.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 439930 [details] [diff] [review]
Patch v0.2

Works very well

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed m-b:
http://hg.mozilla.org/mobile-browser/rev/1c14e68a8418

pushed m-1.1:
http://hg.mozilla.org/releases/mobile-1.1/rev/829466780f23

Comment 10

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.5pre) Gecko/20100506 Namoroka/3.6.5pre Fennec/1.1b2pre

and

Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:1.9.3a5pre) Gecko/20100506 Namoroka/3.7a5pre Fennec/2.0a1pre


This would be a nice addition to our FFTs under the autocomplete group

Comment 11

[[:Aleksej]]

Verified with the 20100506 builds for i686, too.

Comment 12

[Martijn Wargers (dead)]

I'm reassigning this to myself for adding litmus testcases for this (aakashd discussed this on irc with blassey and ted on #mobile).

Comment 13

[Ioana Chiorean]

https://litmus.mozilla.org/show_test.cgi?id=15193